Check deferred nodes after all first passes in an import cycle #2264

Merged
merged 21 commits into from Oct 24, 2016

Projects

None yet

3 participants

@gvanrossum
Member

Might fix #481.

+[file b.py]
+import a
+def g() -> int:
+ return a.y
@JukkaL
JukkaL Oct 19, 2016 Collaborator

Reveal the type of a.y (to verify that it doesn't somehow fall back to Any).

test-data/unit/check-modules.test
+[file a.py]
+import b
+def f() -> int:
+ return b.x
@JukkaL
JukkaL Oct 19, 2016 Collaborator

Reveal the type of b.x.

mypy/checker.py
+ in options.strict_optional_whitelist)
+
+ def check_first_pass(self) -> None:
+ """Type check the entire file, but defer functions with forward references."""
@JukkaL
JukkaL Oct 19, 2016 Collaborator

Be more explicit about forward reference mean. In particular, they mean references to definitions whose types haven't been inferred yet.

@JukkaL
Collaborator
JukkaL commented Oct 19, 2016

Looks good to me! This hopefully fixes one of the biggest pain points mypy users with large code bases have had.

I'd like to see some more test cases before merging:

  1. Add test case where two passes aren't enough, to verify that we still
    generate cannot determine type errors when needed.
  2. Add test case for referring to a decorated function whose type might not be inferred
    (another common case where we had trouble before).
  3. Add test case where we generate an error only on the second pass (to verify
    that the import context is shown correctly). For example, something like this (partial example):
def f() -> None:
    a = b.x  # Type of b.x only available during second pass
    a + 1   # Error that we can't report during first pass
@gvanrossum
Member

I'll add more tests. I also discovered that reveal_type() needs to check for current_node_deferred.

@gvanrossum
Member

Rebased, generalized the multi-pass machinery, and added a test that would require 3 passes.

Still needs tests for (2) and (3). Also, there are still some print() calls left.

@gvanrossum
Member

I could use a hint for (2).

@gvanrossum
Member

I think I have all the required tests now. Jukka, can you double-check?

@codecov-io
codecov-io commented Oct 23, 2016 edited

Current coverage is 84.90% (diff: 100%)

Merging #2264 into master will increase coverage by 1.80%

@@             master      #2264   diff @@
==========================================
  Files            72         72           
  Lines         19037      20953   +1916   
  Methods           0          0           
  Messages          0          0           
  Branches       3919       4824    +905   
==========================================
+ Hits          15819      17790   +1971   
+ Misses         2618       2588     -30   
+ Partials        600        575     -25   

Powered by Codecov. Last update 15adf8b...f1b8872

mypy/checker.py
+ return False
+ self.errors.set_file(self.path)
+ self.pass_num += 1
+ print('---', self.path, 'pass', self.pass_num + 1, '---')
@JukkaL
JukkaL Oct 24, 2016 Collaborator

Remove print.

mypy/checker.py
+ for node, type_name in todo:
+ if node in done:
+ continue
+ print(type_name, '.', node.fullname() or node.name())
@JukkaL
JukkaL Oct 24, 2016 Collaborator

Remove another print.

test-data/unit/check-modules.test
+tmp/a.py:4: error: Cannot determine type of 'x2'
+
+[case testErrorInPassTwo]
+# flags: --hide-error-context
@JukkaL
JukkaL Oct 24, 2016 Collaborator

Don't hide error context to test the it works correctly for errors reported in the second pass.

test-data/unit/check-modules.test
+import a
+x = 1 + 1
+[out]
+tmp/a.py:4: error: Unsupported operand types for + ("int" and "str")
@JukkaL
JukkaL Oct 24, 2016 edited Collaborator

Maybe also test this the other way as well (error reported in b.py in addition a.py).

test-data/unit/check-modules.test
+[file b.py]
+from typing import Any
+import a
+def deco(f: Any) -> Any:
@JukkaL
JukkaL Oct 24, 2016 Collaborator

This decorator probably doesn't trigger a second pass, since simple decorator signatures are special cased in semantic analysis. A more complex decorator signature such as Callable[[T], int] -> Callable[[T]], str] is needed.

test-data/unit/check-modules.test
+[file a.py]
+import b
+def g() -> None:
+ f()
@JukkaL
JukkaL Oct 24, 2016 Collaborator

Make sure that the decorator function returns an interesting type (not Any) and do reveal_type(f()) here to verify that decorator got applied correctly.

gvanrossum added some commits Oct 15, 2016
@gvanrossum gvanrossum Use a separate TypeChecker per file. cd22b73
@gvanrossum gvanrossum Change the TypeChecker toplevel interface. 4bd65c0
@gvanrossum gvanrossum Do deferred (second) type checker pass after all first passes in an SCC. 9b46001
@gvanrossum gvanrossum Add simple tests.
These verify that deferral due to a cross-module dependency works.
d4d26c6
@gvanrossum gvanrossum Fix test failures due to semantic_analysis_only flag. 4afdc13
@gvanrossum gvanrossum Add TODOs about getting rid of BuildResult and all_types. be3dfa1
@gvanrossum gvanrossum Also hide cross-module notes with --hide-error-context de3a076
@gvanrossum gvanrossum Expand docstring for check_first_pass() ed10d6d
@gvanrossum gvanrossum Suppress reveal_type() output when current node is being deferred 30c84fa
@gvanrossum gvanrossum Add reveal_type() calls to testSymmetricImportCycle* f68a7d4
@gvanrossum gvanrossum WIP skip deferred queue duplicates fd5bc22
@gvanrossum gvanrossum Make it easy to do more than 2 passes 53f635b
@gvanrossum gvanrossum Add a test that would require three passes 182059b
@gvanrossum gvanrossum Make lint happy 981c36e
@gvanrossum gvanrossum Test case for error reported by pass two bb5361f
@gvanrossum gvanrossum Test case for deferred decorator 8abacf1
@gvanrossum gvanrossum Explicit test case that reveal_type() shuts up in deferred function f…
…irst time around
6aea45a
@gvanrossum gvanrossum Remove print calls 19345a0
@gvanrossum gvanrossum Remove --hide-error-context from new tests and adjust expected output bb212eb
@gvanrossum gvanrossum Add reversed version of testErrorInPassTwo 331f845
@gvanrossum gvanrossum Make decorator more complicated (both b.deco and a.g are now deferred)
d1778da
@gvanrossum
Member

Ready for review/merge.

@JukkaL
Collaborator
JukkaL commented Oct 24, 2016

LGTM now -- thanks for the updates!

@JukkaL JukkaL merged commit 68c6e96 into master Oct 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@gvanrossum gvanrossum deleted the checker-per-file branch Oct 24, 2016
@gvanrossum
Member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment