Proposed check: reuse of loop variable in nested loop

[matthewlloyd]

I was just bitten by a bug in production where I accidentally reused an outer loop variable in a nested loop. Example:

# Outer loop.
for i in range(10):
    # Enough code here to hide the outermost loop off screen.
    # ...

    # Developer does not notice "i" is already being used as a loop variable
    # and accidentally overwrites it here. The type is the same, so type checkers
    # don't complain.
    for i in range(10):
        print(f"inner loop {i=}")

    # Enough code here to hide the innermost loop off screen.
    # ...

    # Developer expects "i" to still hold the value of the outer loop.
    # Instead, the value is always 9.
    print(f"outer loop {i=}")

Proposed warning: R???: "Variable 'i' is already declared in 'for' loop or 'with' statement above"

Credit for the wording: this is one of PyCharm's built-in inspections.

Note that although I am personally using only Ruff, I have opened a similar enhancement request in the flake8-bugbear repo here: PyCQA/flake8-bugbear#360.

[matthewlloyd]

I have a working draft of this and will submit a PR in the next few days...

[charliermarsh]

Sounds good. This looks reasonable to me, especially if it mirrors an existing PyCharm inspection. If bugbear adds it, we can use their rule code; otherwise, we can put it in RUF.

[matthewlloyd]

Great!

We may even want to create a new category for PyCharm inspections - I looked through them all, filtered them by suitability for implementation in a linter with no type awareness, then filtered out those for which Ruff already had counterparts, and was left with no fewer than 19 total including this one:

• Assignments to 'for' loop or 'with' statement parameter (this)
• Assignment can be replaced with augmented assignment (a = a + b becomes a += b; arguably less useful)
• Class-specific decorator is used outside the class (@classmethod or @staticmethod)
• Dictionary contains duplicate keys ({'a': 1, 'a': 2})
• First argument of the method is reassigned (e.g. reassignment to self)
• Incorrect property definition (e.g. getter that does not return or yield a value)
• __init__ method that returns a value
• Invalid definition of 'typing.NamedTuple' (field with default followed by one or more fields without)
• Method is not declared static (but does not use self)
• Missed call to '__init__' of the super class
• Non-optimal list declaration (l = [1], l.append(2) becomes l = [1, 2]; arguably less useful)
• Old-style class contains new-style class features (__slots__, __getattribute__, super)
• Redeclared names without usages (def x(): pass, followed later by x = 2, no uses in between)
• Redundant boolean variable check (if b == True)
• Redundant parentheses
• Shadowing names from outer scopes
• Statement has no effect (e.g. comprises solely a variable name)
• Too complex chained comparisons ((x >= 0 and x <= 1) becomes (0 <= x <= 1)).
• Unnecessary backslash - backslashes in places where line continuation is implicit inside (), [], and {}.

[matthewlloyd]

I have this tested, with snapshots, and ready to review (thanks to your excellent contribution documentation!). Just need a decision from you on whether to put this under RUF or a create new category for PyCharm (PC? PYC? CH? CHA? CHM?).

[sbrugman]

Great work @matthewlloyd! ruff should definitely incorporate the PyCharm inspections (which I suspect are inspired by pylint).

Regarding to where they should be placed, it depends on the rule.
Some of the checks are already there (e.g. Statement has no effect => B018) or are part of pylint but not yet implemented (could be a reason to prioritize implementing them, PyCharm has autofix for many of them). Any remaining could probably be placed under RUF.

See also: pylint-dev/pylint#5608

Pylint

• redefined-outer-name / W0621 (this)
• Assignment can be replaced with augmented assignment => init method that returns a value => return-in-init, PLE0101)
• consider-using-augmented-assignment PLR6104
• Duplicate dictionary key: PLW0109
• bad-staticmethod-argument / W0211: Method is not declared static (but does not use self)

Let's see if we can find corresponding rules to the remaining ones in the overview of rules in pylint.

[matthewlloyd]

Sounds reasonable to me! I wasn't aware of #970, and agree there is a lot of overlap. I went through all the PyCharm inspections in my list above and categorized them by whether they have PyLint counterparts, mostly by running PyLint directly on the examples provided in the PyCharm Settings panel.

The inspection in this issue is indeed partially covered by PyLint's new PLW2901 redefined-loop-name inspection, though it does not seem to be detected at all by PLW0621 redefined-outer-name. However, the semantics are slightly different between the PyCharm and PyLint versions:

• PyLint's is more general in detecting any nested assignment to a loop variable, not just as the target of another loop statement.
• PyLint's is slightly more specific in working only for loop variables, whereas PyCharm's also examines with statements.

Given that, it might make sense to add this as PLW2901, and either roll in with statements (my vote), or pull out the with variant into a separate RUF. I'll need to extend my code a little to detect all nested assignments.

Already covered by PyLint

• Assignments to 'for' loop or 'with' statement parameter (see above)
  • PLW2901 redefined-loop-name
• Assignment can be replaced with augmented assignment (a = a + b becomes a += b; arguably less useful)
  • PLR6104 consider-using-augmented-assign
• Dictionary contains duplicate keys ({'a': 1, 'a': 2})
  • PLW0109 duplicate-key
• First argument of the method is reassigned (e.g. reassignment to self)
  • PLW0642 self-cls-assignment
• init method that returns a value
  • PLE101 return-in-init
• Method is not declared static (but does not use self)
  • PLR6301 no-self-use
  • PLW0211 bad-staticmethod-argument (not quite the same thing)
• Missed call to 'init' of the super class
  • PLW0231 super-init-not-called
• Redundant boolean variable check (if b == True)
  • PLC0121 singleton-comparison
• Statement has no effect (e.g. comprises solely a variable name)
  • PLW0104 pointless-statement
  • also B018 as above
• Too complex chained comparisons ((x >= 0 and x <= 1) becomes (0 <= x <= 1)).
  • PLR1716 chained-comparison
• Unnecessary backslash - backslashes in places where line continuation is implicit inside (), [], and {}.
  • PyLint does not detect this, but pycodestyle has E502: "The backslash is redundant between brackets" (not yet implemented in Ruff)

PyCharm inspections not covered by PyLint in my tests

(note: this list includes only those I deemed implementable in a non-type-aware linter)

• Class-specific decorator is used outside the class (@classmethod or @staticmethod)

  @staticmethod
  def a(): pass

• Incorrect property definition (e.g. getter that does not return or yield a value)

  class A:
      @property
      def field(): pass

• Invalid definition of 'typing.NamedTuple' (field with default followed by one or more fields without)

  import typing
  
  class FullName(typing.NamedTuple):
      first: str
      last: str = ""
      middle: str

• Non-optimal list declaration (l = [1], l.append(2) becomes l = [1, 2]; arguably less useful)

  l = [1]
  l.append(2)

• Redeclared names without usages (def x(): pass, followed later by x = 2, no uses in between)

  def x(): pass
  x = 2

• Redundant parentheses

  a = (((3)))

• Shadowing names from outer scopes

  def outer(p):
    def inner(p):
        pass

The rule "Old-style class contains new-style class features (slots, getattribute, super)" is no longer relevant since Python 3.