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 issues related to indirect imports in import cycles #4695

Merged
merged 14 commits into from Mar 8, 2018

Conversation

Projects
None yet
3 participants
@JukkaL
Collaborator

JukkaL commented Mar 7, 2018

This adds supports for some cases of importing an imported name
within an import cycle. Originally they could result in false positives
or false negatives.

The idea is to use a new node type ImportedName in semantic
analysis pass 1 to represent an indirect reference to a name in another
module. It will get resolved in semantic analysis pass 2.

ImportedName is not yet used everywhere where it could make
sense and this doesn't fix all related issues with import cycles.

Also did a bit of refactoring of type semantic analysis to avoid passing
multiple callback functions.

Fixes #4049.
Fixes #4429.
Fixes #4682.

Borrowed test cases from #4495 by @carljm. This PR is an alternative
for #4495.

Supersedes #4495

JukkaL added some commits Mar 5, 2018

Resolve more precise types for some imports in import cycles
Previously we could fall back to the `Any` type if we imported an
imported name. This depended on the order in which an SCC was
processed. This fixes the behavior for `from m import x` and
also makes the behavior consistent with fine-grained incremental
mode.

I'll fix other kinds of imports separately.
Refactoring: use abstract class instead of passing multiple callbacks
This makes it easier to add additional functions to the interface.
Remove type forward reference handling to maintain backward compatibi…
…lity

The previous, more correct behavior caused too many breakages in an
internal Dropbox repository. I'll enable this later after the breakages
have been fixed.
@ilevkivskyi

Thanks! I have few questions/suggestions.

@@ -1573,6 +1603,14 @@ def process_import_over_existing_name(self,
def normalize_type_alias(self, node: SymbolTableNode,
ctx: Context) -> Optional[SymbolTableNode]:
"""If node refers to a built-intype alias, normalize it.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 7, 2018

Collaborator

Typo buit-intype (space mising).

existing.node != node.node) and existing.kind != UNBOUND_IMPORTED:
if (existing
and (not isinstance(node.node, MypyFile) or existing.node != node.node)
and existing.kind != UNBOUND_IMPORTED

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 7, 2018

Collaborator

I would expect that the goal is to get rid of UNBOUND_IMPORTED and replace all such checks with isinstance(node.node, ImportedName), what I am missing here?

This comment has been minimized.

@JukkaL

JukkaL Mar 8, 2018

Collaborator

That's the end goal. This PR doesn't remove all instance of UNBOUND_IMPORTED. In particular, import m statements generate them still. I'll fix them in a separate PR.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 8, 2018

Collaborator

I'll fix them in a separate PR.

OK, having smaller PRs makes sense.

self.errors.report(ctx.get_line(), ctx.get_column(), msg)
def fail_blocker(self, msg: str, ctx: Context) -> None:
self.fail(msg, ctx, blocker=True)
def note(self, msg: str, ctx: Context) -> None:
self.sem.note(msg, ctx)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 7, 2018

Collaborator

What is the reason we can reuse self.sem.note here but not self.sem.fail above?

This comment has been minimized.

@JukkaL

JukkaL Mar 8, 2018

Collaborator

These were different originally, but it seems like there's no good reason. I'll unify them.

@@ -284,6 +284,8 @@ class TypeOfAny(Enum):
special_form = 'special_form'
# Does this Any come from interaction with another Any?
from_another_any = 'from_another_any'
# Does this Any come from an implementation limitation/bug?
implementation_artifact = 'implementation_artifact'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 7, 2018

Collaborator

Big +1 for adding this. I think I have seen other cases where this would be appropriate (but can't remember them now).

[case testIndirectFromImportWithinCycle]
import a
[file a.py]
from b import f

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 7, 2018

Collaborator

Would it make sense to check that the result is independent on the order of import statements (just in case) here and/or in other tests?

This comment has been minimized.

@JukkaL

JukkaL Mar 8, 2018

Collaborator

I'll add a few test cases where we reverse the shape of the import cycle relative to the main module.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Mar 7, 2018

(I updated the description to mention that this also fixes #4682 since I found the relevant code and tests.)

@ilevkivskyi

Looks good! I also edited the description to add Supersedes #4495, this should automatically close the old PR when this is merged (cool new GitHub feature :-))

@JukkaL JukkaL merged commit 0b7597c into master Mar 8, 2018

3 checks passed

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

This comment has been minimized.

Contributor

carljm commented Mar 8, 2018

Confirmed that this fixes the problems in our codebase. Thanks @JukkaL and @ilevkivskyi !

@gvanrossum gvanrossum deleted the imports-in-cycles branch Mar 15, 2018

yedpodtrzitko added a commit to kiwicom/mypy that referenced this pull request Mar 15, 2018

Fix issues related to indirect imports in import cycles (python#4695)
This adds supports for some cases of importing an imported name
within an import cycle. Originally they could result in false positives
or false negatives.

The idea is to use a new node type ImportedName in semantic
analysis pass 1 to represent an indirect reference to a name in another
module. It will get resolved in semantic analysis pass 2.

ImportedName is not yet used everywhere where it could make
sense and this doesn't fix all related issues with import cycles.

Also did a bit of refactoring of type semantic analysis to avoid passing
multiple callback functions.

Fixes python#4049.
Fixes python#4429.
Fixes python#4682.

Inspired by (and borrowed test cases from) python#4495 by @carljm.

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