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

Update imported Vars in the third pass #5635

Merged
merged 13 commits into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Sep 18, 2018

Fixes #5275
Fixes #4498
Fixes #4442

This is a simple band-aid fix for Invalid type in import cycles where type aliases, named tuples, or typed dicts appear. Note that this is a partial fix that only fixes the Invalid type error when a type alias etc. appears in type context. This doesn't fix errors (e.g. Cannot determine type of X) that may appear if the type alias etc. appear in runtime context.

The motivation for partial fix is two-fold:

  • The error often appears in stub files (especially for large libraries/frameworks) where we have more import cycles, but no runtime context at all.
  • Ideally we should refactor semantic analysis to have deferred nodes, and process them in smaller passes when there is more info (like we do in type checking phase). But this is much harder since this requires a large refactoring. Also an alternative fix of updating node of every NameExpr and MemberExpr in third pass is costly from performance point of view, and still nontrivial.

@ilevkivskyi ilevkivskyi requested a review from JukkaL Sep 18, 2018

@JukkaL

Hmm I'm not sure if this fixes the problem in general. What if there are arbitrary references to the imported name that are bound during pass 2 in the module that contains the import -- I think that they don't necessarily get updated.

from b import tp
x: tp
reveal_type(x.x) # E: Revealed type is 'builtins.int'

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

What if you have tp('x') in this file? Or reveal_type(tp)?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 19, 2018

Collaborator

Yes, this will not work. I was thinking about the Invalid type problem. All references in type context will be correctly re-resolved in the third pass. While there is no such thing as forward reference in runtime context.
I however still think this fix may have some value:

  • We have seen many complains about Invalid type, but few complains about Cannot determine type (the error that appears if you try tp('x'))
  • The reason for above fact is that many of the import cycle exist only for annotations, tp('x') will fail at runtime anyway
  • This error quite often appears in stub files for large libraries, where we only care about type context

It is up to you of course.

ilevkivskyi and others added some commits Sep 20, 2018

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 20, 2018

@JukkaL As you requested elsewhere I added a detailed doctring, tests for still not fixed behavior, and updated the PR description.

Ivan Levkivskyi
@JukkaL

JukkaL approved these changes Sep 21, 2018

Thanks for the very clear docstrings! This looks good now (just left a few nits). Please try this with internal Dropbox codebases before merging, though.

A = List[int]
Are seen by mypy as variables, because it doesn't know yet that `List` refers to a type.

This comment has been minimized.

@JukkaL

JukkaL Sep 21, 2018

Collaborator

Grammar nit: use lower case 'a' in 'are'.

In the second pass, such `Var` is replaced with a `TypeAlias`. But in import cycle,
import of `A` will still refer to the old `Var` node. Therefore we need to update it.
Note that this a partial fix that only fixes the "Invalid type" error when a type alias

This comment has been minimized.

@JukkaL

JukkaL Sep 21, 2018

Collaborator

'this a' -> 'this is a'

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented Sep 21, 2018

It would also be good to create a follow-up issue for the remaining problems, if one doesn't exist yet.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2018

It would also be good to create a follow-up issue for the remaining problems, if one doesn't exist yet.

It looks like an issue about Cannot determine type in import cycle was closed as solved
#2015, but the solution doesn't work with named tuples and typed dicts. I think I will open a new issue, and link the old one for more context.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2018

This causes one redundant cast and one real error (bad annotation).

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2018

Appveyor failure looks like a flake.

@ilevkivskyi ilevkivskyi merged commit 792bcdc into python:master Sep 21, 2018

1 of 2 checks passed

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

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:try-fix-import-alias branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment