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

Conversation

msullivan
Copy link
Collaborator

@msullivan msullivan commented Apr 2, 2018

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.

@msullivan msullivan requested review from JukkaL and gvanrossum April 2, 2018 22:29
@JukkaL JukkaL mentioned this pull request Apr 3, 2018
@msullivan msullivan changed the base branch from module-loading to master April 3, 2018 22:11
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
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doc nit.

mypy/build.py Outdated

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing word "flagging" after "spuriously".

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, but it would be good if @gvanrossum would also have a look at this since my context is lacking here.

mypy/build.py Outdated
@@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the suppressed_only argument.

Style nit: Add spaces around = in bool=False.

@@ -4206,3 +4206,14 @@ class Two:
pass
[out2]
tmp/m/two.py:2: error: Revealed type is 'builtins.str'

[case testImportUnusedIgnore]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to also add the second test case from #2960?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why the second case went missing, but:
It turns out that it fails because of another bug, which I have filed (#4856)

@msullivan msullivan merged commit 6784d85 into master Apr 4, 2018
@msullivan msullivan deleted the unused-import branch April 4, 2018 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants