Implemented new check `consider-using-dict-items` #4445

yushao2 · 2021-05-06T13:28:52Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

Implemented a new checker, consider-using-dict-items, which checks for instances where a
for loop is iterating over dictionary keys, and the same dictionary is being sub-scripted by the key in the
loop body.

Type of Changes

	Type
✓	✨ New feature

Related Issue

Closes #3389

yushao2 · 2021-05-06T13:37:49Z

it seems like one of the pre-commit checks is now failing due to the new check implemented 🤣
I will change the source file to make that compliant

************* Module pylint.checkers.typecheck
pylint/checkers/typecheck.py:1439:8: C0206: Consider iterating with .items() (consider-using-dict-items)

coveralls · 2021-05-08T20:16:51Z

Coverage increased (+0.02%) to 91.724% when pulling d9ccab1 on yushao2:checkers-3389 into 547ed55 on PyCQA:master.

Pierre-Sassoulas

Nice new checker. Although I have a feeling the tests are a little light for this kind of checker. I'll check the result on astroid and pygame before approving. (pip3 install git+ssh://[email protected]/yushao2/pylint.git)

yushao2 · 2021-05-09T14:05:57Z

Nice new checker. Although I have a feeling the tests are a little light for this kind of checker. I'll check the result on astroid and pygame before approving. (pip3 install git+ssh://[email protected]/yushao2/pylint.git)

I will try to think of more tests for this in the meantime, thanks!

yushao2 · 2021-05-09T15:11:01Z

Nice new checker. Although I have a feeling the tests are a little light for this kind of checker. I'll check the result on astroid and pygame before approving. (pip3 install git+ssh://[email protected]/yushao2/pylint.git)

I found a new bug where the checker failed to detect a new test case

def another_good():
    for k in out_of_scope_dict:
        k = 1
        k = 2
        k = 3
        print(out_of_scope_dict[k])  #should be okay, but error emitted

and my solution to that was to get the subscript's latest assignment line number and compare it to the node (for-loop)'s line number

                last_definition_lineno = value.lookup(value.name)[1][-1].lineno
                if last_definition_lineno > node.lineno:
                    # Ignore this subscript if it has been redefined after
                    # the for loop. This checks for the line number using .lookup()
                    # to get the line number where the iterating object was last 
                    # defined and compare that to the for loop's line number
                    continue

will update my PR with these changes

cdce8p

I really like this one! As a test I used it to run pylint over the codebase of home-assistant which resulted in multiple warnings. This MR will definitely improve code quality.

I though, I could contribute some test cases as well. Some of them are not yet passing though.

b_dict = {}
for k2 in b_dict:  # Should not emit warning, key access necessary
    b_dict[k2] = 2

for k3 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k3]

for k4 in b_dict.keys():  # [consider-iterating-dictionary,consider-using-dict-items]
    val = b_dict[k4]

class Foo:
    c_dict = {}

for k5 in Foo.c_dict:  # [consider-using-dict-items]
    val = Foo.c_dict[k5]

for k6 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k6]
    b_dict[k6] = 2

val = [(k7, b_dict[k7]) for k7 in b_dict if b_dict[k7]]  # [consider-using-dict-items]
val = any(True for k8 in b_dict if b_dict[k8])  # [consider-using-dict-items]
val = {k9: b_dict[k9] for k9 in b_dict}  # [consider-using-dict-items]

We could also think about what should happen if an index-lookup is used together with dict.items.

for k10, v in b_dict.items():
    val = v + 2
    print(b_dict[k10])

--
I realize this might all be a bit much at once, but keep in mind: The work you have already done is really good! Especially for a first-time contributor 🐬 These are now just the finishing touches. If you encounter any issues or need help, please don't hesitate to ask.

ChangeLog

doc/whatsnew/2.9.rst

pylint/checkers/refactoring/recommendation_checker.py

tests/functional/c/consider/consider_using_dict_items.py

yushao2 · 2021-05-10T02:27:00Z

I really like this one! As a test I used it to run pylint over the codebase of home-assistant which resulted in multiple warnings. This MR will definitely improve code quality.

I though, I could contribute some test cases as well. Some of them are not yet passing though.
b_dict = {}
for k2 in b_dict:  # Should not emit warning, key access necessary
    b_dict[k2] = 2

for k3 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k3]

for k4 in b_dict.keys():  # [consider-iterating-dictionary,consider-using-dict-items]
    val = b_dict[k4]

class Foo:
    c_dict = {}

for k5 in Foo.c_dict:  # [consider-using-dict-items]
    val = Foo.c_dict[k5]

for k6 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k6]
    b_dict[k6] = 2
val = [(k7, b_dict[k7]) for k7 in b_dict if b_dict[k7]]  # [consider-using-dict-items]
val = any(True for k8 in b_dict if b_dict[k8])  # [consider-using-dict-items]
val = {k9: b_dict[k9] for k9 in b_dict}  # [consider-using-dict-items]
We could also think about what should happen if an index-lookup is used together with dict.items.
for k10, v in b_dict.items():
    val = v + 2
    print(b_dict[k10])
--
I realize this might all be a bit much at once, but keep in mind: The work you have already done is really good! Especially for a first-time contributor These are now just the finishing touches. If you encounter any issues or need help, please don't hesitate to ask.

Thank you so much for your review! Admittedly I'm still trying to pick up the ropes here so I'm thankful for the review given and for highlighting new test cases/usage scenarios where I haven't thought of. I'll work on the suggested changes first, then try to look at making the new test cases pass.

Thanks once again!

pylint/checkers/refactoring/recommendation_checker.py

yushao2 · 2021-05-10T17:57:46Z

I really like this one! As a test I used it to run pylint over the codebase of home-assistant which resulted in multiple warnings. This MR will definitely improve code quality.

I though, I could contribute some test cases as well. Some of them are not yet passing though.
b_dict = {}
for k2 in b_dict:  # Should not emit warning, key access necessary
    b_dict[k2] = 2

for k3 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k3]

for k4 in b_dict.keys():  # [consider-iterating-dictionary,consider-using-dict-items]
    val = b_dict[k4]

class Foo:
    c_dict = {}

for k5 in Foo.c_dict:  # [consider-using-dict-items]
    val = Foo.c_dict[k5]

for k6 in b_dict:  # [consider-using-dict-items]
    val = b_dict[k6]
    b_dict[k6] = 2
val = [(k7, b_dict[k7]) for k7 in b_dict if b_dict[k7]]  # [consider-using-dict-items]
val = any(True for k8 in b_dict if b_dict[k8])  # [consider-using-dict-items]
val = {k9: b_dict[k9] for k9 in b_dict}  # [consider-using-dict-items]
We could also think about what should happen if an index-lookup is used together with dict.items.
for k10, v in b_dict.items():
    val = v + 2
    print(b_dict[k10])
--
I realize this might all be a bit much at once, but keep in mind: The work you have already done is really good! Especially for a first-time contributor These are now just the finishing touches. If you encounter any issues or need help, please don't hesitate to ask.

yushao2 · 2021-05-10T18:35:11Z

We could also think about what should happen if an index-lookup is used together with dict.items.
for k10, v in b_dict.items():
    val = v + 2
    print(b_dict[k10])
--
I realize this might all be a bit much at once, but keep in mind: The work you have already done is really good! Especially for a first-time contributor These are now just the finishing touches. If you encounter any issues or need help, please don't hesitate to ask.

@cdce8p,

I think this should be in a different error message altogether -- should I try my hand at implementing it within this PR or perhaps I should create a separate PR for that. I think it might be better to not bloat up this PR

cdce8p

A few more comments. This is already looking really good!

As mentioned here, you can move _check_if_dict_keys_used to pylint/checkers/utils.py. I would also recommend renaming it.

A nice to have would be if you could order the individual test cases a bit. The way they are currently, it's sometimes difficult to understand what they actually should test.

--

think this should be in a different error message altogether -- should I try my hand at implementing it within this PR or perhaps I should create a separate PR for that. I think it might be better to not bloat up this PR

I fully agree. If you like, you could take a stab at it after this PR is done.

pylint/checkers/refactoring/recommendation_checker.py

tests/functional/c/consider/consider_using_dict_items.py

pylint/checkers/refactoring/recommendation_checker.py

cdce8p · 2021-05-10T21:57:48Z

@yushao2 Just one more thing: Could you edit the previous commit messages and remove the issue link and the @ reference? They unfortunately don't work that well together with Github, e.g., I would get a notification anytime the commit is pushed to a new branch.

yushao2 · 2021-05-11T02:30:44Z

A few more comments. This is already looking really good!

As mentioned here, you can move _check_if_dict_keys_used to pylint/checkers/utils.py. I would also recommend renaming it.

A nice to have would be if you could order the individual test cases a bit. The way they are currently, it's sometimes difficult to understand what they actually should test.

--

think this should be in a different error message altogether -- should I try my hand at implementing it within this PR or perhaps I should create a separate PR for that. I think it might be better to not bloat up this PR

I fully agree. If you like, you could take a stab at it after this PR is done.

I just went through the reviews and did the necessary changes -- pardon the amount of commits here, I'll squash the commits in the PR when all looks good and ready to merge

cdce8p

A few more comments. Mainly about the utils function.

FYI: I pushed a small commit with minor style changes. I though it wasn't worth leaving extra comments for them. Just so you know.

I just went through the reviews and did the necessary changes -- pardon the amount of commits here, I'll squash the commits in the PR when all looks good and ready to merge

You don't need to. I'll squash merge the PR anyway. Just keep what you think is logical or what would improve the review process, i.e., I would appreciate if you don't squash your next changes into the old ones 😄

pylint/checkers/refactoring/recommendation_checker.py

pylint/checkers/utils.py

- Also, added accompanying test cases

cdce8p · 2021-05-11T12:31:19Z

Seems like it was necessary after all. py36 - 38 tests are failing. Just interesting that py39 is fine.

if isinstance(value, astroid.Index):
    value = value.value

yushao2 · 2021-05-11T12:45:15Z

Seems like it was necessary after all. py36 - 38 tests are failing. Just interesting that py39 is fine.
if isinstance(value, astroid.Index):
    value = value.value

I'll work on this again in a bit -- got interrupted by some schoolwork matters

cdce8p · 2021-05-11T13:47:09Z

I'll work on this again in a bit -- got interrupted by some schoolwork matters

No hurry. Schoolwork is more important

Seems like it was necessary after all. py36 - 38 tests are failing. Just interesting that py39 is fine.

Just looked at the astroid code, turns out astroid.Index is only used until Python 3.8

cdce8p · 2021-05-11T14:33:47Z

pylint/checkers/utils.py

+    # Is it a proper keys call?
+    is_iterating_dict_keys = (
+        isinstance(node.iter, astroid.Call)
+        and isinstance(node.iter.func, astroid.Attribute)
+        and node.iter.func.attrname == "keys"
+        and isinstance(safe_infer(node.iter.func), astroid.BoundMethod)
+    )
+    # Is it a proper dictionary?
+    is_iterating_dict = isinstance(
+        node.iter, (astroid.Name, astroid.Attribute)
+    ) and isinstance(safe_infer(node.iter), astroid.Dict)
+
+    if is_iterating_dict or is_iterating_dict_keys:
+        return node.iter.as_string().partition(".keys")[0]
+
+    return None


I should have probably explained my suggestion better. IMO this function is now a lot more difficult to understand. What do you think of this one here?

# Is it a proper keys call? if ( isinstance(node.iter, astroid.Call) and isinstance(node.iter.func, astroid.Attribute) and node.iter.func.attrname == "keys" ): inferred = safe_infer(node.iter.func) if not isinstance(inferred, astroid.BoundMethod): return None return node.iter.as_string().rpartition(".keys")[0] # Is it a dictionary? if isinstance(node.iter, (astroid.Name, astroid.Attribute)): inferred = safe_infer(node.iter) if not isinstance(inferred, astroid.Dict): return None return node.iter.as_string() return None

Also note the use of rpartition instead of partition.

I understand the usage of rpartition here -- the new test case was written because it was failing on (left) partition 😄

Thank you so much for your continued patience with reviewing my code and giving suggestions -- I'm learning a lot from this!

Let me work on the changes and ping again when I'm done

Understood your code and what you meant now -- have changed in 797b050

tests/functional/c/consider/consider_using_dict_items.py

pylint/checkers/refactoring/recommendation_checker.py

cdce8p

I pushed some last minor style changes and did a final test run. Everything seems to work as expected, so I guess this PR is finished 🚀

Thank you for continuing to work on it, even after those long reviews. You really did an amazing job here. Looking forward to seeing more PRs from you 😉

Pierre-Sassoulas added Checkers Related to a checker Enhancement ✨ Improvement to a component labels May 6, 2021

yushao2 force-pushed the checkers-3389 branch from 4f1d47f to 901e751 Compare May 6, 2021 13:44

yushao2 mentioned this pull request May 6, 2021

Suggest to iterate thorough dict items() instead of keys #3389

Closed

yushao2 force-pushed the checkers-3389 branch from 901e751 to b38c035 Compare May 8, 2021 17:27

Pierre-Sassoulas reviewed May 9, 2021

View reviewed changes

Pierre-Sassoulas self-requested a review May 9, 2021 12:35

cdce8p changed the title ~~Implemented new check consider-using-dict-items (#3389)~~ Implemented new check consider-using-dict-items May 9, 2021

cdce8p requested changes May 9, 2021

View reviewed changes

yushao2 force-pushed the checkers-3389 branch from 6b65cef to 9c3b031 Compare May 10, 2021 02:29

cdce8p assigned cdce8p and unassigned cdce8p May 10, 2021

yushao2 force-pushed the checkers-3389 branch from 299eeca to 4cf3906 Compare May 10, 2021 17:53

yushao2 requested a review from cdce8p May 10, 2021 17:54

yushao2 commented May 10, 2021

View reviewed changes

pylint/checkers/refactoring/recommendation_checker.py Outdated Show resolved Hide resolved

cdce8p requested changes May 10, 2021

View reviewed changes

yushao2 force-pushed the checkers-3389 branch from 4cf3906 to 68faccb Compare May 11, 2021 02:29

cdce8p requested changes May 11, 2021

View reviewed changes

yushao2 added 2 commits May 11, 2021 19:49

Implemented new check consider-using-dict-items

b011d87

Added additional logic and tests for consider-using-dict-items

b92a573

yushao2 and others added 7 commits May 11, 2021 19:49

Implemented consider-using-dict-items on comprehension

b9af784

- Also, added accompanying test cases

Grouped test items

d2a85db

Refactored utility function

c30df89

Added logic to check for augassign statements

08ba850

Minor style changes

31bbd17

Added more test cases to check for accessing other dicts

e8161a5

Cleaned up logic based on PR review

f54b22d

yushao2 force-pushed the checkers-3389 branch from 91c3201 to f54b22d Compare May 11, 2021 12:00

yushao2 added 2 commits May 11, 2021 21:58

Added test case for .keys() within comprehension

b1fb77b

Revised codes based on review

c79d112

yushao2 requested a review from cdce8p May 11, 2021 14:10

cdce8p requested changes May 11, 2021

View reviewed changes

Improved readability

797b050

yushao2 requested a review from cdce8p May 11, 2021 15:36

Last small style changes

d9ccab1

cdce8p approved these changes May 11, 2021

View reviewed changes

cdce8p merged commit 6044930 into pylint-dev:master May 11, 2021

yushao2 deleted the checkers-3389 branch May 12, 2021 01:20

yushao2 mentioned this pull request May 12, 2021

New checker for when an index-lookup is used together with dict.items() #4470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented new check `consider-using-dict-items` #4445

Implemented new check `consider-using-dict-items` #4445

yushao2 commented May 6, 2021 •

edited

Loading

yushao2 commented May 6, 2021

coveralls commented May 8, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

yushao2 commented May 9, 2021

yushao2 commented May 9, 2021 •

edited

Loading

cdce8p left a comment

yushao2 commented May 10, 2021

yushao2 commented May 10, 2021

yushao2 commented May 10, 2021

cdce8p left a comment

cdce8p commented May 10, 2021

yushao2 commented May 11, 2021

cdce8p left a comment

cdce8p commented May 11, 2021

yushao2 commented May 11, 2021

cdce8p commented May 11, 2021

cdce8p May 11, 2021

yushao2 May 11, 2021

yushao2 May 11, 2021

cdce8p left a comment

Implemented new check consider-using-dict-items #4445

Implemented new check consider-using-dict-items #4445

Conversation

yushao2 commented May 6, 2021 • edited Loading

Steps

Description

Type of Changes

Related Issue

yushao2 commented May 6, 2021

coveralls commented May 8, 2021 • edited Loading

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

yushao2 commented May 9, 2021

yushao2 commented May 9, 2021 • edited Loading

cdce8p left a comment

Choose a reason for hiding this comment

yushao2 commented May 10, 2021

yushao2 commented May 10, 2021

yushao2 commented May 10, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p commented May 10, 2021

yushao2 commented May 11, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p commented May 11, 2021

yushao2 commented May 11, 2021

cdce8p commented May 11, 2021

cdce8p May 11, 2021

Choose a reason for hiding this comment

yushao2 May 11, 2021

Choose a reason for hiding this comment

yushao2 May 11, 2021

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

Implemented new check `consider-using-dict-items` #4445

Implemented new check `consider-using-dict-items` #4445

yushao2 commented May 6, 2021 •

edited

Loading

coveralls commented May 8, 2021 •

edited

Loading

yushao2 commented May 9, 2021 •

edited

Loading