Fix the missing_modules related failures.

This has been a bit of an adventure.

Since we are no longer doing a full `load_graph()`, we can't rely on
it fully populating `missing_modules` if it is cleared between
updates. If `missing_modules` isn't fully populated, then semanal will
spuriously reject `from x import y` where `x.y` is a missing module.

We can't just *not* clear `missing_modules`, however, because
`parse_file` uses it to decide that modules are suppressed. But this
is actually wrong! It can lead to an import failure message not being
generated because some other module has already failed to import it
(and perhaps even ignored the error).

`parse_file()` shouldn't actually *need* to compute `suppressed`,
though. If it is called on a file with no cache, `load_graph()` will
handle moving deps to `suppressed`, and if called on a file that has
had cache info loaded, then the dependency information has all been
loaded from the cache.

So we refactor things so that dep information from the cache is used
when available and `parse_file` doesn't need to deal with it.

I strongly suspect that this refactor obviates the need for
`fix_suppressed_dependencies`, but I was not able to quickly produce a
test case from the description in #2036, so I am not comfortable
making that change.

Author: msullivan

mypy/build.py

@@ -1660,7 +1660,7 @@ def __init__(self,
         else:
             # Parse the file (and then some) to get the dependencies.
             self.parse_file()
-            self.suppressed = []
+            self.compute_dependencies()
             self.child_modules = set()
 
     def skipping_ancestor(self, id: str, path: str, ancestor_for: 'State') -> None:
@@ -1815,6 +1815,8 @@ def fix_suppressed_dependencies(self, graph: Graph) -> None:
         """
         # TODO: See if it's possible to move this check directly into parse_file in some way.
         # TODO: Find a way to write a test case for this fix.
+        # TODO: I suspect that splitting compute_dependencies() out from parse_file
+        # obviates the need for this but lacking a test case for the problem this fixed...
         silent_mode = (self.options.ignore_missing_imports or
                        self.options.follow_imports == 'skip')
         if not silent_mode:
@@ -1881,49 +1883,53 @@ def parse_file(self) -> None:
         # TODO: Why can't SemanticAnalyzerPass1 .analyze() do this?
         self.tree.names = manager.semantic_analyzer.globals
 
+        for pri, id, line in manager.all_imported_modules_in_file(self.tree):
+            if id == '':
+                # Must be from a relative import.
+                manager.errors.set_file(self.xpath, self.id)
+                manager.errors.report(line, 0,
+                                      "No parent module -- cannot perform relative import",
+                                      blocker=True)
+
+        self.check_blockers()
+
+    def compute_dependencies(self):
+        """Compute a module's dependencies after parsing it.
+
+        This is used when we parse a file that we didn't have
+        up-to-date cache information for. When we have an up-to-date
+        cache, we just use the cached info.
+        """
+        manager = self.manager
+
         # Compute (direct) dependencies.
         # Add all direct imports (this is why we needed the first pass).
         # Also keep track of each dependency's source line.
         dependencies = []
-        suppressed = []
         priorities = {}  # type: Dict[str, int]  # id -> priority
         dep_line_map = {}  # type: Dict[str, int]  # id -> line
         for pri, id, line in manager.all_imported_modules_in_file(self.tree):
             priorities[id] = min(pri, priorities.get(id, PRI_ALL))
             if id == self.id:
                 continue
-            # Omit missing modules, as otherwise we could not type-check
-            # programs with missing modules.
-            if id in manager.missing_modules:
-                if id not in dep_line_map:
-                    suppressed.append(id)
-                    dep_line_map[id] = line
-                continue
-            if id == '':
-                # Must be from a relative import.
-                manager.errors.set_file(self.xpath, self.id)
-                manager.errors.report(line, 0,
-                                      "No parent module -- cannot perform relative import",
-                                      blocker=True)
-                continue
             if id not in dep_line_map:
                 dependencies.append(id)
                 dep_line_map[id] = line
         # Every module implicitly depends on builtins.
         if self.id != 'builtins' and 'builtins' not in dep_line_map:
             dependencies.append('builtins')
 
-        # If self.dependencies is already set, it was read from the
-        # cache, but for some reason we're re-parsing the file.
         # NOTE: What to do about race conditions (like editing the
         # file while mypy runs)?  A previous version of this code
         # explicitly checked for this, but ran afoul of other reasons
         # for differences (e.g. silent mode).
+
+        # Missing dependencies will be moved from dependencies to
+        # suppressed when they fail to be loaded in load_graph.
         self.dependencies = dependencies
-        self.suppressed = suppressed
+        self.suppressed = []
         self.priorities = priorities
         self.dep_line_map = dep_line_map
-        self.check_blockers()
 
     def semantic_analysis(self) -> None:
         assert self.tree is not None, "Internal error: method must be called on parsed file only"

mypy/server/update.py

@@ -122,7 +122,7 @@
 
 from mypy.build import (
     BuildManager, State, BuildSource, Graph, load_graph, find_module_clear_caches,
-    DEBUG_FINE_GRAINED,
+    PRI_INDIRECT, DEBUG_FINE_GRAINED,
 )
 from mypy.checker import DeferredNode
 from mypy.errors import Errors, CompileError
@@ -347,7 +347,9 @@ def update_single_isolated(module: str,
     old_modules = dict(manager.modules)
     sources = get_sources(previous_modules, [(module, path)])
 
-    manager.missing_modules.clear()
+    if module in manager.missing_modules:
+        manager.missing_modules.remove(module)
+
     try:
         if module in graph:
             del graph[module]
@@ -524,7 +526,9 @@ def get_all_changed_modules(root_module: str,
 
 def verify_dependencies(state: State, manager: BuildManager) -> None:
     """Report errors for import targets in module that don't exist."""
-    for dep in state.dependencies + state.suppressed:  # TODO: ancestors?
+    # Strip out indirect dependencies. See comment in build.load_graph().
+    dependencies = [dep for dep in state.dependencies if state.priorities.get(dep) != PRI_INDIRECT]
+    for dep in dependencies + state.suppressed:  # TODO: ancestors?
         if dep not in manager.modules and not manager.options.ignore_missing_imports:
             assert state.tree
             line = find_import_line(state.tree, dep) or 1

test-data/unit/check-dmypy-fine-grained.test

@@ -32,7 +32,7 @@ def f() -> str:
 [out2]
 tmp/b.py:3: error: Incompatible types in assignment (expression has type "int", variable has type "str")
 
-[case testAddFileFineGrainedIncremental]
+[case testAddFileFineGrainedIncremental1]
 # cmd: mypy -m a
 # cmd2: mypy -m a b
 [file a.py]
@@ -46,6 +46,19 @@ tmp/a.py:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-impor
 [out2]
 tmp/a.py:2: error: Argument 1 to "f" has incompatible type "int"; expected "str"
 
+[case testAddFileFineGrainedIncremental2]
+# flags: --follow-imports=skip --ignore-missing-imports
+# cmd: mypy -m a
+# cmd2: mypy -m a b
+[file a.py]
+import b
+b.f(1)
+[file b.py.2]
+def f(x: str) -> None: pass
+[out1]
+[out2]
+tmp/a.py:2: error: Argument 1 to "f" has incompatible type "int"; expected "str"
+
 [case testDeleteFileFineGrainedIncremental]
 # cmd: mypy -m a b
 # cmd2: mypy -m a

test-data/unit/check-incremental.test

@@ -4059,3 +4059,71 @@ class Baz:
 [out]
 [out2]
 tmp/a.py:3: error: Unsupported operand types for + ("int" and "str")
+
+[case testIncrementalWithIgnoresTwice]
+import a
+[file a.py]
+import b
+import foo # type: ignore
+[file b.py]
+x = 1
+[file b.py.2]
+x = 'hi'
+[file b.py.3]
+x = 1
+[builtins fixtures/module.pyi]
+[out]
+[out2]
+[out3]
+
+[case testIgnoredImport2]
+import x
+[file y.py]
+import xyz  # type: ignore
+B = 0
+from x import A
+[file x.py]
+A = 0
+from y import B
+[file x.py.2]
+A = 1
+from y import B
+[file x.py.3]
+A = 2
+from y import B
+[out]
+[out2]
+[out3]
+
+[case testDeletionOfSubmoduleTriggersImportFrom2]
+from p.q import f
+f()
+[file p/__init__.py]
+[file p/q.py]
+def f() -> None: pass
+[delete p/q.py.2]
+[file p/q.py.3]
+def f(x: int) -> None: pass
+[out]
+[out2]
+main:1: error: Cannot find module named 'p.q'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)
+[out3]
+main:2: error: Too few arguments for "f"
+
+[case testDeleteIndirectDependency]
+import b
+b.x.foo()
+[file b.py]
+import c
+x = c.Foo()
+[file c.py]
+class Foo:
+    def foo(self) -> None: pass
+[delete c.py.2]
+[file b.py.2]
+class Foo:
+    def foo(self) -> None: pass
+x = Foo()
+[out]
+[out2]

test-data/unit/check-modules.test

@@ -1943,3 +1943,15 @@ main:2: error: Name 'name' is not defined
 main:3: error: Revealed type is 'Any'
 
 [builtins fixtures/module.pyi]
+
+[case testFailedImportFromTwoModules]
+import c
+import b
+[file b.py]
+import c
+
+[out]
+-- TODO: it would be better for this to be in the other order
+tmp/b.py:1: error: Cannot find module named 'c'
+main:1: error: Cannot find module named 'c'
+main:1: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)