Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add value_always_false #817

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Add `value_always_false` error code to catch errors like
asserting on a value that is statically known to be None (#817)
- Flag invalid regexes in arguments to functions like
`re.search` (#816)

Expand Down
1 change: 1 addition & 0 deletions pyanalyze/error_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def __iter__(self) -> Iterator[Error]:
Error("missing_parameter_annotation", "Missing function parameter annotation"),
Error("type_always_true", "Type will always evaluate to 'True'"),
Error("value_always_true", "Value will always evaluate to 'True'"),
Error("value_always_false", "Value will always evaluate to False"),
Error("type_does_not_support_bool", "Type does not support bool()"),
Error("missing_return", "Function may exit without returning a value"),
Error(
Expand Down
25 changes: 23 additions & 2 deletions pyanalyze/name_check_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@
replace_known_sequence_value,
set_self,
stringify_object,
unannotate,
unannotate_value,
unite_and_simplify,
unite_values,
Expand Down Expand Up @@ -4204,7 +4205,7 @@ def visit_Assert(self, node: ast.Assert) -> None:
self._set_name_in_scope(LEAVES_SCOPE, node, AnyValue(AnySource.marker))
# We don't check value_always_true here; it's fine to have an assertion
# that the type checker statically thinks is True.
self._check_boolability(test, node, disabled={ErrorCode.value_always_true})
self._check_boolability(test, node.test, disabled={ErrorCode.value_always_true})

def add_constraint(self, node: object, constraint: AbstractConstraint) -> None:
if constraint is NULL_CONSTRAINT:
Expand Down Expand Up @@ -4606,7 +4607,11 @@ def constraint_from_condition(
if check_boolability:
disabled = set()
else:
disabled = {ErrorCode.type_always_true, ErrorCode.value_always_true}
disabled = {
ErrorCode.type_always_true,
ErrorCode.value_always_true,
ErrorCode.value_always_false,
}
self._check_boolability(condition, node, disabled=disabled)
return condition, constraint

Expand Down Expand Up @@ -4638,6 +4643,22 @@ def _check_boolability(
f"{value} is always True",
error_code=ErrorCode.value_always_true,
)
elif boolability is Boolability.value_always_false:
if (
ErrorCode.value_always_false not in disabled
# allow "assert False"
and not (isinstance(node, ast.Constant) and node.value in (0, False))
and not (
# Allow e.g. "if TYPE_CHECKING"
unannotate(value) in (KnownValue(False), KnownValue(0))
and isinstance(node, ast.Name)
)
):
self.show_error(
node,
f"{value} is always False",
error_code=ErrorCode.value_always_false,
)

def visit_Expr(self, node: ast.Expr) -> Value:
value = self.visit(node.value)
Expand Down
19 changes: 16 additions & 3 deletions pyanalyze/test_boolability.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,15 @@ def test_get_boolability() -> None:
class TestAssert(TestNameCheckVisitorBase):
@assert_passes()
def test_assert_never_fails(self):
def func():
pass

def capybara():
tpl = "this", "doesn't", "work"
assert tpl # E: type_always_true

assert func # E: type_always_true

@assert_passes()
def test_assert_bad_bool(self):
class X(object):
Expand All @@ -152,6 +157,14 @@ def __bool__(self):
def capybara():
assert x # E: type_does_not_support_bool

@assert_passes()
def test_assert_none(self):
def f() -> None:
pass

def capybara():
assert f() # E: value_always_false


class TestConditionAlwaysTrue(TestNameCheckVisitorBase):
@assert_passes()
Expand Down Expand Up @@ -185,10 +198,10 @@ def __len__(self):
def test_object():
obj = object()

def capybara():
def capybara(x):
True if obj else False # E: type_always_true
obj and False # E: type_always_true
[] and obj and False # E: type_always_true
obj and x # E: type_always_true
[] and obj and x # E: type_always_true
obj or True # E: type_always_true
not obj # E: type_always_true

Expand Down
2 changes: 1 addition & 1 deletion pyanalyze/test_implementation.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ def render_feedback_text():
z = []

detail_text = None
if detail_text:
if detail_text: # E: value_always_false
assert_is_value(detail_text, NO_RETURN_VALUE)
z += detail_text

Expand Down
2 changes: 1 addition & 1 deletion pyanalyze/test_unsafe_comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,6 @@ def capybara(
):
assert x == y # E: unsafe_comparison
assert y == x # E: unsafe_comparison
assert fe1 == y # OK
assert fe1 == y # E: value_always_false
assert fe2 == y # OK
assert fe3 == y # OK
Loading