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

Jump to bottom

Fix a false positive for consider-using-dict-items #9594

Merged
merged 8 commits into from
May 4, 2024
Merged

Fix a false positive for consider-using-dict-items #9594

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
13accb5
Fix a false positive for ``consider-using-dict-items`` when iterating…
mbyrnepr2 May 3, 2024
bf17fd7
Update doc/whatsnew/fragments/9554.false_positive
mbyrnepr2 May 4, 2024
9cc8a2a
Update tests/functional/c/consider/consider_using_dict_items.py
mbyrnepr2 May 4, 2024
b3d15fd
Update tests/functional/c/consider/consider_using_dict_items.py
mbyrnepr2 May 4, 2024
530d1e8
Regenerate the functional test output after change to the test module.
mbyrnepr2 May 4, 2024
0bd8a1a
Remove the unnecessary disable.
mbyrnepr2 May 4, 2024
7b5ddea
Run black.
mbyrnepr2 May 4, 2024
1f2a0da
Regenerate functional test output again (after running black).
mbyrnepr2 May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/9554.false_positive
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a false positive for ``consider-using-dict-items`` when iterating ``os.environ`` using the ``os.environ.keys()`` operation and then deleting an item using the key as a lookup.
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
Fix a false positive for ``consider-using-dict-items`` when iterating ``os.environ`` using the ``os.environ.keys()`` operation and then deleting an item using the key as a lookup.
Fix a false positive for ``consider-using-dict-items`` when iterating using ``keys()`` and then deleting an item using the key as a lookup.

The false positive is more generic than just os.environ, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is indeed true and I made the distinction here in the public-facing doc (compared to the commit message) because I thought it might sound odd since this situation suggests a modified iterating dict error would occur (which doesn’t happen in the case of os.environ because it is dict-like but also a bit special.
Having said that I’m not against using the word dict here instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I just checked and the commit message is the same 😅

Copy link
Member

Choose a reason for hiding this comment

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

If it's only a FP for os.environ because it's special then my suggestion is wrong 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it would manifest for any dict but for a standard dict you would also have the modify iterating dict error; but I could be overthinking that.

Copy link
Member Author

@mbyrnepr2 mbyrnepr2 May 4, 2024 •
edited

Choose a reason for hiding this comment

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

I just meant that you don’t run into modify iterating dict error for os.environ which is why I say it’s a bit special.


Closes #9554
4 changes: 4 additions & 0 deletions pylint/checkers/refactoring/recommendation_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,10 @@ def _check_consider_using_dict_items(self, node: nodes.For) -> None:
# Ignore this subscript if it is the target of an assignment
# Early termination as dict index lookup is necessary
return
if isinstance(subscript.parent, nodes.Delete):
# Ignore this subscript if the index is used to delete a
# dictionary item.
return

self.add_message("consider-using-dict-items", node=node)
return
Expand Down
9 changes: 9 additions & 0 deletions tests/functional/c/consider/consider_using_dict_items.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Emit a message for iteration through dict keys and subscripting dict with key."""
# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,redefined-outer-name,use-dict-literal,modified-iterating-dict
# pylint: disable=wrong-import-position
mbyrnepr2 marked this conversation as resolved.
Show resolved Hide resolved

def bad():
a_dict = {1: 1, 2: 2, 3: 3}
Expand Down Expand Up @@ -103,3 +104,11 @@ class Foo:
for k in d: # [consider-using-dict-items]
if '1' in d[k]: # index lookup necessary here, do not emit error
print('found 1')


# False positive in issue #9554
# https://github.com/pylint-dev/pylint/issues/9554
import os
mbyrnepr2 marked this conversation as resolved.
Show resolved Hide resolved
for var in os.environ.keys(): # [consider-iterating-dictionary]
if var.startswith('foo_'):
del os.environ[var] # index lookup necessary here, do not emit error
29 changes: 15 additions & 14 deletions tests/functional/c/consider/consider_using_dict_items.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
consider-using-dict-items:6:4:7:24:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:9:4:10:30:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:21:4:22:35:another_bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:40:0:42:18::Consider iterating with .items():UNDEFINED
consider-using-dict-items:44:0:45:20::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:47:10:47:23::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:47:0:48:20::Consider iterating with .items():UNDEFINED
consider-using-dict-items:54:0:55:24::Consider iterating with .items():UNDEFINED
consider-using-dict-items:67:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:7:4:8:24:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:10:4:11:30:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:22:4:23:35:another_bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:41:0:43:18::Consider iterating with .items():UNDEFINED
consider-using-dict-items:45:0:46:20::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:48:10:48:23::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:48:0:49:20::Consider iterating with .items():UNDEFINED
consider-using-dict-items:55:0:56:24::Consider iterating with .items():UNDEFINED
consider-using-dict-items:68:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:71:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:69:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:72:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:75:0:None:None::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:86:25:86:42::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:86:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:103:0:105:24::Consider iterating with .items():UNDEFINED
consider-using-dict-items:73:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:76:0:None:None::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:87:25:87:42::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:87:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:104:0:106:24::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:112:11:112:28::Consider iterating the dictionary directly instead of calling .keys():INFERENCE