Suppress unnecessary-list-index-lookup for whole loop on write (p…

…ylint-dev#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 pylint-dev#6818 Co-authored-by: Jacob Walls <[email protected]>
Pierre-Sassoulas · Jun 15, 2022 · a4e5743 · a4e5743
1 parent 47d6ffa
commit a4e5743
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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