From 563ebb8a8d853af0e9c2b4edd110a876fff2e84d Mon Sep 17 00:00:00 2001 From: Michael Sullivan Date: Mon, 26 Mar 2018 19:55:43 -0700 Subject: [PATCH 1/2] 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 8135882d16c9..f24a0f06c287 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 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) 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' From ed4c5f94f14cdbfa87006a7269e281be6be0f5cc Mon Sep 17 00:00:00 2001 From: Michael Sullivan Date: Wed, 4 Apr 2018 13:27:56 -0700 Subject: [PATCH 2/2] Some cleanups --- mypy/build.py | 9 ++++++--- test-data/unit/check-incremental.test | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/mypy/build.py b/mypy/build.py index f24a0f06c287..096aabdb010e 100644 --- a/mypy/build.py +++ b/mypy/build.py @@ -1912,8 +1912,11 @@ def write_cache(self) -> None: self.mark_interface_stale() self.interface_hash = new_interface_hash - def verify_dependencies(self, suppressed_only: bool=False) -> None: - """Report errors for import targets in modules that don't exist.""" + 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 assert self.ancestors is not None if suppressed_only: @@ -1955,7 +1958,7 @@ 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. + # 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) diff --git a/test-data/unit/check-incremental.test b/test-data/unit/check-incremental.test index 1499deca7bd0..37c63bce7406 100644 --- a/test-data/unit/check-incremental.test +++ b/test-data/unit/check-incremental.test @@ -4207,7 +4207,7 @@ class Two: [out2] tmp/m/two.py:2: error: Revealed type is 'builtins.str' -[case testImportUnusedIgnore] +[case testImportUnusedIgnore1] # flags: --warn-unused-ignores import a [file a.py] @@ -4217,3 +4217,21 @@ import foo # type: ignore 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