From b199fa6993211c89b391672b79477add64f324c5 Mon Sep 17 00:00:00 2001 From: yushao2 <36848472+yushao2@users.noreply.github.com> Date: Mon, 24 May 2021 04:22:30 +0800 Subject: [PATCH] Implemented new check unnecessary-dict-index-lookup (#4485) --- ChangeLog | 5 + doc/whatsnew/2.9.rst | 2 + .../refactoring/refactoring_checker.py | 130 +++++++++++++++++- .../unnecessary_dict_index_lookup.py | 63 +++++++++ .../unnecessary_dict_index_lookup.txt | 23 ++++ 5 files changed, 220 insertions(+), 3 deletions(-) create mode 100644 tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py create mode 100644 tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt diff --git a/ChangeLog b/ChangeLog index f2bdc484af..d57843e2f4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -62,6 +62,11 @@ modules are added. * Fix documentation errors in "Block disables" paragraph of User Guide. +* New checker ``unnecessary-dict-index-lookup``. Emitted when iterating over dictionary items + (key-value pairs) and accessing the value by index lookup. + + Closes #4470 + What's New in Pylint 2.8.2? =========================== diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 4e6249a9a4..3ea0e19407 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -20,6 +20,8 @@ New checkers * An ``ignore_signatures`` option has been added to the similarity checker. It will permits to reduce false positives when multiple functions have the same parameters. +* ``unnecessary-dict-index-lookup``: Emitted when iterating over dictionary items + (key-value pairs) and accessing the value by index lookup. Other Changes ============= diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 2b01894cc3..92f9371f09 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -6,7 +6,7 @@ import itertools import tokenize from functools import reduce -from typing import List +from typing import List, Union, cast import astroid @@ -347,6 +347,13 @@ class RefactoringChecker(checkers.BaseTokenChecker): "Emitted if a resource-allocating assignment or call may be replaced by a 'with' block. " "By using 'with' the release of the allocated resources is ensured even in the case of an exception.", ), + "R1733": ( + "Unnecessary dictionary index lookup, use '%s' instead", + "unnecessary-dict-index-lookup", + "Emitted when iterating over the dictionary items (key-item pairs) and accessing the " + "value by index lookup. " + "The value can be accessed directly instead.", + ), } options = ( ( @@ -534,9 +541,14 @@ def _check_redefined_argument_from_local(self, name_node): args=(name_node.name,), ) - @utils.check_messages("redefined-argument-from-local", "too-many-nested-blocks") + @utils.check_messages( + "redefined-argument-from-local", + "too-many-nested-blocks", + "unnecessary-dict-index-lookup", + ) def visit_for(self, node): self._check_nested_blocks(node) + self._check_unnecessary_dict_index_lookup(node) for name in node.target.nodes_of_class(astroid.AssignName): self._check_redefined_argument_from_local(name) @@ -1330,9 +1342,10 @@ def _check_consider_using_join(self, aug_assign): def visit_augassign(self, node): self._check_consider_using_join(node) - @utils.check_messages("unnecessary-comprehension") + @utils.check_messages("unnecessary-comprehension", "unnecessary-dict-index-lookup") def visit_comprehension(self, node): self._check_unnecessary_comprehension(node) + self._check_unnecessary_dict_index_lookup(node) def _check_unnecessary_comprehension(self, node): if ( @@ -1601,3 +1614,114 @@ def _check_return_at_the_end(self, node): # return None" elif isinstance(last.value, astroid.Const) and (last.value.value is None): self.add_message("useless-return", node=node) + + def _check_unnecessary_dict_index_lookup( + self, node: Union[astroid.For, astroid.Comprehension] + ) -> None: + """Add message when accessing dict values by index lookup.""" + # Verify that we have a .items() call and + # that the object which is iterated is used as a subscript in the + # body of the for. + # Is it a proper items call? + if ( + isinstance(node.iter, astroid.Call) + and isinstance(node.iter.func, astroid.Attribute) + and node.iter.func.attrname == "items" + ): + inferred = utils.safe_infer(node.iter.func) + if not isinstance(inferred, astroid.BoundMethod): + return + iterating_object_name = node.iter.func.expr.as_string() + + # Verify that the body of the for loop uses a subscript + # with the object that was iterated. This uses some heuristics + # in order to make sure that the same object is used in the + # for body. + + children = ( + node.body + if isinstance(node, astroid.For) + else node.parent.get_children() + ) + for child in children: + for subscript in child.nodes_of_class(astroid.Subscript): + subscript = cast(astroid.Subscript, subscript) + + if not isinstance( + subscript.value, (astroid.Name, astroid.Attribute) + ): + continue + + value = subscript.slice + if isinstance(value, astroid.Index): + value = value.value + + if isinstance(node, astroid.For) and ( + isinstance(subscript.parent, astroid.Assign) + and subscript in subscript.parent.targets + or isinstance(subscript.parent, astroid.AugAssign) + and subscript == subscript.parent.target + ): + # Ignore this subscript if it is the target of an assignment + continue + + # Case where .items is assigned to k,v (i.e., for k, v in d.items()) + if isinstance(value, astroid.Name): + if ( + not isinstance(node.target, astroid.Tuple) + or value.name != node.target.elts[0].name + or iterating_object_name != subscript.value.as_string() + ): + continue + + if ( + isinstance(node, astroid.For) + and value.lookup(value.name)[1][-1].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 + + self.add_message( + "unnecessary-dict-index-lookup", + node=subscript, + args=(node.target.elts[1].as_string()), + ) + + # Case where .items is assigned to single var (i.e., for item in d.items()) + elif isinstance(value, astroid.Subscript): + if ( + not isinstance(node.target, astroid.AssignName) + or node.target.name != value.value.name + or iterating_object_name != subscript.value.as_string() + ): + continue + + if ( + isinstance(node, astroid.For) + and value.value.lookup(value.value.name)[1][-1].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 + + # check if subscripted by 0 (key) + subscript_val = value.slice + if isinstance(subscript_val, astroid.Index): + subscript_val = subscript_val.value + inferred = utils.safe_infer(subscript_val) + if ( + not isinstance(inferred, astroid.Const) + or inferred.value != 0 + ): + continue + self.add_message( + "unnecessary-dict-index-lookup", + node=subscript, + args=("1".join(value.as_string().rsplit("0", maxsplit=1)),), + ) diff --git a/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py new file mode 100644 index 0000000000..901e176b36 --- /dev/null +++ b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py @@ -0,0 +1,63 @@ +# pylint: disable=missing-docstring, too-few-public-methods, expression-not-assigned, line-too-long + +a_dict = dict() +b_dict = dict() + +for k, v in a_dict.items(): + print(a_dict[k]) # [unnecessary-dict-index-lookup] + print(b_dict[k]) # Should not emit warning, accessing other dictionary + a_dict[k] = 123 # Should not emit warning, key access necessary + a_dict[k] += 123 # Should not emit warning, key access necessary + print(a_dict[k]) # [unnecessary-dict-index-lookup] + k = "another key" + print(a_dict[k]) # This is fine, key reassigned + + +# Tests on comprehensions +{v: 1 for k, v in a_dict.items() if a_dict[k]} # [unnecessary-dict-index-lookup] +{v: 1 for k, v in a_dict.items() if k} # This is fine, no indexing +{a_dict[k]: 1 for k, v in a_dict.items() if k} # [unnecessary-dict-index-lookup] +{a_dict[k]: 1 for k, v in a_dict.items() if a_dict[k]} # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup] + +[v for k, v in a_dict.items() if a_dict[k]] # [unnecessary-dict-index-lookup] +[v for k, v in a_dict.items() if k] # This is fine, no indexing +[a_dict[k] for k, v in a_dict.items() if k] # [unnecessary-dict-index-lookup] +[a_dict[k] for k, v in a_dict.items() if a_dict[k]] # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup] + + +# Tests on dict attribute of a class +class Foo: + c_dict = {} + +for k, v in Foo.c_dict.items(): + print(b_dict[k]) # Should not emit warning, accessing other dictionary + print(Foo.c_dict[k]) # [unnecessary-dict-index-lookup] + Foo.c_dict[k] += Foo.c_dict[k] # [unnecessary-dict-index-lookup] + Foo.c_dict[k] += v # key access necessary + +# Tests on comprehensions +{v: 1 for k, v in Foo.c_dict.items() if Foo.c_dict[k]} # [unnecessary-dict-index-lookup] +{v: 1 for k, v in Foo.c_dict.items() if k} # This is fine, no indexing +{Foo.c_dict[k]: 1 for k, v in Foo.c_dict.items() if k} # [unnecessary-dict-index-lookup] +{Foo.c_dict[k]: 1 for k, v in Foo.c_dict.items() if Foo.c_dict[k]} # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup] + +[v for k, v in Foo.c_dict.items() if Foo.c_dict[k]] # [unnecessary-dict-index-lookup] +[v for k, v in Foo.c_dict.items() if k] # This is fine, no indexing +[Foo.c_dict[k] for k, v in Foo.c_dict.items() if k] # [unnecessary-dict-index-lookup] +[Foo.c_dict[k] for k, v in Foo.c_dict.items() if Foo.c_dict[k]] # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup] + +# Test assigning d.items() to a single variable +d = {1: "a", 2: "b"} +for item in d.items(): + print(item[0]) + print(d[item[0]]) # [unnecessary-dict-index-lookup] + +[item[0] for item in d.items()] +[d[item[0]] for item in d.items()] # [unnecessary-dict-index-lookup] + +# Reassigning single var +for item in d.items(): + print(item[0]) + print(d[item[0]]) # [unnecessary-dict-index-lookup] + item = (2, "b") + print(d[item[0]]) # This is fine, no warning thrown as key has been reassigned diff --git a/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt new file mode 100644 index 0000000000..dedf9e7ccf --- /dev/null +++ b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt @@ -0,0 +1,23 @@ +unnecessary-dict-index-lookup:7:10::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:11:10::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:17:36::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:19:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:20:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:20:44::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:22:33::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:24:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:25:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:25:41::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:34:10::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:35:21::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:39:40::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:41:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:42:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:42:52::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:44:37::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:46:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:47:1::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:47:49::Unnecessary dictionary index lookup, use 'v' instead +unnecessary-dict-index-lookup:53:10::Unnecessary dictionary index lookup, use 'item[1]' instead +unnecessary-dict-index-lookup:56:1::Unnecessary dictionary index lookup, use 'item[1]' instead +unnecessary-dict-index-lookup:61:10::Unnecessary dictionary index lookup, use 'item[1]' instead