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

Fix reachability inference with 'isinstance(Any, Any)' #7048

Merged

Conversation

@Michael0x2a
Copy link
Collaborator

commented Jun 24, 2019

While experimenting with some reachability inference logic, I discovered the following program does not seem to behave as expected:

from typing import Any
from foo import A  # type: ignore
x: Any
if isinstance(x, A):
    reveal_type(x)
else:
    reveal_type(x)

Both branches really ought to be reachable: since both x and A are of type Any, we really can't say whether or not any particular branch will run: both branches ought to be reachable.

However, mypy currently instead assumes that only the first branch is reachable and does not type-check the second branch. So in this example, only the first reveal_type(...) outputs a note.

This pull request fixes this bug.

Michael0x2a added some commits Jun 24, 2019

Fix reachability inference with 'isinstance(Any, Any)'
While experimenting with some reachability inference logic, I discovered
the following program does not seem to behave as expected:

```python
from typing import Any
from foo import A  # type: ignore
x: Any
if isinstance(x, A):
    reveal_type(x)
else:
    reveal_type(x)
```

Both branches really ought to be reachable: since both `x` and `A` are
of type `Any`, we really can't say whether or not any particular branch
will run.

However, mypy currently instead assumes that only the first branch is
reachable and does not type-check the second branch. So in this example,
only the first `reveal_type(...)` outputs a note.

This pull request fixes this bug.

Michael0x2a added a commit to Michael0x2a/mypy that referenced this pull request Jun 24, 2019

Add flag to warn about unreachable branches and exprs
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.
@msullivan
Copy link
Collaborator

left a comment

Great catch. Looks good.

@Michael0x2a Michael0x2a merged commit d748e77 into python:master Jun 26, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:fix-isinstance-check-with-dual-any branch Jun 26, 2019

JukkaL added a commit that referenced this pull request Jun 28, 2019

Add flag to warn about unreachable branches and exprs (#7050)
This diff adds a `--warn-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 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 #7048)

2. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.