Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 29 additions & 14 deletions mypy/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)


Expand Down
31 changes: 30 additions & 1 deletion test-data/unit/check-incremental.test
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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