Don't error on re-exported imports in cycles. Fixes #4049. #4495

carljm · 2018-01-20T06:23:40Z

carljm · 2018-01-20T06:31:06Z

So it turned out that just removing the blanket prohibition on re-exporting a node of kind UNBOUND_IMPORTED was all it took to make the example in #4049 (and a similar example I had independently discovered) work as expected. I had to reintroduce a specific check for a module importing from itself in order to make the testImportFromSameModule tests pass.

Without the changes in semanal.py, the added test here fails (as expected) with:

Expected:
  main:2: error: Revealed type is 'def () -> m.one.One' (diff)
Actual:
  tmp/m/two.py:1: error: Module 'm' has no attribute 'One' (diff)
  main:2: error: Revealed type is 'def () -> m.one.One' (diff)

Alignment of first line difference:
  E: main:2: error: Revealed type is 'def () -> m.one.One'
  A: tmp/m/two.py:1: error: Module 'm' has no attribute 'One'

carljm · 2018-01-20T06:43:29Z

Er, this isn't quite done yet; I hadn't asserted the resolved type at the right spot. On my other cyclic import example it works fully (all types resolve correctly), but on this example it leaves a type as Any where it shouldn't be; pushed a failing version of the test to demonstrate.

I think that bogus Any is a better failure mode than "Module has no attribute" false positives, so I still consider this an improvement over the status quo, but I'm looking into whether we can visit import nodes again in pass 3 to resolve these types correctly.

carljm · 2018-01-20T07:14:47Z

Pushed my initial attempt to get the type corrected. Not working yet, will look into it more later. Any tips welcome!

elliott-beach · 2018-01-21T04:35:53Z

I spent some time debugging this but could not figure it out. I'm eager to see what you come up with.

carljm · 2018-01-23T00:28:01Z

This passes all tests, including the added one, but in internal Instagram codebases it now generates new "{name} is not defined" errors on certain inline imports. Haven't found a simple-case repro for that yet, tracking it down.

carljm · 2018-01-23T06:38:45Z

Ok, this fixes the problem and causes no new errors (both in mypy test suite and in internal Instagram code). I think it's ready for an initial round of review.

carljm · 2018-02-01T01:34:16Z

Anything I could do by way of testing or anything else to make this more attractive for review? Not complaining, I know everyone's got plenty to do (been on the "overloaded maintainer" side plenty often), just wondering if I can help move it along somehow.

ilevkivskyi · 2018-02-03T16:16:32Z

This is a "controversial" topic, and some refactoring of symbol table kinds will happen soon. Therefore this may wait for some time, sorry.

carljm · 2018-02-03T22:55:12Z

@ilevkivskyi Thanks for the explanation. This fixes a clear bug, the new behavior is unequivocally better, it's been thoroughly tested on a large real-world code base, and the PR is not complex; it doesn't change any major behaviors, just does some fixups in third pass on things that were missed in the second pass. It doesn't seem likely to add significant complexity to any upcoming refactor of symbol table kinds. Is there any strong reason to let it languish rather than just merging it?

JukkaL · 2018-02-05T13:44:27Z

@ilevkivskyi Can you give specific reasons why this PR should be postponed? If you are worried about type alias refactoring, it looks to me like the required change to this PR should be pretty minor and is not enough to block this from being merged before the type alias refactor.

@carljm Fixing issues related to import cycles like #4049 is definitely high on our list of things we want to fix in the next few months or so.

ilevkivskyi

Not a real review, just some things I spotted when looking at this yesterday.

ilevkivskyi · 2018-02-05T14:50:04Z

mypy/semanal.py

@@ -1499,7 +1502,8 @@ def visit_import_from(self, imp: ImportFrom) -> None:
                    symbol = SymbolTableNode(GDEF, ast_node)
                    self.add_symbol(name, symbol, imp)
                    return
-            if node and node.kind != UNBOUND_IMPORTED and not node.module_hidden:
+
+            if node and not node.module_hidden and not (is_unbound and is_same_module):


This line is complex (many conditions), so it deserves a comment.

ilevkivskyi · 2018-02-05T14:53:34Z

mypy/semanal.py

            possible_module_id = import_id + '.' + id

            # If the module does not contain a symbol with the name 'id',
            # try checking if it's a module instead.
-            if not node or node.kind == UNBOUND_IMPORTED:
+            if not node or is_unbound:


I find the original form simpler (don't need look above to is_unbound).

ilevkivskyi · 2018-02-05T14:54:13Z

test-data/unit/check-modules.test

+reveal_type(One)
+class Two:
+    pass
+[out]


Add also tests for incremental mode.

Ok, I can look at some existing incremental-mode tests to see how that's done.

ilevkivskyi · 2018-02-05T14:55:14Z

test-data/unit/check-modules.test

@@ -1943,3 +1943,24 @@ main:2: error: Name 'name' is not defined
 main:3: error: Revealed type is 'Any'

 [builtins fixtures/module.pyi]
+
+
+[case testImportCycle]


I would prefer a more descriptive name.

ilevkivskyi · 2018-02-05T14:56:09Z

test-data/unit/check-modules.test

+reveal_type(One)
+class Two:
+    pass
+[out]


Please also add tests for type aliases (you seem to care about them in visit_import_from).

So the type-alias stuff is there just because it seemed better to be complete about transferring node info (in the same way that visit_import_from in second pass is complete), rather than only transfer some of the node attributes. I have not had any real-world issue with type aliases and circular imports that those lines are intended to solve. I will try to see if I can repro some case involving type aliases that is fixed by those lines.

JukkaL

As discussed the comments, this only fixes the import cycle issue in some contexts. This might still be a reasonable incremental improvement, but note that we are planning a more general fix in the near/medium term.

We have at least two options:

Polish this PR a bit and merge, and create a new issue about the remaining problems.
Postpone this and wait for the more general fix.

What do you think?

JukkaL · 2018-02-05T13:56:47Z

mypy/semanal_pass3.py

+    def visit_name_expr(self, expr: NameExpr) -> None:
+        # Fixup remaining UNBOUND_IMPORTED nodes from import cycles
+        if expr.kind == UNBOUND_IMPORTED:
+            n = self.sem.lookup(expr.name, expr, suppress_errors=True)


Calling lookup() in pass 3 generally doesn't do the right thing, since not all the namespace context is set up correctly. It works for module-level attributes, though. Add a TODO comment about this since there currently really isn't a better way to do this.

JukkaL · 2018-02-05T13:57:22Z

test-data/unit/check-modules.test

@@ -1943,3 +1943,24 @@ main:2: error: Name 'name' is not defined
 main:3: error: Revealed type is 'Any'

 [builtins fixtures/module.pyi]
+


Style nit: we use one empty line between test cases.

JukkaL · 2018-02-05T14:03:46Z

test-data/unit/check-modules.test

+
+[case testImportCycle]
+from m import One
+reveal_type(One)


It would make sense to also try doing other things with One, just to make sure that import is resolved correctly. For example, use it as a base class and in a type annotation.

JukkaL · 2018-02-05T14:06:23Z

test-data/unit/check-modules.test

+[file m/__init__.py]
+from .one import One
+from .two import Two
+reveal_type(One)


Again, it would be nice to test how all these references work with type aliases and named tuples, for example. (In separate tese cases.)

JukkaL · 2018-02-05T16:16:30Z

test-data/unit/check-modules.test

+    pass
+[file m/two.py]
+from m import One
+reveal_type(One)


Note that references to One in annotations still don't work correctly:

from m import One o: One reveal_type(o) # Any

Similarly, if we make One a type alias and use it in an annotation, it will be treated as Any.

If we create a subclass of One in m.two, it seems to have an implicit Any base class, which is unexpected.

Good catches! I think these are probably fixable by adding more third-pass visits to different node types to propagate type info that we missed in second pass due to the cycle, but it does begin to feel a bit like a game of whack-a-mole.

carljm · 2018-02-05T21:09:30Z

Thanks for the review comments! I see that this fix is going to get uglier as I make it more complete. I'm curious what the longer-term plan is to address import cycles in a more thorough way (I guess some kind of fixed-point iteration in semantic analysis might be needed?)

From a practical point of view doing gradual typing in a large codebase, spurious Any is a preferable failure mode to false positive import errors, so I'd have some preference for improving this and merging it (and I'm willing to work on getting it merge ready). It seems like at the very least the tests will be a useful addition regardless of future refactoring. But if there are specific ways that merging this will hinder progress towards a better fix, I'm open to dropping it.

ilevkivskyi · 2018-02-05T23:23:52Z

But if there are specific ways that merging this will hinder progress towards a better fix, I'm open to dropping it.

We discussed with @JukkaL and it looks like this PR is unlikely to cause any long-term harm, and it would be OK as a temporary partial solution (provided you fix some review comments).

JukkaL · 2018-02-06T11:34:00Z

I'm curious what the longer-term plan is to address import cycles in a more thorough way (I guess some kind of fixed-point iteration in semantic analysis might be needed?)

Here are a few ideas:

Create "indirect cross-module references" (perhaps a new AST node class?) during the first pass of semantic analysis. These would be dereferenced on demand during later phases of semantic analysis. We would support multiple levels of indirection. For example, assume we have from x import y in module m. This would make m.y an indirect reference to x.y. x.y could be another indirect reference to, say, a.b. When analyzing a y reference in m, we'd follow the indirect reference chain until we reach a.b, which is the actual target.
Create cross-module ForwardRef objects if there is a reference to a type in another module that hasn't been analyzed yet. We already support forward references within a module, and hopefully it wouldn't be hard to generalize these a bit.

* master: (32 commits) Fix some fine-grained cache/fswatcher problems (python#4560) Sync typeshed (python#4559) Add _cached suffix to test cases in fine-grained tests with cache (python#4558) Add back support for simplified fine-grained logging (python#4557) Type checking of class decorators (python#4544) Sync typeshed (python#4556) When loading from a fine-grained cache, use the real path, not the cached (python#4555) Switch all of the fine-grained debug logging to use manager.log (python#4550) Caching for fine-grained incremental mode (python#4483) Fix --warn-return-any for NotImplemented (python#4545) Remove myunit (python#4369) Store line numbers of imports in the cache metadata (python#4533) README.md: Fix a typo (python#4529) Enable generation and caching of fine-grained dependencies from normal runs (python#4526) Move argument parsing for the fine-grained flag into the main arg parsing code (python#4524) Don't warn about unrecognized options starting with 'x_' (python#4522) stubgen: don't append star arg when args list already has varargs appended (python#4518) Handle TypedDict in diff and deps (python#4510) Fix Options.__repr__ to not infinite recurse (python#4514) Fix some fine-grained incremental bugs with newly imported files (python#4502) ...

* master: (27 commits) Don't call --strict-optional and --incremental experimental (python#4642) Sync typeshed (python#4641) Fix callable types with inconsistent argument counts (python#4611) Fix example (add 'class A:') Make psutil an optional dependency (python#4634) mypy and mypy_extensions aren't posix only (python#3765) Documentation for attr support (python#4632) Use read_with_python_encoding in stubgen to handle file encoding (python#3790) Sync typeshed (python#4631) Add remaining core team emails to CREDITS (python#4629) Fix issues with attr code. (python#4628) Better support for converter in attrs plugin. (python#4607) Clean up credits (python#4626) Support type aliases in fine-grained incremental mode (python#4525) Fine-grained: Fix crash caused by unreachable class (python#4613) Treat divmod like a binary operator (python#4585) Sync typeshed (python#4605) Fine-grained: Don't infer partial types from multiple targets (python#4553) Fine-grained: Compare symbol table snapshots when following dependencies (python#4598) Fix type of forward reference to a decorated class method (python#4486) ...

* master: New files shouldn't trigger a coarse-grained rebuild in fg cache mode (python#4669) Bump version to 0.580-dev Update revision history for 0.570 (python#4662) Fine-grained: Fix crashes when refreshing synthetic types (python#4667) Fine-grained: Support NewType and reset subtype caches (python#4656) Fine-grained: Detect changes in additional TypeInfo attributes (python#4659) Fine-grained: Apply semantic analyzer patch callbacks (python#4658) Optimize fine-grained update by using Graph as the cache (python#4622) Cleanup check_reverse_op_method (python#4017) Fine-grained: Fix AST merge issues (python#4652) Optionally check that we don't have duplicate nodes after AST merge (python#4647)

carljm · 2018-03-06T10:06:52Z

Ok, I think I've addressed all the feedback so far. I fixed several new cases (member expressions, use in type annotations) and added tests for type aliases, named tuples, and incremental mode. I left TODOs for a) inheriting from a re-exported-in-cycle import (it seemed too complex to fix up in third pass) and b) name expressions in scopes other than module-level variables. I certainly won't claim that this is a complete fix, but I think it does improve behavior incrementally for many common cases, and at the very least the tests and TODOs will be a useful starting point for a more complete fix. Review and further comments much appreciated.

I'll be very curious to see what form the more complete fix takes, having wrestled with this for awhile. I'm probably missing something key, but it feels to me like the mentioned approaches (cross-module references, using ForwardRef) are effectively just different ways to handle the book-keeping for unresolved cycles; they don't address the root problem with this diff: that we have to go back and fix up all kinds of things in third pass that normally should be handled in second pass, by effectively copying bits of code in from second pass. That doesn't feel like a path to "complete fix", it feels like whack-a-mole, where the asymptote is that third pass becomes a more and more "complete" copy of second pass. It seems like for a thorough and sustainable fix of these kinds of issues, some kind of scheme for fixed-point iteration in second pass will be needed instead. But I'm sure I'm missing something, and I look forward to learning a lot from seeing the better fix implemented :-)

Thanks @JukkaL and @ilevkivskyi for reviewing!

JukkaL · 2018-03-06T10:32:15Z

@carljm Thanks for the updates! I'm currently prototyping what might become a cleaner fix, where we handle many exported imported names in semantic analyzer pass 2. If it works out, it might be at least a partial replacement of this PR, but your PR has been useful first step towards fixing the underlying issue. I'm going to try your test cases with my work-in-progress fix. I still don't have complete story for references to named tuples, type aliases and such with my approach.

JukkaL · 2018-03-07T16:18:18Z

@carljm #4695 is an alternative fix to the same issue, which I believe to be somewhat more general. It handles base classes, at least, and it may be easier to generalize to fix certain other import cycle related issues.

@carljm

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. Inspired by (and borrowed test cases from) #4495 by @carljm. Supersedes #4495

emmatyping · 2018-03-08T11:41:03Z

This is superseded by 0b7597c, so closing.

ilevkivskyi · 2018-03-08T11:43:29Z

Interesting that it didn't happen automatically. Looking at the previous type when it worked, it looks like it auto-closes superseded PRs only on comments like Fixes #PR (which is weird).

emmatyping · 2018-03-08T11:44:45Z

Yeah, I think github's keywords are a bit specific here. (I think closes may work, but not sure)

sersorrel · 2018-03-08T11:46:30Z

close, fix, and resolve, as well as variants like closes, all work: https://help.github.com/articles/closing-issues-using-keywords/

@carljm

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

carljm added 2 commits January 19, 2018 22:15

Don't error on re-exported imports in cycles. Fixes python#4049.

1dc60ec

Move test to check-modules and assert on type of imported obj.

02c401b

carljm added 2 commits January 19, 2018 22:34

Small simplification of the test.

5edf2af

Add failing assertion.

9903110

First attempt to correct type of cyclic import; not working.

9f36fec

Also update UNBOUND_IMPORTED Name expressions.

3cc7e2e

Suppress errors in followup lookup.

2255e1c

ilevkivskyi reviewed Feb 5, 2018

View reviewed changes

JukkaL reviewed Feb 5, 2018

View reviewed changes

carljm added 8 commits February 9, 2018 13:49

Also fix up member expressions.

f78ae95

Clean up lint and typecheck.

b5bf41d

Adjust for addition of cur_mod_node attribute.

06dbcc6

Add support for type annotation.

2efc322

Typecheck fixes and TODO comments.

f7c3d43

carljm added 2 commits March 6, 2018 02:42

Add tests for namedtuple and type alias.

5adf42b

Add test for incremental mode.

f675706

JukkaL mentioned this pull request Mar 7, 2018

Fix issues related to indirect imports in import cycles #4695

Merged

emmatyping closed this Mar 8, 2018

		@@ -1943,3 +1943,24 @@ main:2: error: Name 'name' is not defined
		main:3: error: Revealed type is 'Any'

		[builtins fixtures/module.pyi]

Don't error on re-exported imports in cycles. Fixes #4049. #4495

Don't error on re-exported imports in cycles. Fixes #4049. #4495

Conversation

carljm commented Jan 20, 2018 • edited Loading

carljm commented Jan 20, 2018

carljm commented Jan 20, 2018

carljm commented Jan 20, 2018

elliott-beach commented Jan 21, 2018 • edited Loading

carljm commented Jan 23, 2018

carljm commented Jan 23, 2018

carljm commented Feb 1, 2018

ilevkivskyi commented Feb 3, 2018

carljm commented Feb 3, 2018

JukkaL commented Feb 5, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Feb 5, 2018

ilevkivskyi commented Feb 5, 2018

JukkaL commented Feb 6, 2018

carljm commented Mar 6, 2018

JukkaL commented Mar 6, 2018

JukkaL commented Mar 7, 2018

emmatyping commented Mar 8, 2018

ilevkivskyi commented Mar 8, 2018

emmatyping commented Mar 8, 2018 • edited Loading

sersorrel commented Mar 8, 2018

carljm commented Jan 20, 2018 •

edited

Loading

elliott-beach commented Jan 21, 2018 •

edited

Loading

emmatyping commented Mar 8, 2018 •

edited

Loading