Extending redefined-outer-name check to cover the case of loop variables

.github/workflows/primer_comment.yaml

@@ -31,6 +31,8 @@ jobs:
     steps:
       - name: Check out code from GitHub
         uses: actions/[email protected]
+        with:
+          ref: extending-redefined-outer-name-to-loop-variables
       - name: Set up Python ${{ env.DEFAULT_PYTHON }}
         id: python
         uses: actions/[email protected]

doc/data/messages/r/redefined-outer-name/bad.py

@@ -4,3 +4,7 @@
 def count_it(count):  # [redefined-outer-name]
     for i in range(count):
         print(i)
+
+
+for count in range(10):  # [redefined-outer-name]
+    print(count)

doc/whatsnew/fragments/8646.new_check

@@ -0,0 +1,4 @@
+The rule ``redefined-outer-name`` now checks for loop variables
+shadowing variables defined before in the local scope.
+
+Closes #8646

pylint/checkers/variables.py

@@ -1295,15 +1295,53 @@ def __init__(self, linter: PyLinter) -> None:
         ] = {}
         self._postponed_evaluation_enabled = False
 
+    @cached_property
+    def _dummy_rgx(self) -> re.Pattern[str]:
+        return self.linter.config.dummy_variables_rgx  # type: ignore[no-any-return]
+
     @utils.only_required_for_messages(
+        "redefined-outer-name",
         "unbalanced-dict-unpacking",
     )
     def visit_for(self, node: nodes.For) -> None:
+        targets = (
+            node.target.elts if isinstance(node.target, nodes.Tuple) else [node.target]
+        )
+        # This loop check for potential redefined-outer-name
+        # The test covering this is redefined_loop_variable.py
+        for target in targets:
+            if isinstance(target, nodes.Starred):
+                target = target.value
+            for ref in node.scope().locals.get(target.name, []):
+                if ref == target:
+                    continue
+                if isinstance(ref.parent, nodes.Arguments):
+                    continue  # ignoring case covered by redefined-argument-from-local
+                if self._dummy_rgx and self._dummy_rgx.match(target.name):
+                    continue  # no message for variables matching dummy-variables-rgx
+                if _is_variable_defined_in_ancestor_loop_assignment(ref, target.name):
+                    continue
+                if not isinstance(ref, nodes.Arguments):
+                    # if ref is an instance of Arguments, we have a definitive match,
+                    # the following checks are not needed:
+                    if _is_assignment_performed_after(ref, node):
+                        continue
+                    if (
+                        isinstance(ref.parent, nodes.AnnAssign)
+                        and ref.parent.value is None
+                    ):
+                        continue  # ignoring type-only definitions of variables
+                self.add_message(
+                    "redefined-outer-name",
+                    args=(target.name, ref.lineno),
+                    node=node,
+                    confidence=HIGH,
+                )
+                break  # no need to check other refs once a message has been emitted
+
         if not isinstance(node.target, nodes.Tuple):
             return
 
-        targets = node.target.elts
-
         inferred = utils.safe_infer(node.iter)
         if not isinstance(inferred, DICT_TYPES):
             return
@@ -3322,5 +3360,50 @@ def visit_const(self, node: nodes.Const) -> None:
             pass
 
 
+def _is_variable_defined_in_ancestor_loop_assignment(
+    node: nodes.NodeNG, name: str
+) -> bool:
+    for for_node in node.node_ancestors():
+        if not isinstance(for_node, nodes.For):
+            continue
+        targets = (
+            for_node.target.elts
+            if isinstance(for_node.target, nodes.Tuple)
+            else [for_node.target]
+        )
+        if _contains_assignment_matching_name(targets, name):
+            return True
+    return False
+
+
+def _contains_assignment_matching_name(targets: tuple[nodes.NodeNG], name: str) -> bool:
+    for target in targets:
+        if isinstance(target, nodes.Tuple):
+            # E.g. with SECOND in test redeclared_assigned_name.py
+            if _contains_assignment_matching_name(target.elts, name):
+                return True
+        else:
+            if isinstance(target, nodes.Starred):
+                target = target.value
+            # assert(isinstance(target, nodes.AssignName)), str(type(target))
+            if target.name == name:
+                return True
+    return False
+
+
+def _is_assignment_performed_after(
+    assign: nodes.AssignName, for_node: nodes.For
+) -> bool:
+    """
+    This function check if the assignment was made
+    in the same function/module as the loop, but after it.
+    """
+    # assert(isinstance(assign, nodes.AssignName)), str(type(assign))
+    for node in assign.node_ancestors():
+        if node is for_node.parent:
+            return assign.lineno > for_node.lineno  # type: ignore[no-any-return]
+    return False
+
+
 def register(linter: PyLinter) -> None:
     linter.register_checker(VariablesChecker(linter))

pylint/testutils/_primer/primer_compare_command.py

@@ -49,6 +49,8 @@ def _cross_reference(
     ) -> tuple[PackageMessages, PackageMessages]:
         missing_messages_data: PackageMessages = {}
         for package, data in main_data.items():
+            if package not in pr_data:
+                continue
             package_missing_messages: list[OldJsonExport] = []
             for message in data["messages"]:
                 try:

pylintrc

@@ -109,6 +109,7 @@ disable=
     # We anticipate #3512 where it will become optional
     fixme,
     consider-using-assignment-expr,
+    redefined-outer-name,
 
 
 [REPORTS]

tests/functional/c/cellvar_escaping_loop.py

@@ -1,4 +1,4 @@
-# pylint: disable=unnecessary-comprehension,missing-docstring,too-few-public-methods,unnecessary-direct-lambda-call
+# pylint: disable=unnecessary-comprehension,missing-docstring,too-few-public-methods,unnecessary-direct-lambda-call,redefined-outer-name
 """Tests for loopvar-in-closure."""
 
 

tests/functional/r/redeclared_assigned_name.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring
+# pylint: disable=missing-docstring,redefined-outer-name
 
 FIRST, FIRST = (1, 2)  # [redeclared-assigned-name]
 

tests/functional/r/redefined/redefined_loop_variable.py

@@ -0,0 +1,81 @@
+"redefined-outer-name with loop variables - cf. issue #8646 for context"
+# pylint: disable=global-statement,invalid-name,missing-function-docstring
+from collections import defaultdict
+errors = [
+    Exception("E101", "Boom!"),
+    Exception("E101", "Bam!"),
+    Exception("E102", "Shazam!"),
+]
+errors_per_arg0 = defaultdict(list)
+for error in errors:
+    errors_per_arg0[error.args[0]].append(error)
+for arg0, errors in errors_per_arg0.items():  # [redefined-outer-name]
+    print(arg0, "count:", len(errors))
+
+pair = 2
+for pair in errors_per_arg0.items():  # [redefined-outer-name]
+    print(pair)
+
+words = (
+    ("A", ("a", "abandon", "ability", ...)),
+    ("B", ("ball", "banana", ...)),
+)
+for letter, *words in words:  # [redefined-outer-name]
+    print(letter, words)
+
+def func0(*args, **kwargs):
+    print(args, kwargs)
+    for args in range(10):  # [redefined-outer-name]
+        pass
+    for _, kwargs in {}.items():  # [redefined-outer-name]
+        pass
+
+
+#--- Notable cases where redefined-outer-name should NOT be raised:
+
+# When a dummy variable is used:
+_ = 42
+for _ in range(10):
+    pass
+
+# When loop variables are re-assigned:
+for i, err in enumerate(errors):
+    i += 1
+    err = None
+
+# When the same loop variables are used consecutively:
+for k, v in errors_per_arg0.items():
+    print(k, v)
+for k, v in errors_per_arg0.items():
+    print(k, v)
+
+# When a similarly named loop variable is assigned in another loop:
+for n in range(10):
+    n += 1
+for n in range(10):
+    n += 1
+
+# When the loop variable is re-assigned AFTER the for loop:
+def func1():
+    for value in range(10):
+        print(value)
+    if 1 + 1:
+        value = 42
+
+# When the variable is global or nonlocal:
+glob = 42
+def func2():
+    global glob
+    for glob in range(10):
+        pass
+
+# When the variable was just type-declared before the loop:
+var: int
+for var in (1, 2):
+    print(var)
+
+# Function argument override, already covered by redefined-argument-from-local:
+def func3(arg):
+    print(arg)
+    for arg in range(10):  # [redefined-argument-from-local]
+        pass

tests/functional/r/redefined/redefined_loop_variable.txt

@@ -0,0 +1,6 @@
+redefined-outer-name:12:0:13:38::Redefining name 'errors' from outer scope (line 4):HIGH
+redefined-outer-name:16:0:17:15::Redefining name 'pair' from outer scope (line 15):HIGH
+redefined-outer-name:23:0:24:24::Redefining name 'words' from outer scope (line 19):HIGH
+redefined-outer-name:28:4:29:12:func0:Redefining name 'args' from outer scope (line None):HIGH
+redefined-outer-name:30:4:31:12:func0:Redefining name 'kwargs' from outer scope (line None):HIGH
+redefined-argument-from-local:80:8:80:11:func3:Redefining argument with the local name 'arg':UNDEFINED

tests/functional/s/statement_without_effect.py

@@ -1,5 +1,5 @@
 """Test for statements without effects."""
-# pylint: disable=too-few-public-methods, unnecessary-comprehension, unnecessary-ellipsis, use-list-literal
+# pylint: disable=redefined-outer-name, too-few-public-methods, unnecessary-comprehension, unnecessary-ellipsis, use-list-literal
 
 # +1:[pointless-string-statement]
 """inline doc string should use a separated message"""

tests/functional/u/unnecessary/unnecessary_list_index_lookup.py

@@ -61,7 +61,7 @@ def process_list_again(data):
 
 num_list = [1, 2, 3]
 for a, b in enumerate(num_list):
-    ([x, num_list[a]], _) = ([5, 6], 1)
+    ([n, num_list[a]], _) = ([5, 6], 1)
 
 # Regression test for https://github.com/pylint-dev/pylint/issues/6818
 updated_list = [1, 2, 3]

tests/functional/u/unused/unused_variable.py

@@ -161,7 +161,7 @@ def main(lst):
     except ValueError as e:  # [unused-variable]
         pass
 
-    for e in lst:
+    for e in lst:  # [redefined-outer-name]
         pass
 
     # e will be undefined if lst is empty

tests/functional/u/unused/unused_variable.txt

@@ -25,5 +25,6 @@ unused-variable:144:8:145:28:func3:Unused variable 'error':UNDEFINED
 unused-variable:150:4:154:26:func4:Unused variable 'error':UNDEFINED
 redefined-outer-name:153:8:154:26:func4:Redefining name 'error' from outer scope (line 150):UNDEFINED
 unused-variable:161:4:162:12:main:Unused variable 'e':UNDEFINED
+redefined-outer-name:164:4:165:12:main:Redefining name 'e' from outer scope (line 161):HIGH
 undefined-loop-variable:168:10:168:11:main:Using possibly undefined loop variable 'e':UNDEFINED
 unused-variable:217:8:218:16:test_regression_8595:Unused variable 'e':UNDEFINED

tests/primer/packages_to_prime.json

@@ -20,11 +20,6 @@
     "directories": ["src/flask"],
     "url": "https://github.com/pallets/flask"
   },
-  "home-assistant": {
-    "branch": "dev",
-    "directories": ["homeassistant"],
-    "url": "https://github.com/home-assistant/core"
-  },
   "music21": {
     "branch": "master",
     "directories": ["music21"],