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

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

Merged
merged 2 commits into from Apr 4, 2018

Conversation

Projects
None yet
3 participants
@msullivan
Collaborator

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 Apr 2, 2018

@JukkaL JukkaL referenced this pull request Apr 3, 2018

Closed

Release 0.590 planning #4839

@msullivan msullivan changed the base branch from module-loading to master Apr 3, 2018

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
@gvanrossum

One doc nit.

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.

This comment has been minimized.

@gvanrossum

gvanrossum Apr 4, 2018

Member

missing word "flagging" after "spuriously".

@JukkaL

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

@@ -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:

This comment has been minimized.

@JukkaL

JukkaL Apr 4, 2018

Collaborator

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]

This comment has been minimized.

@JukkaL

JukkaL Apr 4, 2018

Collaborator

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

This comment has been minimized.

@msullivan

msullivan Apr 4, 2018

Collaborator

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@msullivan msullivan deleted the unused-import branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment