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 flag to warn about unreachable branches and exprs #7050

Merged
merged 7 commits into from
Jun 28, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 25 additions & 1 deletion mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -1777,13 +1777,36 @@ def check_import(self, node: ImportBase) -> None:

def visit_block(self, b: Block) -> None:
if b.is_unreachable:
# This block was marked as being unreachable during semantic analysis.
# It turns out any blocks marked in this way are *intentionally* marked
# as unreachable -- so we don't display an error.
self.binder.unreachable()
return
for s in b.body:
if self.binder.is_unreachable():
if self.options.disallow_inferred_unreachable and not self.is_raising_or_empty(s):
self.msg.unreachable_branch(s)
break
self.accept(s)

def is_raising_or_empty(self, s: Statement) -> bool:
Michael0x2a marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(s, AssertStmt) and is_false_literal(s.expr):
return True
elif isinstance(s, (RaiseStmt, PassStmt)):
return True
elif isinstance(s, ExpressionStmt):
if isinstance(s.expr, EllipsisExpr):
return True
elif isinstance(s.expr, CallExpr):
self.expr_checker.msg.disable_errors()
typ = self.expr_checker.accept(
s.expr, allow_none_return=True, always_allow_any=True)
self.expr_checker.msg.enable_errors()

if isinstance(typ, UninhabitedType):
return True
return False

def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
"""Type check an assignment statement.

Expand Down Expand Up @@ -2956,7 +2979,8 @@ def visit_try_stmt(self, s: TryStmt) -> None:
# type checks in both contexts, but only the resulting types
# from the latter context affect the type state in the code
# that follows the try statement.)
self.accept(s.finally_body)
if not self.binder.is_unreachable():
self.accept(s.finally_body)

def visit_try_without_finally(self, s: TryStmt, try_frame: bool) -> None:
"""Type check a try statement, ignoring the finally block.
Expand Down
30 changes: 26 additions & 4 deletions mypy/checkexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -2367,14 +2367,25 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
restricted_left_type = true_only(left_type)
result_is_left = not left_type.can_be_false

# If right_map is None then we know mypy considers the right branch
# to be unreachable and therefore any errors found in the right branch
# should be suppressed.
#
# Note that we perform these checks *before* we take into account
# the analysis from the semanal phase below. We assume that nodes
# marked as unreachable during semantic analysis were done so intentionally.
# So, we shouldn't report an error.
if self.chk.options.disallow_inferred_unreachable:
if left_map is None:
self.msg.always_same_truth_value_left_operand(e.op, e.left)
if right_map is None:
self.msg.unreachable_right_operand(e.op, e.right)

if e.right_unreachable:
right_map = None
elif e.right_always:
left_map = None

# If right_map is None then we know mypy considers the right branch
# to be unreachable and therefore any errors found in the right branch
# should be suppressed.
if right_map is None:
self.msg.disable_errors()
try:
Expand Down Expand Up @@ -3174,19 +3185,30 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No
self.accept(condition)

# values are only part of the comprehension when all conditions are true
true_map, _ = self.chk.find_isinstance_check(condition)
true_map, false_map = self.chk.find_isinstance_check(condition)

if true_map:
for var, type in true_map.items():
self.chk.binder.put(var, type)

if self.chk.options.disallow_inferred_unreachable:
if true_map is None:
self.msg.comprehension_cond_always_same(False, condition)
elif false_map is None:
self.msg.comprehension_cond_always_same(True, condition)

def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
self.accept(e.cond)
ctx = self.type_context[-1]

# Gain type information from isinstance if it is there
# but only for the current expression
if_map, else_map = self.chk.find_isinstance_check(e.cond)
if self.chk.options.disallow_inferred_unreachable:
if if_map is None:
self.msg.unreachable_branch_in_inline_if('if', e.cond)
elif else_map is None:
self.msg.unreachable_branch_in_inline_if('else', e.cond)

if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)

Expand Down
4 changes: 4 additions & 0 deletions mypy/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ def add_invertible_flag(flag: str,
help="Treat imports as private unless aliased",
group=strictness_group)

add_invertible_flag('--disallow-inferred-unreachable', default=False, strict_flag=False,
Michael0x2a marked this conversation as resolved.
Show resolved Hide resolved
help="Disallow branches inferred to be unreachable after type analysis",
group=strictness_group)

incremental_group = parser.add_argument_group(
title='Incremental mode',
description="Adjust how mypy incrementally type checks and caches modules. "
Expand Down
24 changes: 24 additions & 0 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about renaming this to unreachable_statement? Would it be clearer (and correct)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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'.
Expand Down
5 changes: 5 additions & 0 deletions mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class BuildType:
"disallow_any_generics",
"disallow_any_unimported",
"disallow_incomplete_defs",
"disallow_inferred_unreachable",
"disallow_subclassing_any",
"disallow_untyped_calls",
"disallow_untyped_decorators",
Expand Down Expand Up @@ -164,6 +165,10 @@ def __init__(self) -> None:
# This makes 1 == '1', 1 in ['1'], and 1 is '1' errors.
self.strict_equality = False

# Report an error for any branches inferred to be unreachable as a result of
# type analysis.
self.disallow_inferred_unreachable = False

# Variable names considered True
self.always_true = [] # type: List[str]

Expand Down
176 changes: 176 additions & 0 deletions test-data/unit/check-unreachable-code.test
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something like DEBUG: Final = False followed by if DEBUG: print('foo')? Is there a test for this use case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 if DEBUG: ... else: ... block.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea for message: Statement is unreachable

Even better, maybe we can somehow detect that this is caused by an always-true isinstance check and generate an error on the line with the isinstance check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 unreachable() for easily silencing errors about unreachable code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 mypy_extensions? Or maybe that's overkill?

Another idea: once the "tag all error messages with a code" PR is landed, the user could maybe add a # type: ignore for just the "Statement is unreachable" warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 unreachable() are actually unreachable?

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 reveal_type(...) on the provided args before reporting an error that the statement was actually reachable. This would also let us supplant this "assert never" pattern (and providing a runtime implementation of the same logic in mypy_extensions or something would feel less like overkill).

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 x if we hit an error.

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]