Don't produce spurious unused type ignore messages in incremental mode (#4841)

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,21 @@ def write_cache(self) -> None:
             self.mark_interface_stale()
             self.interface_hash = new_interface_hash
 
-    def verify_dependencies(self) -> None:
-        """Report errors for import targets in modules that don't exist."""
-        # Strip out indirect dependencies. See comment in build.load_graph().
+    def verify_dependencies(self, suppressed_only: bool = False) -> None:
+        """Report errors for import targets in modules that don't exist.
+
+        If suppressed_only is set, only check suppressed dependencies.
+        """
         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 +1949,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 flagging 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,32 @@ class Two:
     pass
 [out2]
 tmp/m/two.py:2: error: Revealed type is 'builtins.str'
+
+[case testImportUnusedIgnore1]
+# 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'
+
+-- TODO: Fails due to #4856
+[case testImportUnusedIgnore2-skip]
+# flags: --warn-unused-ignores
+import a
+[file a.py]
+import b
+import c  # type: ignore
+[file b.py]
+x = 1
+[file b.py.2]
+x = 'hi'
+[file c.py.3]
+pass
+[out]
+[out2]
+[out3]
+tmp/a.py:1: note: unused 'type: ignore' comment