From a4e5743183512be25e97e8346d790d55858c9c98 Mon Sep 17 00:00:00 2001 From: Tim Martin Date: Sat, 11 Jun 2022 07:47:55 +0100 Subject: [PATCH] Suppress ``unnecessary-list-index-lookup`` for whole loop on write (#6845) The ``unnecessary-list-index-lookup`` checker was assuming that if the subscript was written to then it would only invalidate access on later lines, but this is not necessarily the case if an inner loop is nested inside the one being checked. Fixes #6818 Co-authored-by: Jacob Walls --- doc/whatsnew/2/2.14/full.rst | 5 + .../refactoring/refactoring_checker.py | 110 +++++++++++++++--- .../unnecessary_dict_index_lookup.py | 6 + .../unnecessary_list_index_lookup.py | 12 ++ .../unnecessary_list_index_lookup.txt | 1 + 5 files changed, 116 insertions(+), 18 deletions(-) diff --git a/doc/whatsnew/2/2.14/full.rst b/doc/whatsnew/2/2.14/full.rst index 9099c17926..b8023ddc6d 100644 --- a/doc/whatsnew/2/2.14/full.rst +++ b/doc/whatsnew/2/2.14/full.rst @@ -5,6 +5,11 @@ What's New in Pylint 2.14.2? ---------------------------- Release date: TBA +* Fixed a false positive in ``unnecessary-list-index-lookup`` and ``unnecessary-dict-index-lookup`` + when the subscript is updated in the body of a nested loop. + + Closes #6818 + * Fixed a false positive for ``used-before-assignment`` when a try block returns but an except handler defines a name via type annotation. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 66773e2b38..265b59d992 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1921,14 +1921,30 @@ def _check_unnecessary_dict_index_lookup( return iterating_object_name = node.iter.func.expr.as_string() + # Store potential violations. These will only be reported if we don't + # discover any writes to the collection during the loop. + messages = [] + # 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, nodes.For) else node.parent.get_children() + node.body + if isinstance(node, nodes.For) + else list(node.parent.get_children()) + ) + + # Check if there are any for / while loops within the loop in question; + # If so, we will be more conservative about reporting errors as we + # can't yet do proper control flow analysis to be sure when + # reassignment will affect us + nested_loops = itertools.chain.from_iterable( + child.nodes_of_class((nodes.For, nodes.While)) for child in children ) + has_nested_loops = next(nested_loops, None) is not None + for child in children: for subscript in child.nodes_of_class(nodes.Subscript): if not isinstance(subscript.value, (nodes.Name, nodes.Attribute)): @@ -1968,11 +1984,19 @@ def _check_unnecessary_dict_index_lookup( # 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()), - ) + if has_nested_loops: + messages.append( + { + "node": subscript, + "variable": node.target.elts[1].as_string(), + } + ) + else: + 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, nodes.Subscript): @@ -1999,11 +2023,31 @@ def _check_unnecessary_dict_index_lookup( inferred = utils.safe_infer(value.slice) if not isinstance(inferred, nodes.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)),), - ) + + if has_nested_loops: + messages.append( + { + "node": subscript, + "variable": "1".join( + value.as_string().rsplit("0", maxsplit=1) + ), + } + ) + else: + self.add_message( + "unnecessary-dict-index-lookup", + node=subscript, + args=( + "1".join(value.as_string().rsplit("0", maxsplit=1)), + ), + ) + + for message in messages: + self.add_message( + "unnecessary-dict-index-lookup", + node=message["node"], + args=(message["variable"],), + ) def _check_unnecessary_list_index_lookup( self, node: nodes.For | nodes.Comprehension @@ -2029,9 +2073,25 @@ def _check_unnecessary_list_index_lookup( iterating_object_name = node.iter.args[0].name value_variable = node.target.elts[1] + # Store potential violations. These will only be reported if we don't + # discover any writes to the collection during the loop. + bad_nodes = [] + children = ( - node.body if isinstance(node, nodes.For) else node.parent.get_children() + node.body + if isinstance(node, nodes.For) + else list(node.parent.get_children()) + ) + + # Check if there are any for / while loops within the loop in question; + # If so, we will be more conservative about reporting errors as we + # can't yet do proper control flow analysis to be sure when + # reassignment will affect us + nested_loops = itertools.chain.from_iterable( + child.nodes_of_class((nodes.For, nodes.While)) for child in children ) + has_nested_loops = next(nested_loops, None) is not None + for child in children: for subscript in child.nodes_of_class(nodes.Subscript): if isinstance(node, nodes.For) and _is_part_of_assignment_target( @@ -2070,9 +2130,23 @@ def _check_unnecessary_list_index_lookup( # reassigned on a later line, so it can't be used. continue - self.add_message( - "unnecessary-list-index-lookup", - node=subscript, - args=(node.target.elts[1].name,), - confidence=HIGH, - ) + if has_nested_loops: + # Have found a likely issue, but since there are nested + # loops we don't want to report this unless we get to the + # end of the loop without updating the collection + bad_nodes.append(subscript) + else: + self.add_message( + "unnecessary-list-index-lookup", + node=subscript, + args=(node.target.elts[1].name,), + confidence=HIGH, + ) + + for subscript in bad_nodes: + self.add_message( + "unnecessary-list-index-lookup", + node=subscript, + args=(node.target.elts[1].name,), + confidence=HIGH, + ) diff --git a/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py index 79f9f6a351..74bf4e611c 100644 --- a/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py @@ -124,3 +124,9 @@ class Foo: d = {'a': 1, 'b': 2, 'c': 3} for key, val in d.items(): ([d[key], x], y) = ([1, 2], 3) + +# Regression test for https://github.com/PyCQA/pylint/issues/6818 +d = {'a': 1, 'b': 2, 'c': 3} +for key, val in d.items(): + while d[key] > 0: + d[key] -= 1 diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py index 355d42a7f0..c9aad6d9a2 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py @@ -62,3 +62,15 @@ def process_list_again(data): num_list = [1, 2, 3] for a, b in enumerate(num_list): ([x, num_list[a]], _) = ([5, 6], 1) + +# Regression test for https://github.com/PyCQA/pylint/issues/6818 +updated_list = [1, 2, 3] +for idx, val in enumerate(updated_list): + while updated_list[idx] > 0: + updated_list[idx] -= 1 + +updated_list = [1, 2, 3] +for idx, val in enumerate(updated_list): + print(updated_list[idx]) # [unnecessary-list-index-lookup] + updated_list[idx] -= 1 + print(updated_list[idx]) diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt index 0ff19dbf77..9a894e139e 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt @@ -1,3 +1,4 @@ unnecessary-list-index-lookup:8:10:8:22::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:43:52:43:64::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:46:10:46:22::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:74:10:74:27::Unnecessary list index lookup, use 'val' instead:HIGH