Repository navigation
Fix issues related to indirect imports in import cycles - #4695
Conversation
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.
This makes it easier to add additional functions to the interface.
…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
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! I have few questions/suggestions.
|
|
||
| def normalize_type_alias(self, node: SymbolTableNode, | ||
| ctx: Context) -> Optional[SymbolTableNode]: | ||
| """If node refers to a built-intype alias, normalize it. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll fix them in a separate PR.
OK, having smaller PRs makes sense.
| self.fail(msg, ctx, blocker=True) | ||
|
|
||
| def note(self, msg: str, ctx: Context) -> None: | ||
| self.sem.note(msg, ctx) |
There was a problem hiding this comment.
What is the reason we can reuse self.sem.note here but not self.sem.fail above?
There was a problem hiding this comment.
These were different originally, but it seems like there's no good reason. I'll unify them.
| # 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' |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'll add a few test cases where we reverse the shape of the import cycle relative to the main module.
|
(I updated the description to mention that this also fixes #4682 since I found the relevant code and tests.) |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
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 :-))
|
Confirmed that this fixes the problems in our codebase. Thanks @JukkaL and @ilevkivskyi ! |
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
3 participants
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
ImportedNamein semanticanalysis pass 1 to represent an indirect reference to a name in another
module. It will get resolved in semantic analysis pass 2.
ImportedNameis not yet used everywhere where it could makesense 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