From 157e2700ac7e5c7ef63cf3d71a61732966b555a1 Mon Sep 17 00:00:00 2001 From: Michael Sullivan Date: Mon, 26 Mar 2018 19:55:43 -0700 Subject: [PATCH] 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. --- mypy/build.py | 38 ++++++++++++++++++--------- test-data/unit/check-incremental.test | 13 ++++++++- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/mypy/build.py b/mypy/build.py index e84e1b261a73..59a1ed495433 100644 --- a/mypy/build.py +++ b/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 module 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: if dep not in manager.modules and not manager.options.ignore_missing_imports: line = self.dep_line_map.get(dep, 1) try: @@ -1938,13 +1945,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) # Module import and diagnostic glue diff --git a/test-data/unit/check-incremental.test b/test-data/unit/check-incremental.test index e569e373710a..1499deca7bd0 100644 --- a/test-data/unit/check-incremental.test +++ b/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'