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

* Use a separate TypeChecker per file.

* Change the TypeChecker toplevel interface.

* Do deferred (second) type checker pass after all first passes in an SCC.

* Add simple tests.

These verify that deferral due to a cross-module dependency works.

* Fix test failures due to semantic_analysis_only flag.

* Add TODOs about getting rid of BuildResult and all_types.

* Also hide cross-module notes with --hide-error-context

* Expand docstring for check_first_pass()

* Suppress reveal_type() output when current node is being deferred

* Add reveal_type() calls to testSymmetricImportCycle*

* WIP skip deferred queue duplicates

* Make it easy to do more than 2 passes

* Add a test that would require three passes

* Make lint happy

* Test case for error reported by pass two

* Test case for deferred decorator

* Explicit test case that reveal_type() shuts up in deferred function first time around

* Remove print calls

* Remove --hide-error-context from new tests and adjust expected output

* Add reversed version of testErrorInPassTwo

* Make decorator more complicated (both b.deco and a.g are now deferred)

Author: gvanrossum

mypy/build.py

@@ -33,9 +33,11 @@
 from mypy import moduleinfo
 from mypy import util
 from mypy.fixup import fixup_module_pass_one, fixup_module_pass_two
+from mypy.nodes import Expression
 from mypy.options import Options
 from mypy.parse import parse
 from mypy.stats import dump_type_stats
+from mypy.types import Type
 from mypy.version import __version__
 
 
@@ -49,6 +51,7 @@
 Graph = Dict[str, 'State']
 
 
+# TODO: Get rid of BuildResult.  We might as well return a BuildManager.
 class BuildResult:
     """The result of a successful build.
 
@@ -62,7 +65,7 @@ class BuildResult:
     def __init__(self, manager: 'BuildManager') -> None:
         self.manager = manager
         self.files = manager.modules
-        self.types = manager.type_checker.type_map
+        self.types = manager.all_types
         self.errors = manager.errors.messages()
 
 
@@ -184,7 +187,7 @@ def build(sources: List[BuildSource],
         manager.log("Build finished in %.3f seconds with %d modules, %d types, and %d errors" %
                     (time.time() - manager.start_time,
                      len(manager.modules),
-                     len(manager.type_checker.type_map),
+                     len(manager.all_types),
                      manager.errors.num_messages()))
         # Finish the HTML or XML reports even if CompileError was raised.
         reports.finish()
@@ -307,6 +310,8 @@ def default_lib_path(data_dir: str, pyversion: Tuple[int, int]) -> List[str]:
 PRI_ALL = 99  # include all priorities
 
 
+# TODO: Get rid of all_types.  It's not used except for one log message.
+#       Maybe we could instead publish a map from module ID to its type_map.
 class BuildManager:
     """This class holds shared state for building a mypy program.
 
@@ -322,7 +327,7 @@ class BuildManager:
                        Semantic analyzer, pass 2
       semantic_analyzer_pass3:
                        Semantic analyzer, pass 3
-      type_checker:    Type checker
+      all_types:       Map {Expression: Type} collected from all modules
       errors:          Used for reporting all errors
       options:         Build options
       missing_modules: Set of modules that could not be imported encountered so far
@@ -349,7 +354,7 @@ def __init__(self, data_dir: str,
         self.semantic_analyzer = SemanticAnalyzer(lib_path, self.errors)
         self.modules = self.semantic_analyzer.modules
         self.semantic_analyzer_pass3 = ThirdPass(self.modules, self.errors)
-        self.type_checker = TypeChecker(self.errors, self.modules)
+        self.all_types = {}  # type: Dict[Expression, Type]
         self.indirection_detector = TypeIndirectionVisitor()
         self.missing_modules = set()  # type: Set[str]
         self.stale_modules = set()  # type: Set[str]
@@ -461,9 +466,9 @@ def module_not_found(self, path: str, line: int, id: str) -> None:
                                'or using the "--silent-imports" flag would help)',
                                severity='note', only_once=True)
 
-    def report_file(self, file: MypyFile) -> None:
+    def report_file(self, file: MypyFile, type_map: Dict[Expression, Type]) -> None:
         if self.source_set.is_source(file):
-            self.reports.file(file, type_map=self.type_checker.type_map)
+            self.reports.file(file, type_map)
 
     def log(self, *message: str) -> None:
         if self.options.verbosity >= 1:
@@ -1407,23 +1412,42 @@ def semantic_analysis_pass_three(self) -> None:
             if self.options.dump_type_stats:
                 dump_type_stats(self.tree, self.xpath)
 
-    def type_check(self) -> None:
+    def type_check_first_pass(self) -> None:
         manager = self.manager
         if self.options.semantic_analysis_only:
             return
         with self.wrap_context():
-            manager.type_checker.visit_file(self.tree, self.xpath, self.options)
+            self.type_checker = TypeChecker(manager.errors, manager.modules, self.options,
+                                            self.tree, self.xpath)
+            self.type_checker.check_first_pass()
+
+    def type_check_second_pass(self) -> bool:
+        if self.options.semantic_analysis_only:
+            return False
+        with self.wrap_context():
+            return self.type_checker.check_second_pass()
+
+    def finish_passes(self) -> None:
+        manager = self.manager
+        if self.options.semantic_analysis_only:
+            return
+        with self.wrap_context():
+            manager.all_types.update(self.type_checker.type_map)
 
             if self.options.incremental:
-                self._patch_indirect_dependencies(manager.type_checker.module_refs)
+                self._patch_indirect_dependencies(self.type_checker.module_refs,
+                                                  self.type_checker.type_map)
 
             if self.options.dump_inference_stats:
                 dump_type_stats(self.tree, self.xpath, inferred=True,
-                                typemap=manager.type_checker.type_map)
-            manager.report_file(self.tree)
-
-    def _patch_indirect_dependencies(self, module_refs: Set[str]) -> None:
-        types = self.manager.type_checker.module_type_map.values()
+                                typemap=self.type_checker.type_map)
+            manager.report_file(self.tree, self.type_checker.type_map)
+
+    def _patch_indirect_dependencies(self,
+                                     module_refs: Set[str],
+                                     type_map: Dict[Expression, Type]) -> None:
+        types = set(type_map.values())
+        types.discard(None)
         valid = self.valid_references()
 
         encountered = self.manager.indirection_detector.find_modules(types) | module_refs
@@ -1726,7 +1750,15 @@ def process_stale_scc(graph: Graph, scc: List[str]) -> None:
     for id in scc:
         graph[id].semantic_analysis_pass_three()
     for id in scc:
-        graph[id].type_check()
+        graph[id].type_check_first_pass()
+    more = True
+    while more:
+        more = False
+        for id in scc:
+            if graph[id].type_check_second_pass():
+                more = True
+    for id in scc:
+        graph[id].finish_passes()
         graph[id].write_cache()
         graph[id].mark_as_rechecked()
 

mypy/checker.py

@@ -59,6 +59,8 @@
 
 T = TypeVar('T')
 
+LAST_PASS = 1  # Pass numbers start at 0
+
 
 # A node which is postponed to be type checked during the next pass.
 DeferredNode = NamedTuple(
@@ -73,6 +75,8 @@ class TypeChecker(NodeVisitor[Type]):
     """Mypy type checker.
 
     Type check mypy source files that have been semantically analyzed.
+
+    You must create a separate instance for each source file.
     """
 
     # Are we type checking a stub?
@@ -83,8 +87,6 @@ class TypeChecker(NodeVisitor[Type]):
     msg = None  # type: MessageBuilder
     # Types of type checked nodes
     type_map = None  # type: Dict[Expression, Type]
-    # Types of type checked nodes within this specific module
-    module_type_map = None  # type: Dict[Expression, Type]
 
     # Helper for managing conditional types
     binder = None  # type: ConditionalTypeBinder
@@ -121,56 +123,60 @@ class TypeChecker(NodeVisitor[Type]):
     # directly or indirectly.
     module_refs = None  # type: Set[str]
 
-    def __init__(self, errors: Errors, modules: Dict[str, MypyFile]) -> None:
+    def __init__(self, errors: Errors, modules: Dict[str, MypyFile], options: Options,
+                 tree: MypyFile, path: str) -> None:
         """Construct a type checker.
 
         Use errors to report type check errors.
         """
         self.errors = errors
         self.modules = modules
+        self.options = options
+        self.tree = tree
+        self.path = path
         self.msg = MessageBuilder(errors, modules)
-        self.type_map = {}
-        self.module_type_map = {}
-        self.binder = ConditionalTypeBinder()
         self.expr_checker = mypy.checkexpr.ExpressionChecker(self, self.msg)
+        self.binder = ConditionalTypeBinder()
+        self.globals = tree.names
         self.return_types = []
         self.type_context = []
         self.dynamic_funcs = []
         self.function_stack = []
         self.partial_types = []
         self.deferred_nodes = []
-        self.pass_num = 0
-        self.current_node_deferred = False
+        self.type_map = {}
         self.module_refs = set()
-
-    def visit_file(self, file_node: MypyFile, path: str, options: Options) -> None:
-        """Type check a mypy file with the given path."""
-        self.options = options
         self.pass_num = 0
-        self.is_stub = file_node.is_stub
-        self.errors.set_file(path)
-        self.globals = file_node.names
-        self.enter_partial_types()
-        self.is_typeshed_stub = self.errors.is_typeshed_file(path)
-        self.module_type_map = {}
-        self.module_refs = set()
-        if self.options.strict_optional_whitelist is None:
-            self.suppress_none_errors = not self.options.show_none_errors
+        self.current_node_deferred = False
+        self.is_stub = tree.is_stub
+        self.is_typeshed_stub = errors.is_typeshed_file(path)
+        if options.strict_optional_whitelist is None:
+            self.suppress_none_errors = not options.show_none_errors
         else:
             self.suppress_none_errors = not any(fnmatch.fnmatch(path, pattern)
                                                 for pattern
-                                                in self.options.strict_optional_whitelist)
+                                                in options.strict_optional_whitelist)
+
+    def check_first_pass(self) -> None:
+        """Type check the entire file, but defer functions with unresolved references.
+
+        Unresolved references are forward references to variables
+        whose types haven't been inferred yet.  They may occur later
+        in the same file or in a different file that's being processed
+        later (usually due to an import cycle).
+
+        Deferred functions will be processed by check_second_pass().
+        """
+        self.errors.set_file(self.path)
+        self.enter_partial_types()
 
         with self.binder.top_frame_context():
-            for d in file_node.defs:
+            for d in self.tree.defs:
                 self.accept(d)
 
         self.leave_partial_types()
 
-        if self.deferred_nodes:
-            self.check_second_pass()
-
-        self.current_node_deferred = False
+        assert not self.current_node_deferred
 
         all_ = self.globals.get('__all__')
         if all_ is not None and all_.type is not None:
@@ -181,21 +187,31 @@ def visit_file(self, file_node: MypyFile, path: str, options: Options) -> None:
                 self.fail(messages.ALL_MUST_BE_SEQ_STR.format(str_seq_s, all_s),
                           all_.node)
 
-        del self.options
+    def check_second_pass(self) -> bool:
+        """Run second or following pass of type checking.
 
-    def check_second_pass(self) -> None:
-        """Run second pass of type checking which goes through deferred nodes."""
-        self.pass_num = 1
-        for node, type_name in self.deferred_nodes:
+        This goes through deferred nodes, returning True if there were any.
+        """
+        if not self.deferred_nodes:
+            return False
+        self.errors.set_file(self.path)
+        self.pass_num += 1
+        todo = self.deferred_nodes
+        self.deferred_nodes = []
+        done = set()  # type: Set[FuncItem]
+        for node, type_name in todo:
+            if node in done:
+                continue
+            done.add(node)
             if type_name:
                 self.errors.push_type(type_name)
             self.accept(node)
             if type_name:
                 self.errors.pop_type()
-        self.deferred_nodes = []
+        return True
 
     def handle_cannot_determine_type(self, name: str, context: Context) -> None:
-        if self.pass_num == 0 and self.function_stack:
+        if self.pass_num < LAST_PASS and self.function_stack:
             # Don't report an error yet. Just defer.
             node = self.function_stack[-1]
             if self.errors.type_name:
@@ -2232,8 +2248,6 @@ def check_type_equivalency(self, t1: Type, t2: Type, node: Context,
     def store_type(self, node: Expression, typ: Type) -> None:
         """Store the type of a node in the type map."""
         self.type_map[node] = typ
-        if typ is not None:
-            self.module_type_map[node] = typ
 
     def in_checked_function(self) -> bool:
         """Should we type-check the current function?

mypy/checkexpr.py

@@ -1367,7 +1367,8 @@ def is_valid_cast(self, source_type: Type, target_type: Type) -> bool:
     def visit_reveal_type_expr(self, expr: RevealTypeExpr) -> Type:
         """Type check a reveal_type expression."""
         revealed_type = self.accept(expr.expr)
-        self.msg.reveal_type(revealed_type, expr)
+        if not self.chk.current_node_deferred:
+            self.msg.reveal_type(revealed_type, expr)
         return revealed_type
 
     def visit_type_application(self, tapp: TypeApplication) -> Type:

mypy/errors.py

@@ -290,7 +290,9 @@ def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int, int,
 
         for e in errors:
             # Report module import context, if different from previous message.
-            if e.import_ctx != prev_import_context:
+            if self.hide_error_context:
+                pass
+            elif e.import_ctx != prev_import_context:
                 last = len(e.import_ctx) - 1
                 i = last
                 while i >= 0:

test-data/unit/check-expressions.test

@@ -1605,6 +1605,14 @@ reveal_type("foo") # E: Argument 1 to "reveal_type" has incompatible type "str";
 reveal_type = 1
 1 + "foo" # E: Unsupported operand types for + ("int" and "str")
 
+[case testRevealForward]
+def f() -> None:
+    reveal_type(x)
+x = 1 + 1
+[out]
+main: note: In function "f":
+main:2: error: Revealed type is 'builtins.int'
+
 [case testEqNone]
 None == None
 [builtins fixtures/ops.pyi]