-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
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
There was a problem hiding this 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. |
There was a problem hiding this comment.
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".
There was a problem hiding this 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: |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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.