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.

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

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

3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/9554.false_positive
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

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
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
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
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
Loading