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

Re-broaden type to Any if it started as Any but was narrowed in an enclosing frame #3361

Merged
merged 5 commits into from Jul 11, 2017

Conversation

Projects
None yet
3 participants
@lincolnq
Contributor

lincolnq commented May 13, 2017

Fixes #3338.

  • Note 1: I don't understand mypy very well yet so be warned that this may break things I don't understand.
  • Note 2: it looks like I was confused in the original bug report, and attributing the problem to strict-optional was actually a red herring. So I created a new test case for which strict-optional isn't needed.

My investigation notes:

The explanation is that the bug is caused by an interaction between most_recent_enclosing_type and its caller, assign_type. assign_type has a special case for when the most recent enclosing type is Any, commented with the following note: "If x is Any and y is int, after x = y we do not infer that x is int. This could be changed." The special case does nothing (the normal case assigns the inferred type to the variable).

In my test case, most_recent_enclosing_type misses the frame which reassigns v because int is not a subtype of str, and so returns Any and so the special case applies (this explains why declaring with type Union doesn't reproduce the problem). So it doesn't assign any new type for this assignment, causing the bug.

I took the approach of fixing that special case so if type is not bound to Any at the moment of assignment, it re-binds it to Any (or, really, whatever is the result of most_recent_enclosing_type).

Another approach might be to remove the subtype check from most_recent_enclosing_type. https://github.com/python/mypy/compare/master...lincolnq:lincoln/remove-subtype-check-from-binder?expand=1 is where I tried that. I thought that change was more aggressive / likely to break things, and I didn't understand why the subtype check was put there in the first place. But it has the desirable (to me) property of inferring int on the reassignment rather than Any.

Both approaches pass the automated tests.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 16, 2017

Member

Thanks for your contribution! We're currently getting ready for PyCon but hopefully we'll get to reviewing this PR soon.

Member

gvanrossum commented May 16, 2017

Thanks for your contribution! We're currently getting ready for PyCon but hopefully we'll get to reviewing this PR soon.

@ddfisher

This looks good! Thanks for the detailed explanation.

@ddfisher ddfisher merged commit 677a6fc into python:master Jul 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment