From cf102ffb1ef4a3b1de839f86f4f4a38c685b5ebc Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 4 May 2024 22:07:38 +0200 Subject: [PATCH] Fix a false positive for ``consider-using-dict-items`` (#9594) (#9597) 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 > (cherry picked from commit c864cd4d88e5b4e9b9bf1adc575b2fe5e1a194dd) Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> --- doc/whatsnew/fragments/9554.false_positive | 3 ++ .../refactoring/recommendation_checker.py | 4 +++ .../c/consider/consider_using_dict_items.py | 32 +++++++++++++----- .../c/consider/consider_using_dict_items.txt | 33 ++++++++++--------- 4 files changed, 48 insertions(+), 24 deletions(-) create mode 100644 doc/whatsnew/fragments/9554.false_positive diff --git a/doc/whatsnew/fragments/9554.false_positive b/doc/whatsnew/fragments/9554.false_positive new file mode 100644 index 0000000000..c8c8d71ac8 --- /dev/null +++ b/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 diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 617406613f..a1e4108be3 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -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 diff --git a/tests/functional/c/consider/consider_using_dict_items.py b/tests/functional/c/consider/consider_using_dict_items.py index 7fd74814fc..16f73d1dcd 100644 --- a/tests/functional/c/consider/consider_using_dict_items.py +++ b/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] @@ -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 @@ -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] @@ -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 diff --git a/tests/functional/c/consider/consider_using_dict_items.txt b/tests/functional/c/consider/consider_using_dict_items.txt index 280ffecf37..63f684cc6a 100644 --- a/tests/functional/c/consider/consider_using_dict_items.txt +++ b/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