New checker unnecessary-list-index-lookup (#4525) (#5834)

Author: timmartin

ChangeLog

@@ -16,6 +16,10 @@ Release date: TBA
 
   Closes #578
 
+* Added new checker ``unnecessary-list-index-lookup`` for indexing into a list while
+  iterating over ``enumerate()``.
+
+  Closes #4525
 
 * Fix false negative for ``no-member`` when attempting to assign an instance
   attribute to itself without any prior assignment.

doc/data/messages/u/unnecessary-list-index-lookup/bad.py

@@ -0,0 +1,4 @@
+letters = ['a', 'b', 'c']
+
+for index, letter in enumerate(letters):
+    print(letters[index])  # [unnecessary-list-index-lookup]

doc/data/messages/u/unnecessary-list-index-lookup/good.py

@@ -0,0 +1,4 @@
+letters = ['a', 'b', 'c']
+
+for index, letter in enumerate(letters):
+    print(letter)

doc/whatsnew/2.14.rst

@@ -18,6 +18,11 @@ New checkers
 
   Closes #578
 
+* Added new checker ``unnecessary-list-index-lookup`` for indexing into a list while
+  iterating over ``enumerate()``.
+
+  Closes #4525
+
 Removed checkers
 ================
 

pylint/checkers/refactoring/refactoring_checker.py

@@ -18,6 +18,7 @@
 from pylint import utils as lint_utils
 from pylint.checkers import utils
 from pylint.checkers.utils import node_frame_class
+from pylint.interfaces import HIGH
 
 if sys.version_info >= (3, 8):
     from functools import cached_property
@@ -437,6 +438,13 @@ class RefactoringChecker(checkers.BaseTokenChecker):
             "Emitted when using dict() to create an empty dictionary instead of the literal {}. "
             "The literal is faster as it avoids an additional function call.",
         ),
+        "R1736": (
+            "Unnecessary list index lookup, use '%s' instead",
+            "unnecessary-list-index-lookup",
+            "Emitted when iterating over an enumeration and accessing the "
+            "value by index lookup. "
+            "The value can be accessed directly instead.",
+        ),
     }
     options = (
         (
@@ -635,10 +643,12 @@ def _check_redefined_argument_from_local(self, name_node):
         "redefined-argument-from-local",
         "too-many-nested-blocks",
         "unnecessary-dict-index-lookup",
+        "unnecessary-list-index-lookup",
     )
     def visit_for(self, node: nodes.For) -> None:
         self._check_nested_blocks(node)
         self._check_unnecessary_dict_index_lookup(node)
+        self._check_unnecessary_list_index_lookup(node)
 
         for name in node.target.nodes_of_class(nodes.AssignName):
             self._check_redefined_argument_from_local(name)
@@ -1556,10 +1566,15 @@ def _check_consider_using_join(self, aug_assign):
     def visit_augassign(self, node: nodes.AugAssign) -> None:
         self._check_consider_using_join(node)
 
-    @utils.check_messages("unnecessary-comprehension", "unnecessary-dict-index-lookup")
+    @utils.check_messages(
+        "unnecessary-comprehension",
+        "unnecessary-dict-index-lookup",
+        "unnecessary-list-index-lookup",
+    )
     def visit_comprehension(self, node: nodes.Comprehension) -> None:
         self._check_unnecessary_comprehension(node)
         self._check_unnecessary_dict_index_lookup(node)
+        self._check_unnecessary_list_index_lookup(node)
 
     def _check_unnecessary_comprehension(self, node: nodes.Comprehension) -> None:
         if (
@@ -1963,3 +1978,72 @@ def _check_unnecessary_dict_index_lookup(
                             node=subscript,
                             args=("1".join(value.as_string().rsplit("0", maxsplit=1)),),
                         )
+
+    def _check_unnecessary_list_index_lookup(
+        self, node: Union[nodes.For, nodes.Comprehension]
+    ) -> None:
+        if (
+            not isinstance(node.iter, nodes.Call)
+            or not isinstance(node.iter.func, nodes.Name)
+            or not node.iter.func.name == "enumerate"
+            or not isinstance(node.iter.args[0], nodes.Name)
+        ):
+            return
+
+        if not isinstance(node.target, nodes.Tuple) or len(node.target.elts) < 2:
+            # enumerate() result is being assigned without destructuring
+            return
+
+        iterating_object_name = node.iter.args[0].name
+        value_variable = node.target.elts[1]
+
+        children = (
+            node.body if isinstance(node, nodes.For) else node.parent.get_children()
+        )
+        for child in children:
+            for subscript in child.nodes_of_class(nodes.Subscript):
+                if isinstance(node, nodes.For) and (
+                    isinstance(subscript.parent, nodes.Assign)
+                    and subscript in subscript.parent.targets
+                    or isinstance(subscript.parent, nodes.AugAssign)
+                    and subscript == subscript.parent.target
+                ):
+                    # Ignore this subscript if it is the target of an assignment
+                    # Early termination; after reassignment index lookup will be necessary
+                    return
+
+                if isinstance(subscript.parent, nodes.Delete):
+                    # Ignore this subscript if it's used with the delete keyword
+                    return
+
+                index = subscript.slice
+                if isinstance(index, nodes.Name):
+                    if (
+                        index.name != node.target.elts[0].name
+                        or iterating_object_name != subscript.value.as_string()
+                    ):
+                        continue
+
+                    if (
+                        isinstance(node, nodes.For)
+                        and index.lookup(index.name)[1][-1].lineno > node.lineno
+                    ):
+                        # Ignore this subscript if it has been redefined after
+                        # the for loop.
+                        continue
+
+                    if (
+                        isinstance(node, nodes.For)
+                        and index.lookup(value_variable.name)[1][-1].lineno
+                        > node.lineno
+                    ):
+                        # The variable holding the value from iteration has been
+                        # 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,
+                    )

tests/functional/c/consider/consider_using_enumerate.py

@@ -1,6 +1,6 @@
 """Emit a message for iteration through range and len is encountered."""
 
-# pylint: disable=missing-docstring, import-error, useless-object-inheritance, unsubscriptable-object, too-few-public-methods
+# pylint: disable=missing-docstring, import-error, useless-object-inheritance, unsubscriptable-object, too-few-public-methods, unnecessary-list-index-lookup
 
 def bad():
     iterable = [1, 2, 3]

tests/functional/u/unnecessary/unnecessary_list_index_lookup.py

@@ -0,0 +1,46 @@
+"""Tests for unnecessary-list-index-lookup."""
+
+# pylint: disable=missing-docstring, too-few-public-methods, expression-not-assigned, line-too-long, unused-variable
+
+my_list = ['a', 'b']
+
+for idx, val in enumerate(my_list):
+    print(my_list[idx]) # [unnecessary-list-index-lookup]
+
+for idx, _ in enumerate(my_list):
+    print(my_list[0])
+    if idx > 0:
+        print(my_list[idx - 1])
+
+for idx, val in enumerate(my_list):
+    del my_list[idx]
+
+for idx, val in enumerate(my_list):
+    my_list[idx] = 42
+
+for vals in enumerate(my_list):
+    # This could be refactored, but too complex to infer
+    print(my_list[vals[0]])
+
+def process_list(data):
+    for index, value in enumerate(data):
+        index = 1
+        print(data[index])
+
+def process_list_again(data):
+    for index, value in enumerate(data):
+        value = 1
+        print(data[index]) # Can't use value here, it's been redefined
+
+other_list = [1, 2]
+for idx, val in enumerate(my_list):
+    print(other_list[idx])
+
+OTHER_INDEX = 0
+for idx, val in enumerate(my_list):
+    print(my_list[OTHER_INDEX])
+
+result = [val for idx, val in enumerate(my_list) if my_list[idx] == 'a'] # [unnecessary-list-index-lookup]
+result = [val for idx, val in enumerate(my_list) if idx > 0 and my_list[idx - 1] == 'a']
+result = [val for idx, val in enumerate(my_list) if other_list[idx] == 'a']
+result = [my_list[idx] for idx, val in enumerate(my_list)] # [unnecessary-list-index-lookup]

tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt

@@ -0,0 +1,3 @@
+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

tests/functional/u/unnecessary/unnecessary_list_index_lookup_py38.py

@@ -0,0 +1,9 @@
+"""Tests for unnecessary-list-index-lookup with assignment expressions."""
+
+# pylint: disable=missing-docstring, too-few-public-methods, expression-not-assigned, line-too-long, unused-variable
+
+my_list = ['a', 'b']
+
+for idx, val in enumerate(my_list):
+    if (val := 42) and my_list[idx] == 'b':
+        print(1)

tests/functional/u/unnecessary/unnecessary_list_index_lookup_py38.rc

@@ -0,0 +1,2 @@
+[testoptions]
+min_pyver=3.8