Fixes ''unused-argument`` false-positives with overshadowed variable in dictionary comprehension #1757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ume, consumed and scope_type informatios. Use of this class inside some visit/leave methods but more interestingly inside visit_name method, trying to make it a little bit clearer. Add of a method named _check_name_identical_to_args in order to deal with a name used inside function args and inside a comprehension. Deletion of _next_to_consume static method replaced by NamesConsumer.get_next_to_consume.
…NamesConsumer.get_next_to_consume signature
…r_function_scope. Adding docstring inside _has_homonym_in_upper_function_scope. Restricting the match in _has_homonym_in_upper_function_scope to function scope.
PCManticore
added
the
Needs review 🔍
PCManticore
approved these changes
Jan 3, 2018
There was a problem hiding this comment.
@hippo91 this looks super straightforward! Thank you for tackling this issue. Left just a naming comment, please rebase it and let's get this merged.
pylint/checkers/variables.py
Outdated
| @@ -336,6 +337,64 @@ def _assigned_locally(name_node): | |||
|
|
|||
| } | |||
|
|
|||
|
|
|||
| NamesConsumerAtomic = collections.namedtuple("NamesConsumerAtomic", | |||
There was a problem hiding this comment.
Maybe we can rename it to ScopeConsumer since it is used mostly to hold names belonging to a scope?
PCManticore
added
reviewed-waiting-updates
and removed
Needs review 🔍
|
@PCManticore i merged it onto master. Should i backport it to 1.8? |
|
Sure :) |
|
@PCManticore do all bug corrections be backported to 1.8? |
hippo91
added a commit
to hippo91/pylint
that referenced
this pull request
Jan 4, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an answer to #1731.
The
unused-argumentmessage is emitted from theleave_functiondefmethod ofVariablesCheckerclass.In the example given by @bokhi the wrong emission is due to the fact that the
not_consumedvariable holds a reference to the concerned argument whereas it should not.The
_to_consumevariable of theVariablesCheckerinstance is not correct in this case. The origin of the problem is located inside thevisit_namemethod.On line 1102, there is a test to know if the name has already been consumed. When checking for the name
keycorresponding to the node inside the dict comprehension (key.items()) the answer is that it has been consumed because there is a member ofconsumedwhich name iskey. But this one corresponds to the variablekeyof the dict comprehension (for key, value in ...). Due to this confusion the namekeycorresponding to the nodekey.items()is not marked as consumed and thus leads to the emission of the false-positive message.I just added a test to avoid this confusion.
This test is inside the
_has_homonym_in_upper_function_scopemethod which returns True if there is another member with the same name in an upper function scope.As i got some problems to understand this method i decided to introduce a new class named
NamesConsumer. The_to_consumevariable is now a list ofNamesConsumerinstances instead of being a list of tuples with two first members being dictionaries and the third member a string.It allows also to have a better printing in debug sessions thanks to the
__repr__method.I hope the method to be a little bit clearer with this change.
@PCManticore, has i do not yet fully understand this complex method could you please review this PR carefully (i'm sure you always do!). I just ensured that there are no more false positive and that i didn't break the unit tests.