Skip to content

Commit

Permalink
Fix a false positive for consider-using-dict-items (#9594)
Browse files Browse the repository at this point in the history
When iterating ``os.environ`` using the ``os.environ.keys()`` operation and then deleting an item using the key as a lookup.

Closes #9554

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>>
  • Loading branch information
mbyrnepr2 and Pierre Sassoulas committed May 4, 2024
1 parent c032181 commit c864cd4
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 24 deletions.
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/9554.false_positive
@@ -0,0 +1,3 @@
Fix a false positive for ``consider-using-dict-items`` when iterating using ``keys()`` and then deleting an item using the key as a lookup.

Closes #9554
4 changes: 4 additions & 0 deletions pylint/checkers/refactoring/recommendation_checker.py
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
32 changes: 24 additions & 8 deletions tests/functional/c/consider/consider_using_dict_items.py
@@ -1,6 +1,10 @@
"""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

import os


def bad():
a_dict = {1: 1, 2: 2, 3: 3}
for k in a_dict: # [consider-using-dict-items]
Expand All @@ -15,12 +19,15 @@ def good():
for k in a_dict:
print(k)


out_of_scope_dict = dict()


def another_bad():
for k in out_of_scope_dict: # [consider-using-dict-items]
print(out_of_scope_dict[k])


def another_good():
for k in out_of_scope_dict:
k = 1
Expand All @@ -47,9 +54,11 @@ def another_good():
for k4 in b_dict.keys(): # [consider-iterating-dictionary,consider-using-dict-items]
val = b_dict[k4]


class Foo:
c_dict = {}


# Should emit warning when iterating over a dict attribute of a class
for k5 in Foo.c_dict: # [consider-using-dict-items]
val = Foo.c_dict[k5]
Expand Down Expand Up @@ -88,18 +97,25 @@ class Foo:
# Test false positive described in #4630
# (https://github.com/pylint-dev/pylint/issues/4630)

d = {'key': 'value'}
d = {"key": "value"}

for k in d: # this is fine, with the reassignment of d[k], d[k] is necessary
d[k] += '123'
if '1' in d[k]: # index lookup necessary here, do not emit error
print('found 1')
d[k] += "123"
if "1" in d[k]: # index lookup necessary here, do not emit error
print("found 1")

for k in d: # if this gets rewritten to d.items(), we are back to the above problem
d[k] = d[k] + 1
if '1' in d[k]: # index lookup necessary here, do not emit error
print('found 1')
if "1" in d[k]: # index lookup necessary here, do not emit error
print("found 1")

for k in d: # [consider-using-dict-items]
if '1' in d[k]: # index lookup necessary here, do not emit error
print('found 1')
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
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
33 changes: 17 additions & 16 deletions tests/functional/c/consider/consider_using_dict_items.txt
@@ -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: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: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:10:4:11:24:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:13:4:14:30:bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:27:4:28:35:another_bad:Consider iterating with .items():UNDEFINED
consider-using-dict-items:47:0:49:18::Consider iterating with .items():UNDEFINED
consider-using-dict-items:51:0:52:20::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:54:10:54:23::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:54:0:55:20::Consider iterating with .items():UNDEFINED
consider-using-dict-items:63:0:64:24::Consider iterating with .items():UNDEFINED
consider-using-dict-items:76:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:77:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:80:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:81:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:84:0:None:None::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:95:25:95:42::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
consider-using-dict-items:95:0:None:None::Consider iterating with .items():UNDEFINED
consider-using-dict-items:112:0:114:24::Consider iterating with .items():UNDEFINED
consider-iterating-dictionary:119:11:119:28::Consider iterating the dictionary directly instead of calling .keys():INFERENCE

0 comments on commit c864cd4

Please sign in to comment.