Add flag to warn about unreachable branches and exprs by Michael0x2a · Pull Request #7050 · python/mypy

Michael0x2a · 2019-06-24T08:54:06Z

This diff adds a --warn-unreachable flag that reports an error if:

Any statement is inferred to be unreachable
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 statements 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:

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 --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 #2395; partially addresses #7008.

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.

JukkaL

Thanks for working on this -- I've wanted something like this for a long time. Looks mostly good, I mostly had a few nits and ideas about improving wording of error messages.

JukkaL · 2019-06-25T16:29:28Z

        self.note('"{}.__call__" has type {}'.format(self.format_bare(subtype),
                                                     self.format(call, verbosity=1)), context)

+    def unreachable_branch(self, context: Context) -> None:


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

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

JukkaL · 2019-06-25T16:31:21Z

 reveal_type('')  # N: Revealed type is 'builtins.str'
 [builtins fixtures/ops.pyi]
+
+[case testUnreachableFlagWithBadControlFlow]


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

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

The current behavior seems reasonable to me.

Michael0x2a · 2019-06-26T16:19:58Z

@JukkaL -- Thanks for the CR! I updated the PR; this should be ready for a second look.

bluetech · 2019-06-28T12:04:05Z

I tried this PR on some of our code. It found a bunch of useless is None checks (and a typeshed bug), which is nice.

[Edit: this is discussed above already - didn't notice.] I was interested to check whether this would "break" the assert_never trick from #5818, which is like the opposite of this feature ("warn if this is not unreachable"), but the warning is not triggered in that case, which is good. I used this:

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)

JukkaL

Thanks for the updates! Looks great!

ilevkivskyi · 2019-06-28T12:38:39Z

Nice feature! Would be great to add docs for this.

JukkaL reviewed Jun 25, 2019

View reviewed changes

Michael0x2a added 6 commits June 25, 2019 20:38

Rename flag to '--warn-unreachable'

615f3b2

Apply trivial renames

ce8c357

Rename functions; adjust code to handle TypeVars with value restrictions

74663e8

Adjust help text for flag

d16dd8e

Add suggested test cases for platform and version checks

0ccb367

Add test case for statement after return

373c6ec

JukkaL approved these changes Jun 28, 2019

View reviewed changes

JukkaL merged commit 8030804 into python:master Jun 28, 2019

Michael0x2a deleted the unreachability-warnings branch June 28, 2019 14:33

Conversation

Michael0x2a commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JukkaL Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Michael0x2a Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JukkaL Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Michael0x2a Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

JukkaL Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

Michael0x2a commented Jun 26, 2019

Uh oh!

bluetech commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Jun 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Michael0x2a commented Jun 24, 2019 •

edited

Loading

bluetech commented Jun 28, 2019 •

edited

Loading