Skip to content
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.

Sign up for GitHub

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

Make consider-iterating-dictionary consider memb. check #4997

Merged
merged 9 commits into from
Sep 14, 2021
Merged

Make consider-iterating-dictionary consider memb. check #4997

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
400f8d7
Make ``consider-iterating-dictionary`` consider memb. check
DanielNoord Sep 13, 2021
82cd6dc
Add additional tests and use-cases
DanielNoord Sep 14, 2021
0b40b49
Split pylint disable's
DanielNoord Sep 14, 2021
556d627
Code review
DanielNoord Sep 14, 2021
5a3b8e9
Merge branch 'main' of https://github.com/PyCQA/pylint into consider-…
DanielNoord Sep 14, 2021
46ade44
Change var name to uppercase
DanielNoord Sep 14, 2021
6a77424
Add additional check and test
DanielNoord Sep 14, 2021
aae8afc
Update pylint/checkers/refactoring/recommendation_checker.py
DanielNoord Sep 14, 2021
d8bf54c
Doc cleanup
cdce8p Sep 14, 2021
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
4 changes: 4 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ Release date: TBA

Closes #4616

* The ``consider-iterating-dictionary`` checker now also considers membership checks

Closes #4069


What's New in Pylint 2.10.3?
============================
Expand Down
4 changes: 4 additions & 0 deletions doc/whatsnew/2.11.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,7 @@ Other Changes

Closes #1375
Closes #330

* The ``consider-iterating-dictionary`` checker now also considers membership checks

Closes #4069
23 changes: 13 additions & 10 deletions pylint/checkers/refactoring/recommendation_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ class RecommendationChecker(checkers.BaseChecker):
"Consider iterating the dictionary directly instead of calling .keys()",
"consider-iterating-dictionary",
"Emitted when the keys of a dictionary are iterated through the .keys() "
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
"method. It is enough to just iterate through the dictionary itself, as "
'in "for key in dictionary".',
"method or when .keys() is used for a membership check. "
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
"It is enough to just iterate through the dictionary itself, as "
'in "for key in dictionary". For membership checks '
'("if key in dictionary") this is actually faster.',
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
),
"C0206": (
"Consider iterating with .items()",
Expand Down Expand Up @@ -75,16 +77,17 @@ def _check_consider_iterating_dictionary(self, node: nodes.Call) -> None:
return
if node.func.attrname != "keys":
return
if not isinstance(node.parent, (nodes.For, nodes.Comprehension)):
return

inferred = utils.safe_infer(node.func)
if not isinstance(inferred, astroid.BoundMethod) or not isinstance(
inferred.bound, nodes.Dict
if (
isinstance(node.parent, (nodes.For, nodes.Comprehension))
or isinstance(node.parent, nodes.Compare)
and any(op for op, _ in node.parent.ops if op in {"in", "==", "is"})
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
):
return
inferred = utils.safe_infer(node.func)
if not isinstance(inferred, astroid.BoundMethod) or not isinstance(
inferred.bound, nodes.Dict
):
return

if isinstance(node.parent, (nodes.For, nodes.Comprehension)):
self.add_message("consider-iterating-dictionary", node=node)

def _check_use_maxsplit_arg(self, node: nodes.Call) -> None:
Expand Down
31 changes: 30 additions & 1 deletion tests/functional/c/consider/consider_iterating_dictionary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods, no-member, import-error, no-self-use, line-too-long, useless-object-inheritance, unnecessary-comprehension
# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods
# pylint: disable=no-member, import-error, no-self-use, line-too-long, useless-object-inheritance
# pylint: disable=unnecessary-comprehension, use-dict-literal
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# pylint: disable=unnecessary-comprehension, use-dict-literal
# pylint: disable=unnecessary-comprehension, use-dict-literal,invalid-name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be better to change the name than add an additional disable here.
However, I don't see var in bad-names in ./pylintrc.

Furthermore, this doesn't explain why this is only being thrown in the CI. The tests pass locally for me (and I think also for @cdce8p). Probably should try and investigate what is exactly causing this before adding a disable.
@cdce8p seems to have an idea, or at least has located the source

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to replicate the failure when running under tox: (tox -e py36 -- -k consider_iterating). Sorry about jumping in, I was just pinged by the mention of pylint-dev/astroid#979

Copy link
Collaborator Author

@DanielNoord DanielNoord Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I forgot to add --recreate, which you probably did some time before. Can now indeed reproduce.

I can change this to another variable name. Can somebody tell me where pylint is getting the bad-names list from? I'm probably overlooking something..

Edit: Got it. It is because var is lowercase, uppercase fixes the issue. Pushing now!

Copy link
Contributor

Choose a reason for hiding this comment

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

As to why this shows up after that merge on astroid, my reading of https://github.com/PyCQA/pylint/blob/bc95cd34071ec2e71de5bca8ff95cc9b88e23814/pylint/checkers/base.py#L1975-L1984 is that invalid-name is only emitted if the Assign .value can be inferred. Since Compare nodes are only just now inferred, these popped up.

Copy link
Member

Choose a reason for hiding this comment

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

Seems I mixed up invalid-name with disallowed-name myself.

var is fine to us, but here it's inferred as constant which should be uppercase (like @DanielNoord mentioned).
pylint-dev/astroid#979 added additional inference for compare nodes which is why these can now correctly be inferred as constants. Thus invalid-name is emitted now.

Sorry for the ping @nelfin.


from unknown import Unknown

Expand Down Expand Up @@ -36,3 +38,30 @@ def keys(self):
COMP1 = [k * 2 for k in DICT.keys()] + [k * 3 for k in DICT.keys()] # [consider-iterating-dictionary,consider-iterating-dictionary]
COMP2, COMP3 = [k * 2 for k in DICT.keys()], [k * 3 for k in DICT.keys()] # [consider-iterating-dictionary,consider-iterating-dictionary]
SOME_TUPLE = ([k * 2 for k in DICT.keys()], [k * 3 for k in DICT.keys()]) # [consider-iterating-dictionary,consider-iterating-dictionary]

# Checks for membership checks
if 1 in dict().keys(): # [consider-iterating-dictionary]
pass
if 1 in {}.keys(): # [consider-iterating-dictionary]
pass
if 1 in Unknown().keys():
pass
if 1 in Unknown.keys():
pass
if 1 in CustomClass().keys():
pass
if 1 in dict():
pass
if 1 in dict().values():
pass
if (1, 1) in dict().items():
pass
if [1] == {}.keys(): # [consider-iterating-dictionary]
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
pass
if [1] == {}:
pass
if [1] == dict():
pass
var = 1 in {}.keys() # [consider-iterating-dictionary]
var = 1 in {}
var = 1 in dict()
34 changes: 19 additions & 15 deletions tests/functional/c/consider/consider_iterating_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
consider-iterating-dictionary:23:16::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:24:16::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:25:16::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:26:21::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:27:24::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:28:24::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:29:24::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:30:29::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:31:11::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:36:24::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:36:55::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:37:31::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:37:61::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:38:30::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:38:60::Consider iterating the dictionary directly instead of calling .keys()
consider-iterating-dictionary:25:16::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:26:16::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:27:16::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:28:21::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:29:24::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:30:24::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:31:24::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:32:29::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:33:11::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:38:24::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:38:55::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:39:31::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:39:61::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:40:30::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:40:60::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:43:8::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:45:8::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:59:10::Consider iterating the dictionary directly instead of calling .keys():HIGH
consider-iterating-dictionary:65:11::Consider iterating the dictionary directly instead of calling .keys():HIGH