Don't produce spurious unused type ignore messages in incremental mode

This is accomplished by generating diagnostics for suppressed
dependencies before generating unused ignore notes.  This requires
that we store line information for suppressed dependencies in our
cache files.

Fixes #2960.

Author: msullivan

mypy/build.py

@@ -396,6 +396,8 @@ def default_lib_path(data_dir: str,
                         ('suppressed', List[str]),  # dependencies that weren't imported
                         ('child_modules', List[str]),  # all submodules of the given module
                         ('options', Optional[Dict[str, object]]),  # build options
+                        # dep_prios and dep_lines are in parallel with
+                        # dependencies + suppressed.
                         ('dep_prios', List[int]),
                         ('dep_lines', List[int]),
                         ('interface_hash', str),  # hash representing the public interface
@@ -964,8 +966,8 @@ def find_cache_meta(id: str, path: str, manager: BuildManager) -> Optional[Cache
     # Ignore cache if generated by an older mypy version.
     if ((m.version_id != manager.version_id and not manager.options.skip_version_check)
             or m.options is None
-            or len(m.dependencies) != len(m.dep_prios)
-            or len(m.dependencies) != len(m.dep_lines)):
+            or len(m.dependencies) + len(m.suppressed) != len(m.dep_prios)
+            or len(m.dependencies) + len(m.suppressed) != len(m.dep_lines)):
         manager.log('Metadata abandoned for {}: new attributes are missing'.format(id))
         return None
 
@@ -1514,12 +1516,13 @@ def __init__(self,
             # compare them to the originals later.
             self.dependencies = list(self.meta.dependencies)
             self.suppressed = list(self.meta.suppressed)
-            assert len(self.meta.dependencies) == len(self.meta.dep_prios)
+            all_deps = self.dependencies + self.suppressed
+            assert len(all_deps) == len(self.meta.dep_prios)
             self.priorities = {id: pri
-                               for id, pri in zip(self.meta.dependencies, self.meta.dep_prios)}
-            assert len(self.meta.dependencies) == len(self.meta.dep_lines)
+                               for id, pri in zip(all_deps, self.meta.dep_prios)}
+            assert len(all_deps) == len(self.meta.dep_lines)
             self.dep_line_map = {id: line
-                                 for id, line in zip(self.meta.dependencies, self.meta.dep_lines)}
+                                 for id, line in zip(all_deps, self.meta.dep_lines)}
             self.child_modules = set(self.meta.child_modules)
         else:
             # When doing a fine-grained cache load, pretend we only
@@ -1909,14 +1912,18 @@ def write_cache(self) -> None:
             self.mark_interface_stale()
             self.interface_hash = new_interface_hash
 
-    def verify_dependencies(self) -> None:
+    def verify_dependencies(self, suppressed_only: bool=False) -> None:
         """Report errors for import targets in modules that don't exist."""
-        # Strip out indirect dependencies. See comment in build.load_graph().
         manager = self.manager
-        dependencies = [dep for dep in self.dependencies
-                        if self.priorities.get(dep) != PRI_INDIRECT]
         assert self.ancestors is not None
-        for dep in dependencies + self.suppressed + self.ancestors:
+        if suppressed_only:
+            all_deps = self.suppressed
+        else:
+            # Strip out indirect dependencies. See comment in build.load_graph().
+            dependencies = [dep for dep in self.dependencies
+                            if self.priorities.get(dep) != PRI_INDIRECT]
+            all_deps = dependencies + self.suppressed + self.ancestors
+        for dep in all_deps:
             options = manager.options.clone_for_module(dep)
             if dep not in manager.modules and not options.ignore_missing_imports:
                 line = self.dep_line_map.get(dep, 1)
@@ -1939,13 +1946,18 @@ def verify_dependencies(self) -> None:
                     pass
 
     def dependency_priorities(self) -> List[int]:
-        return [self.priorities.get(dep, PRI_HIGH) for dep in self.dependencies]
+        return [self.priorities.get(dep, PRI_HIGH) for dep in self.dependencies + self.suppressed]
 
     def dependency_lines(self) -> List[int]:
-        return [self.dep_line_map.get(dep, 1) for dep in self.dependencies]
+        return [self.dep_line_map.get(dep, 1) for dep in self.dependencies + self.suppressed]
 
     def generate_unused_ignore_notes(self) -> None:
         if self.options.warn_unused_ignores:
+            # If this file was initially loaded from the cache, it may have suppressed
+            # dependencies due to imports with ignores on them. We need to generate
+            # those errors to avoid spuriously them as unused ignores.
+            if self.meta:
+                self.verify_dependencies(suppressed_only=True)
             self.manager.errors.generate_unused_ignore_notes(self.xpath)
 
 

test-data/unit/check-incremental.test

@@ -28,7 +28,7 @@
 -- annotation, and any files expect to be stale (aka have a modified interface)
 -- should be annotated in the [stale] annotation. Note that a file that ends up
 -- producing an error has its caches deleted and is marked stale automatically.
--- Such files don't need to be included in [stale ...] list.
+-- Such files do not need to be included in [stale ...] list.
 --
 -- The test suite will automatically assume that __main__ is stale and rechecked in
 -- all cases so we can avoid constantly having to annotate it. The list of
@@ -4206,3 +4206,14 @@ class Two:
     pass
 [out2]
 tmp/m/two.py:2: error: Revealed type is 'builtins.str'
+
+[case testImportUnusedIgnore]
+# flags: --warn-unused-ignores
+import a
+[file a.py]
+import b
+import foo  # type: ignore
+[file b.py]
+x = 1
+[file b.py.2]
+x = '2'