-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add flag to warn about unreachable branches and exprs #7050
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
Changes from 1 commit
a5b3737
615f3b2
ce8c357
74663e8
d16dd8e
0ccb367
373c6ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1228,6 +1228,30 @@ def note_call(self, subtype: Type, call: Type, context: Context) -> None: | |
self.note('"{}.__call__" has type {}'.format(self.format_bare(subtype), | ||
self.format(call, verbosity=1)), context) | ||
|
||
def unreachable_branch(self, context: Context) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about renaming this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! I think it'd be more correct, especially since we're also reporting these errors after things like asserts and returns. (I also added a test case for the latter). |
||
self.fail("This branch is inferred to be unreachable", context) | ||
|
||
def always_same_truth_value_left_operand(self, op_name: str, context: Context) -> None: | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
value = 'true' if op_name == 'and' else 'false' | ||
self.fail( | ||
"The left operand of this '{}' expression is always {}".format(op_name, value), | ||
context, | ||
) | ||
|
||
def unreachable_right_operand(self, op_name: str, context: Context) -> None: | ||
self.fail( | ||
"The right operand of this '{}' expression is never evaluated".format(op_name), | ||
context, | ||
) | ||
|
||
def comprehension_cond_always_same(self, simplified_result: bool, context: Context) -> None: | ||
template = "The conditional check in this comprehension is always {}" | ||
self.fail(template.format(str(simplified_result).lower()), context) | ||
|
||
def unreachable_branch_in_inline_if(self, sub_expr_name: str, context: Context) -> None: | ||
template = "The '{}' expression in this inline if expression is never evaluated" | ||
self.fail(template.format(sub_expr_name), context) | ||
|
||
def report_protocol_problems(self, subtype: Union[Instance, TupleType, TypedDictType], | ||
supertype: Instance, context: Context) -> None: | ||
"""Report possible protocol conflicts between 'subtype' and 'supertype'. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -715,3 +715,179 @@ if sys.version_info[0] >= 2: | |
reveal_type('') # N: Revealed type is 'builtins.str' | ||
reveal_type('') # N: Revealed type is 'builtins.str' | ||
[builtins fixtures/ops.pyi] | ||
|
||
[case testUnreachableFlagWithBadControlFlow] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our reachability checks currently don't special-case Literal bools (or Finals that are bools) at all -- we'd actually just end up checking both branches in the I'm not sure exactly what behavior we want though, which is why I refrained from adding a test case. (On one hand, it'd be nice to make mypy smarter about reachability in general; on the other improving mypy's understanding would lead to actual regressions in type inference if users don't opt-in to using this new flag.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current behavior seems reasonable to me. |
||
# flags: --disallow-inferred-unreachable | ||
a: int | ||
if isinstance(a, int): | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
reveal_type(a) # N: Revealed type is 'builtins.int' | ||
else: | ||
reveal_type(a) # E: This branch is inferred to be unreachable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idea for message: Even better, maybe we can somehow detect that this is caused by an always-true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I experimented a bit with this idea. I think switching to reporting an error on the check might be a bit messy -- it'd actually be much easier to report both error message. Do you think that's ok? If so, I can work on submitting a separate PR with this change after this one is landed. (I'll probably need to do some mild refactoring so we can reuse the "should we report a warning with this line?" logic, and I'd prefer to do that in a separate branch.) |
||
|
||
b: int | ||
while isinstance(b, int): | ||
reveal_type(b) # N: Revealed type is 'builtins.int' | ||
else: | ||
reveal_type(b) # E: This branch is inferred to be unreachable | ||
|
||
def foo(c: int) -> None: | ||
reveal_type(c) # N: Revealed type is 'builtins.int' | ||
assert not isinstance(c, int) | ||
reveal_type(c) # E: This branch is inferred to be unreachable | ||
|
||
d: int | ||
if False: | ||
reveal_type(d) # E: This branch is inferred to be unreachable | ||
|
||
e: int | ||
if True: | ||
reveal_type(e) # N: Revealed type is 'builtins.int' | ||
else: | ||
reveal_type(e) # E: This branch is inferred to be unreachable | ||
|
||
[builtins fixtures/isinstancelist.pyi] | ||
|
||
[case testUnreachableFlagTryBlocks] | ||
# flags: --disallow-inferred-unreachable | ||
|
||
def foo(x: int) -> int: | ||
try: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
return x | ||
reveal_type(x) # E: This branch is inferred to be unreachable | ||
finally: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
if True: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
else: | ||
reveal_type(x) # E: This branch is inferred to be unreachable | ||
|
||
def bar(x: int) -> int: | ||
try: | ||
if True: | ||
raise Exception() | ||
reveal_type(x) # E: This branch is inferred to be unreachable | ||
except: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
return x | ||
else: | ||
reveal_type(x) # E: This branch is inferred to be unreachable | ||
|
||
def baz(x: int) -> int: | ||
try: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
except: | ||
# Mypy assumes all lines could throw an exception | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
return x | ||
else: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
return x | ||
[builtins fixtures/exception.pyi] | ||
|
||
[case testUnreachableFlagIgnoresSemanticAnalysisUnreachable] | ||
# flags: --disallow-inferred-unreachable --python-version 3.7 --platform win32 --always-false FOOBAR | ||
import sys | ||
from typing import TYPE_CHECKING | ||
|
||
x: int | ||
if TYPE_CHECKING: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if sys.platform == 'darwin': | ||
reveal_type(x) | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if sys.version_info == (2, 7): | ||
reveal_type(x) | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
FOOBAR = "" | ||
if FOOBAR: | ||
reveal_type(x) | ||
else: | ||
reveal_type(x) # N: Revealed type is 'builtins.int' | ||
[builtins fixtures/ops.pyi] | ||
|
||
[case testUnreachableFlagIgnoresSemanticAnalysisExprUnreachable] | ||
# flags: --disallow-inferred-unreachable --always-false FOOBAR | ||
import sys | ||
from typing import TYPE_CHECKING | ||
|
||
FOOBAR = "" | ||
|
||
def foo() -> bool: ... | ||
|
||
lst = [1, 2, 3] | ||
|
||
a = FOOBAR and foo() | ||
b = (not FOOBAR) or foo() | ||
c = 1 if FOOBAR else 2 | ||
d = [x for x in lst if FOOBAR] | ||
[builtins fixtures/list.pyi] | ||
|
||
[case testUnreachableFlagOkWithDeadStatements] | ||
# flags: --disallow-inferred-unreachable | ||
from typing import NoReturn | ||
def assert_never(x: NoReturn) -> NoReturn: | ||
assert False | ||
|
||
def nonthrowing_assert_never(x: NoReturn) -> None: ... | ||
|
||
def expect_str(x: str) -> str: pass | ||
|
||
x: int | ||
if False: | ||
assert False | ||
reveal_type(x) | ||
|
||
if False: | ||
raise Exception() | ||
reveal_type(x) | ||
|
||
if False: | ||
assert_never(x) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random thought (no need to address in this PR): What do you think about providing a magic utility function such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That could be handy, though possibly annoying to use for people with linters (since they don't understand mypy magic functions). We could work around this by adding the function to Another idea: once the "tag all error messages with a code" PR is landed, the user could maybe add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or another idea -- what if we instead modified mypy so that we mandate that any calls to We could also modify this function so that optionally take in any number of arguments: if the checker runs into this function, it'll run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that would be how I imagined it to work. Not sure about the arguments though. My idea would be to do something like this: X = Union[A, B]
def f(x: X) -> None:
if isinstance(x, A):
...
elif isinstance(x, B):
...
else:
unreachable() # Error only if we forgot to handle some case Maybe we could automatically recognize that pattern and print out the inferred type of |
||
reveal_type(x) | ||
|
||
if False: | ||
nonthrowing_assert_never(x) # E: This branch is inferred to be unreachable | ||
reveal_type(x) | ||
|
||
if False: | ||
# Ignore obvious type errors | ||
assert_never(expect_str(x)) | ||
reveal_type(x) | ||
[builtins fixtures/exception.pyi] | ||
|
||
[case testUnreachableFlagExpressions] | ||
# flags: --disallow-inferred-unreachable | ||
def foo() -> bool: ... | ||
|
||
lst = [1, 2, 3, 4] | ||
|
||
a = True or foo() # E: The right operand of this 'or' expression is never evaluated | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
b = False or foo() # E: The left operand of this 'or' expression is always false | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
c = True and foo() # E: The left operand of this 'and' expression is always true | ||
d = False and foo() # E: The right operand of this 'and' expression is never evaluated | ||
e = True or (True or (True or foo())) # E: The right operand of this 'or' expression is never evaluated | ||
f = (True or foo()) or (True or foo()) # E: The right operand of this 'or' expression is never evaluated | ||
g = 3 if True else 4 # E: The 'else' expression in this inline if expression is never evaluated | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
h = 3 if False else 4 # E: The 'if' expression in this inline if expression is never evaluated | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
i = [x for x in lst if True] # E: The conditional check in this comprehension is always true | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
j = [x for x in lst if False] # E: The conditional check in this comprehension is always false | ||
|
||
k = [x for x in lst if isinstance(x, int) or foo()] # E: The conditional check in this comprehension is always true \ | ||
# E: The right operand of this 'or' expression is never evaluated | ||
[builtins fixtures/isinstancelist.pyi] | ||
|
||
[case testUnreachableFlagMiscTestCaseMissingMethod] | ||
# flags: --disallow-inferred-unreachable | ||
|
||
class Case1: | ||
def test1(self) -> bool: | ||
return False and self.missing() # E: The right operand of this 'and' expression is never evaluated | ||
|
||
def test2(self) -> bool: | ||
return not self.property_decorator_missing and self.missing() # E: The right operand of this 'and' expression is never evaluated | ||
|
||
def property_decorator_missing(self) -> bool: | ||
return True | ||
[builtins fixtures/bool.pyi] |
Uh oh!
There was an error while loading. Please reload this page.