Add optional check redefined-loop-name (#5649)

* Also change message type for inner loops overwriting the outer loop variable from `redefined-outer-name` to `redefined-loop-name`

* Update redefined-outer-name description

* Add coverage and fix preexisting false positive

When the check for inner loops redefining outer loop variables was redefined-outer-name, it assumed enclosing for-loops were in the same namespace.

Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Daniël van Noord <[email protected]>

Author: 3 people

ChangeLog

@@ -181,6 +181,16 @@ Release date: TBA
 
   Closes #578
 
+* Added optional extension ``redefined-loop-name`` to emit messages when a loop variable
+  is redefined in the loop body.
+
+  Closes #5072
+
+* Changed message type from ``redefined-outer-name`` to ``redefined-loop-name``
+  (optional extension) for redefinitions of outer loop variables by inner loops.
+
+  Closes #5608
+
 * The ``ignore-mixin-members`` option has been deprecated. You should now use the new
   ``ignored-checks-for-mixins`` option.
 

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

@@ -0,0 +1,2 @@
+for name in names:
+    name = name.lower()  # [redefined-loop-name]

doc/data/messages/r/redefined-loop-name/good.py

@@ -0,0 +1,2 @@
+for name in names:
+    lowercased_name = name.lower()

doc/data/messages/r/redefined-loop-name/pylintrc

@@ -0,0 +1,2 @@
+[MASTER]
+load-plugins=pylint.extensions.redefined_loop_name,

doc/whatsnew/2.14.rst

@@ -46,6 +46,11 @@ New checkers
 
   Closes #4525
 
+* Added optional extension ``redefined-loop-name`` to emit messages when a loop variable
+  is redefined in the loop body.
+
+  Closes #5072
+
 * Added new message called ``duplicate-value`` which identifies duplicate values inside sets.
 
   Closes #5880
@@ -233,3 +238,8 @@ Other Changes
   attribute to itself without any prior assignment.
 
   Closes #1555
+
+* Changed message type from ``redefined-outer-name`` to ``redefined-loop-name``
+  (optional extension) for redefinitions of outer loop variables by inner loops.
+
+  Closes #5608

pylint/checkers/utils.py

@@ -1742,3 +1742,11 @@ def in_type_checking_block(node: nodes.NodeNG) -> bool:
         and ancestor.test.as_string() in TYPING_TYPE_CHECKS_GUARDS
         for ancestor in node.node_ancestors()
     )
+
+
+@lru_cache()
+def in_for_else_branch(parent: nodes.NodeNG, stmt: nodes.Statement) -> bool:
+    """Returns True if stmt is inside the else branch for a parent For stmt."""
+    return isinstance(parent, nodes.For) and any(
+        else_stmt.parent_of(stmt) or else_stmt == stmt for else_stmt in parent.orelse
+    )

pylint/checkers/variables.py

@@ -132,13 +132,6 @@ def _is_from_future_import(stmt, name):
     return None
 
 
-def in_for_else_branch(parent, stmt):
-    """Returns True if stmt in inside the else branch for a parent For stmt."""
-    return isinstance(parent, nodes.For) and any(
-        else_stmt.parent_of(stmt) or else_stmt == stmt for else_stmt in parent.orelse
-    )
-
-
 @lru_cache(maxsize=1000)
 def overridden_method(klass, name):
     """Get overridden method if any."""
@@ -448,7 +441,7 @@ def _has_locals_call_after_node(stmt, scope):
     "W0621": (
         "Redefining name %r from outer scope (line %s)",
         "redefined-outer-name",
-        "Used when a variable's name hides a name defined in the outer scope.",
+        "Used when a variable's name hides a name defined in an outer scope or except handler.",
     ),
     "W0622": (
         "Redefining built-in %r",
@@ -961,7 +954,7 @@ class VariablesChecker(BaseChecker):
     Checks for
     * unused variables / imports
     * undefined variables
-    * redefinition of variable from builtins or from an outer scope
+    * redefinition of variable from builtins or from an outer scope or except handler
     * use of variable before assignment
     * __all__ consistency
     * self/cls assignment
@@ -1062,7 +1055,6 @@ def __init__(self, linter=None):
         super().__init__(linter)
         self._to_consume: list[NamesConsumer] = []
         self._checking_mod_attr = None
-        self._loop_variables = []
         self._type_annotation_names = []
         self._except_handler_names_queue: list[
             tuple[nodes.ExceptHandler, nodes.AssignName]
@@ -1079,31 +1071,7 @@ def open(self) -> None:
             "undefined-loop-variable"
         )
 
-    @utils.only_required_for_messages("redefined-outer-name")
-    def visit_for(self, node: nodes.For) -> None:
-        assigned_to = [a.name for a in node.target.nodes_of_class(nodes.AssignName)]
-
-        # Only check variables that are used
-        dummy_rgx = self.linter.config.dummy_variables_rgx
-        assigned_to = [var for var in assigned_to if not dummy_rgx.match(var)]
-
-        for variable in assigned_to:
-            for outer_for, outer_variables in self._loop_variables:
-                if variable in outer_variables and not in_for_else_branch(
-                    outer_for, node
-                ):
-                    self.add_message(
-                        "redefined-outer-name",
-                        args=(variable, outer_for.fromlineno),
-                        node=node,
-                    )
-                    break
-
-        self._loop_variables.append((node, assigned_to))
-
-    @utils.only_required_for_messages("redefined-outer-name")
     def leave_for(self, node: nodes.For) -> None:
-        self._loop_variables.pop()
         self._store_type_annotation_names(node)
 
     def visit_module(self, node: nodes.Module) -> None:
@@ -2231,7 +2199,7 @@ def _loopvar_name(self, node: astroid.Name) -> None:
         for i, stmt in enumerate(astmts[1:]):
             if astmts[i].statement(future=True).parent_of(
                 stmt
-            ) and not in_for_else_branch(astmts[i].statement(future=True), stmt):
+            ) and not utils.in_for_else_branch(astmts[i].statement(future=True), stmt):
                 continue
             _astmts.append(stmt)
         astmts = _astmts

pylint/extensions/redefined_loop_name.py

@@ -0,0 +1,89 @@
+# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
+# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt
+
+"""Optional checker to warn when loop variables are overwritten in the loop's body."""
+
+from __future__ import annotations
+
+from astroid import nodes
+
+from pylint import checkers
+from pylint.checkers import utils
+from pylint.interfaces import HIGH
+from pylint.lint import PyLinter
+
+
+class RedefinedLoopName(checkers.BaseChecker):
+
+    name = "redefined-loop-name"
+
+    msgs = {
+        "W2901": (
+            "Redefining %r from loop (line %s)",
+            "redefined-loop-name",
+            "Used when a loop variable is overwritten in the loop body.",
+        ),
+    }
+
+    def __init__(self, linter: PyLinter) -> None:
+        super().__init__(linter)
+        self._loop_variables: list[
+            tuple[nodes.For, list[str], nodes.LocalsDictNodeNG]
+        ] = []
+
+    @utils.only_required_for_messages("redefined-loop-name")
+    def visit_assignname(self, node: nodes.AssignName) -> None:
+        assign_type = node.assign_type()
+        if not isinstance(assign_type, (nodes.Assign, nodes.AugAssign)):
+            return
+        node_scope = node.scope()
+        for outer_for, outer_variables, outer_for_scope in self._loop_variables:
+            if node_scope is not outer_for_scope:
+                continue
+            if node.name in outer_variables and not utils.in_for_else_branch(
+                outer_for, node
+            ):
+                self.add_message(
+                    "redefined-loop-name",
+                    args=(node.name, outer_for.fromlineno),
+                    node=node,
+                    confidence=HIGH,
+                )
+                break
+
+    @utils.only_required_for_messages("redefined-loop-name")
+    def visit_for(self, node: nodes.For) -> None:
+        assigned_to = [a.name for a in node.target.nodes_of_class(nodes.AssignName)]
+        # Only check variables that are used
+        assigned_to = [
+            var
+            for var in assigned_to
+            if not self.linter.config.dummy_variables_rgx.match(var)
+        ]
+
+        node_scope = node.scope()
+        for variable in assigned_to:
+            for outer_for, outer_variables, outer_for_scope in self._loop_variables:
+                if node_scope is not outer_for_scope:
+                    continue
+                if variable in outer_variables and not utils.in_for_else_branch(
+                    outer_for, node
+                ):
+                    self.add_message(
+                        "redefined-loop-name",
+                        args=(variable, outer_for.fromlineno),
+                        node=node,
+                        confidence=HIGH,
+                    )
+                    break
+
+        self._loop_variables.append((node, assigned_to, node.scope()))
+
+    @utils.only_required_for_messages("redefined-loop-name")
+    def leave_for(self, node: nodes.For) -> None:  # pylint: disable=unused-argument
+        self._loop_variables.pop()
+
+
+def register(linter: PyLinter) -> None:
+    linter.register_checker(RedefinedLoopName(linter))

tests/functional/c/cellvar_escaping_loop.py

@@ -72,7 +72,7 @@ def good_case9():
 
 
 def good_case10():
-    """Ignore when a loop variable is showdowed by an inner function"""
+    """Ignore when a loop variable is shadowed by an inner function"""
     lst = []
     for i in range(10):  # pylint: disable=unused-variable
         def func():

tests/functional/ext/redefined_loop_name/redefined_loop_name.py

@@ -0,0 +1,53 @@
+"""Tests for redefinitions of loop variables inside the loop body.
+
+See: https://github.com/PyCQA/pylint/issues/5608
+"""
+# pylint: disable=invalid-name
+
+lines = ["1\t", "2\t"]
+for line in lines:
+    line = line.strip()  # [redefined-loop-name]
+
+for i in range(8):
+    for j in range(8):
+        for i in range(j):  # [redefined-loop-name]
+            j = i  # [redefined-loop-name]
+            print(i, j)
+
+for i in range(8):
+    for j in range(8):
+        for k in range(j):
+            k = (i, j)  # [redefined-loop-name]
+            print(i, j, k)
+
+lines = [(1, "1\t"), (2, "2\t")]
+for i, line in lines:
+    line = line.strip()  # [redefined-loop-name]
+
+line = "no warning"
+
+for i in range(10):
+    i += 1  # [redefined-loop-name]
+
+
+def outer():
+    """No redefined-loop-name for variables in nested scopes"""
+    for i1 in range(5):
+        def inner():
+            """No warning, because i has a new scope"""
+            for i1 in range(3):
+                print(i1)
+        print(i1)
+        inner()
+
+
+def outer2():
+    """Similar, but with an assignment instead of homonymous loop variables"""
+    for i1 in range(5):
+        def inner():
+            """No warning, because i has a new scope"""
+            for _ in range(3):
+                i1 = 0
+                print(i1)
+        print(i1)
+        inner()

tests/functional/ext/redefined_loop_name/redefined_loop_name.rc

@@ -0,0 +1,2 @@
+[MASTER]
+load-plugins=pylint.extensions.redefined_loop_name,

tests/functional/ext/redefined_loop_name/redefined_loop_name.txt

@@ -0,0 +1,6 @@
+redefined-loop-name:9:4:9:8::Redefining 'line' from loop (line 8):HIGH
+redefined-loop-name:13:8:15:23::Redefining 'i' from loop (line 11):HIGH
+redefined-loop-name:14:12:14:13::Redefining 'j' from loop (line 12):HIGH
+redefined-loop-name:20:12:20:13::Redefining 'k' from loop (line 19):HIGH
+redefined-loop-name:25:4:25:8::Redefining 'line' from loop (line 24):HIGH
+redefined-loop-name:30:4:30:5::Redefining 'i' from loop (line 29):HIGH

tests/functional/r/reused_outer_loop_variable.py renamed to tests/functional/ext/redefined_loop_name/reused_outer_loop_variable.py

@@ -4,22 +4,22 @@
 
 # Simple nested loop
 for i in range(10):
-    for i in range(10): #[redefined-outer-name]
+    for i in range(10): #[redefined-loop-name]
         print(i)
 
 # When outer loop unpacks a tuple
 for i, i_again in enumerate(range(10)):
-    for i in range(10): #[redefined-outer-name]
+    for i in range(10): #[redefined-loop-name]
         print(i, i_again)
 
 # When inner loop unpacks a tuple
 for i in range(10):
-    for i, i_again in range(10): #[redefined-outer-name]
+    for i, i_again in range(10): #[redefined-loop-name]
         print(i, i_again)
 
 # With nested tuple unpacks
 for (a, (b, c)) in [(1, (2, 3))]:
-    for i, a in range(10): #[redefined-outer-name]
+    for i, a in range(10): #[redefined-loop-name]
         print(i, a, b, c)
 
 # Ignores when in else
@@ -35,3 +35,8 @@
 for _ in range(10):
     for _ in range(10):
         print("Hello")
+
+# Unpacking
+for i, *j in [(1, 2, 3, 4)]:
+    for j in range(i):  # [redefined-loop-name]
+        print(j)

tests/functional/ext/redefined_loop_name/reused_outer_loop_variable.rc

@@ -0,0 +1,2 @@
+[MASTER]
+load-plugins=pylint.extensions.redefined_loop_name,

tests/functional/ext/redefined_loop_name/reused_outer_loop_variable.txt

@@ -0,0 +1,5 @@
+redefined-loop-name:7:4:8:16::Redefining 'i' from loop (line 6):HIGH
+redefined-loop-name:12:4:13:25::Redefining 'i' from loop (line 11):HIGH
+redefined-loop-name:17:4:18:25::Redefining 'i' from loop (line 16):HIGH
+redefined-loop-name:22:4:23:25::Redefining 'a' from loop (line 21):HIGH
+redefined-loop-name:41:4:42:16::Redefining 'j' from loop (line 40):HIGH

tests/functional/r/redefine_loop.py

@@ -1,7 +1,8 @@
-"""Test case for variable redefined in inner loop."""
+"""No message is emitted for variables overwritten in inner loops by default.
+See redefined-loop-name optional checker."""
 for item in range(0, 5):
     print("hello")
-    for item in range(5, 10): #[redefined-outer-name]
+    for item in range(5, 10):
         print(item)
         print("yay")
     print(item)

tests/functional/r/redefine_loop.txt

@@ -9,7 +9,7 @@ def my_function():
                 while True:
                     try:
                         if True:
-                            i += 1
+                            print(i)
                     except IOError:
                         pass
 
@@ -18,7 +18,7 @@ def my_function():
             if i == 2:
                 while True:
                     try:
-                        i += 1
+                        print(i)
                     except IOError:
                         pass