Add flag to warn about unreachable branches and exprs #7050
Conversation
This diff adds a `--disallow-inferred-unreachable` flag that reports an error if: 1. Any branch is inferred to be unreachable 2. Any subexpression in boolean expressions, inline if statements, and list/set/generator/dict comprehensions are always unreachable or redundant. This only takes into account the results of *type* analysis. We do not report errors for branches that are inferred to be unreachable in the semantic analysis phase (e.g. due to `sys.platform` checks and the like). Those checks are all intentional/deliberately flag certain branches as unreachable, so error messages would be annoying in those cases. A few additional notes: 1. I didn't spend a huge amount of time trying to come up with error messages. They could probably do with some polishing/rewording. 2. I thought about enabling this flag for default with mypy, but unfortunately that ended up producing ~40 to 50 (apparently legitimate) error messages. About a fourth of them were due to runtime checks checking to see if some type is None (despite not being declared to be Optional), about a fourth seemed to be due to mypy not quite understanding how to handle things like traits and Bogus[...], and about a fourth seemed to be legitimately unnecessary checks we could safely remove. The final checks were a mixture of typeshed bugs and misc errors with the reachability checks. (e.g. see python#7048) 3. For these reasons, I decided against adding this flag to `--strict` and against documenting it yet. We'll probably need to flush out a few bugs in mypy first/get mypy to the state where we can at least honestly dogfood this. Resolves python#2395; partially addresses python#7008.
mypy/messages.py
Outdated
| @@ -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.
What do you think about renaming this to unreachable_statement? Would it be clearer (and correct)?
There was a problem hiding this comment.
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).
| @@ -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.
What about something like DEBUG: Final = False followed by if DEBUG: print('foo')? Is there a test for this use case?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
The current behavior seems reasonable to me.
|
@JukkaL -- Thanks for the CR! I updated the PR; this should be ready for a second look. |
|
I tried this PR on some of our code. It found a bunch of useless [Edit: this is discussed above already - didn't notice.] I was interested to check whether this would "break" the from __future__ import annotations
from typing import Union, NoReturn
def assert_never(value: NoReturn) -> NoReturn:
assert False, f'Unhandled value: {value} ({type(value).__name__})'
def f(x: Union[int, str]) -> None:
if isinstance(x, int):
pass
elif isinstance(x, str):
pass
else:
# Triggers "Statement is unreachable".
# print('xx')
# Doesn't trigger.
assert_never(x) |
|
Nice feature! Would be great to add docs for this. |
This diff adds a
--warn-unreachableflag that reports an error if:This only takes into account the results of type analysis. We do not report errors for statements that are inferred to be unreachable in the semantic analysis phase (e.g. due to
sys.platformchecks and the like).Those checks are all intentional/deliberately flag certain branches as unreachable, so error messages would be annoying in those cases.
A few additional notes:
I didn't spend a huge amount of time trying to come up with error messages. They could probably do with some polishing/rewording.
I thought about enabling this flag for default with mypy, but unfortunately that ended up producing ~40 to 50 (apparently legitimate) error messages.
About a fourth of them were due to runtime checks checking to see if some type is None (despite not being declared to be Optional), about a fourth seemed to be due to mypy not quite understanding how to handle things like traits and Bogus[...], and about a fourth seemed to be legitimately unnecessary checks we could safely remove.
The final fourth was a mixture of typeshed bugs and misc errors with the reachability checks. (e.g. see Fix reachability inference with 'isinstance(Any, Any)' #7048)
For these reasons, I decided against adding this flag to
--strictand against documenting it yet. We'll probably need to flush out a few bugs in mypy first/get mypy to the state where we can at least honestly dogfood this.Resolves #2395; partially addresses #7008.