[pylint] redefined-loop-name (W2901)

[matthewlloyd]

Slightly broadens W2901 to cover with statements too.

Closes #2972.

[sbrugman]

Perhaps update the pylint issue with your findings, quite some nice insights (since it will be closed when merging this)

[matthewlloyd]

Perhaps update the pylint issue with your findings, quite some nice insights (since it will be closed when merging this)

Done, and opened a new issue with the potential PyCharm-derived inspections: #3040

[charliermarsh]

(Just out of interest, how closely does this match Pylint's own implementation? Did you look at the Pylint source, or is it implemented from scratch?)

[matthewlloyd]

It was implemented from scratch, but after I learned Pylint has it too, I did look at the Pylint source later to compare. Pylint has a slightly more efficient algorithm, using a single recursion over the AST, keeping track of defined names using a stack, and pushing and popping the names on entering and leaving each for loop. This could probably be done with a deeper integration into ast.rs.

[charliermarsh]

So I just ran this over Zulip as a smoke test, and it looks like the rule is currently flagging usages like this:

for email in verified_emails:
    existing_account = common_get_active_user(email, realm, {})
    if existing_account is not None:
        existing_account_emails.append(email)
        avatars[email] = avatar_url(existing_account)

Also attribute assignments, like scope.fingerprint = ["worker-timeout", self.queue_name].

[matthewlloyd]

Oh gosh, great point! I will know to run on some larger codebases when testing in future. Agreed, we need to avoid the lexer and traverse the AST of the target. It's really interesting learning new intricacies of Python syntax. I had never considered these, for example:

>>> d = {}
>>> for d["i"] in range(2):
...     print(d["i"])
...
0
1
>>> a = [None]
>>> for a[0] in range(2):
...     print(a[0])
...
0
1
>>> i = 0
>>> for a[i] in range(2):
...     print(a[i])
...     i = 0  # no error
...
0
1

I'll try to think through all these edge cases, add tests, and get this fixed later today or tomorrow.

[charliermarsh]

No rush! Sorry for not noticing this earlier :) Let me know if you have questions as you work through it.

[matthewlloyd]

Ran this new version on Zulip - still quite a few "false positives", only a few of which are IMO bad code smells. The most noisy false positives tend to be of the form:

for line in file:
    line = line.strip()  # PLW2901

which are flagged by Pylint's redefined-loop-name extension and by my code here, but which in practice are of course commonplace and harmless, and where the developer has to jump through an annoying hoop to avoid the warning e.g. using a new stripped_line name. I'm in two minds about whether these should always be flagged, or whether reuse by direct assignment should be optional or under a different code.

On the other extreme we have this (from queue_processors.py) which does seem worth flagging, since another developer may not remember Python's unique scope rules and might add code that uses the outer scope variable without realizing it has been overwritten:

        with configure_scope() as scope:
            scope.set_context(
              # ...
            )
            if isinstance(exception, WorkerTimeoutError):
                with sentry_sdk.push_scope() as scope:  # PLW2901
                    scope.fingerprint = ["worker-timeout", self.queue_name]
                    logging.exception(exception, stack_info=True)
            else:
                logging.exception(
                    "Problem handling data on queue %s", self.queue_name, stack_info=True
                )

Similarly we have cases like this one (from test_openapi.py) where the code works and is correct, but again another developer may add code to the outer loop expecting element to still be from diff and get tripped up:

        for element in diff:
            vname = element[0]
            for element in openapi_params:  # PLW2901
                if element[0] == vname:
                    opvtype = element[1]
                    break
            for element in function_params:  # PLW2901
                if element[0] == vname:
                    fdvtype = element[1]
                    break

Finally we have an example in between (from rocketchat.py), where the name reuse is gratuitous, and while not an actual type error, arguably a notional error in regards to the "semantic type" of reaction_dict (by which I mean what keys are present and what the associated values represent). A developer adding code to the outer loop seems unlikely get tripped up here, but I'd say it's still worth flagging since finding a new name improves readability:

    for reaction_dict in reactions:
        emoji_name = reaction_dict["name"]
        user_id = reaction_dict["user_id"]
        # Check in realm emoji
        if emoji_name in realmemoji:
          # ...

        reaction_id = NEXT_ID("reaction")
        reaction = Reaction(
          # ...
        )

        reaction_dict = model_to_dict(reaction, exclude=["message", "user_profile"])  # PLW2901
        reaction_dict["message"] = message_id
        reaction_dict["user_profile"] = user_id
        total_reactions.append(reaction_dict)

[charliermarsh]

Awesome! Will review this shortly.

[charliermarsh]

I agree with the analysis in your comment. One other lens to apply here: where does this version deviate from Pylint, and where is it the same? (The less it deviates, the less strongly I feel about things that might feel excessive in practice.) (I know you mention that Pylint flags the first example -- wondering if it's the case for the others too.)

[matthewlloyd]

I agree with the analysis in your comment. One other lens to apply here: where does this version deviate from Pylint, and where is it the same? (The less it deviates, the less strongly I feel about things that might feel excessive in practice.) (I know you mention that Pylint flags the first example -- wondering if it's the case for the others too.)

Good question. As the code currently stands, for the Zulip codebase, this version exactly matches Pylint with the only difference being this version works not just for for but also for with statements.

This version finds 35 (34 for + 1 with):

zulip/manage.py:90:21: PLW2901 Outer for loop variable `app` overwritten by inner assignment target with same name
zulip/manage.py:92:21: PLW2901 Outer for loop variable `app` overwritten by inner assignment target with same name
zulip/scripts/lib/check_rabbitmq_queue.py:142:9: PLW2901 Outer for loop variable `line` overwritten by inner assignment target with same name
zulip/scripts/lib/setup_venv.py:110:13: PLW2901 Outer for loop variable `package` overwritten by inner assignment target with same name
zulip/scripts/lib/setup_venv.py:114:17: PLW2901 Outer for loop variable `package` overwritten by inner assignment target with same name
zulip/scripts/lib/setup_venv.py:116:9: PLW2901 Outer for loop variable `package` overwritten by inner assignment target with same name
zulip/scripts/lib/zulip_tools.py:429:13: PLW2901 Outer for loop variable `line` overwritten by inner assignment target with same name
zulip/tools/lib/capitalization.py:235:9: PLW2901 Outer for loop variable `text` overwritten by inner assignment target with same name
zulip/tools/lib/html_branches.py:46:17: PLW2901 Outer for loop variable `lst` overwritten by inner assignment target with same name
zulip/tools/lib/test_script.py:99:9: PLW2901 Outer for loop variable `file` overwritten by inner assignment target with same name
zulip/zerver/data_import/rocketchat.py:363:9: PLW2901 Outer for loop variable `reaction_dict` overwritten by inner assignment target with same name
zulip/zerver/lib/ccache.py:112:13: PLW2901 Outer for loop variable `tlv` overwritten by inner assignment target with same name
zulip/zerver/lib/events.py:1084:25: PLW2901 Outer for loop variable `sub` overwritten by inner assignment target with same name
zulip/zerver/lib/markdown/__init__.py:1273:21: PLW2901 Outer for loop variable `found_url` overwritten by inner assignment target with same name
zulip/zerver/lib/narrow.py:643:17: PLW2901 Outer for loop variable `term` overwritten by inner assignment target with same name
zulip/zerver/lib/narrow.py:644:17: PLW2901 Outer for loop variable `term` overwritten by inner assignment target with same name
zulip/zerver/lib/test_runner.py:335:21: PLW2901 Outer for loop variable `test_name` overwritten by inner assignment target with same name
zulip/zerver/lib/test_runner.py:348:29: PLW2901 Outer for loop variable `test_name` overwritten by inner assignment target with same name
zulip/zerver/management/commands/enqueue_file.py:17:9: PLW2901 Outer for loop variable `line` overwritten by inner assignment target with same name
zulip/zerver/management/commands/import.py:88:13: PLW2901 Outer for loop variable `path` overwritten by inner assignment target with same name
zulip/zerver/management/commands/makemessages.py:168:17: PLW2901 Outer for loop variable `match` overwritten by inner assignment target with same name
zulip/zerver/management/commands/makemessages.py:169:17: PLW2901 Outer for loop variable `match` overwritten by inner assignment target with same name
zulip/zerver/migrations/0102_convert_muted_topic.py:32:13: PLW2901 Outer for loop variable `stream_name` overwritten by inner assignment target with same name
zulip/zerver/migrations/0102_convert_muted_topic.py:53:13: PLW2901 Outer for loop variable `stream_name` overwritten by inner assignment target with same name
zulip/zerver/migrations/0423_fix_email_gateway_attachment_owner.py:113:13: PLW2901 Outer for loop variable `attachment` overwritten by inner assignment target with same name
zulip/zerver/openapi/openapi.py:468:17: PLW2901 Outer for loop variable `error` overwritten by inner assignment target with same name
zulip/zerver/openapi/test_curl_examples.py:86:17: PLW2901 Outer for loop variable `curl_command_text` overwritten by inner assignment target with same name
zulip/zerver/tests/test_decorators.py:2036:17: PLW2901 Outer for loop variable `line` overwritten by inner assignment target with same name
zulip/zerver/tests/test_openapi.py:376:17: PLW2901 Outer for loop variable `element` overwritten by inner for loop target with same name
zulip/zerver/tests/test_openapi.py:380:17: PLW2901 Outer for loop variable `element` overwritten by inner for loop target with same name
zulip/zerver/tests/test_openapi.py:439:13: PLW2901 Outer for loop variable `defval` overwritten by inner assignment target with same name
zulip/zerver/views/invite.py:104:13: PLW2901 Outer for loop variable `email` overwritten by inner assignment target with same name
zulip/zerver/webhooks/bitbucket2/view.py:437:13: PLW2901 Outer for loop variable `change` overwritten by inner assignment target with same name
zulip/zerver/worker/queue_processors.py:378:49: PLW2901 Outer with statement variable `scope` overwritten by inner with statement target with same name

Pylint finds 34 (34 for):

zulip/manage.py:90:20: W2901: Redefining 'app' from loop (line 88) (redefined-loop-name)
zulip/manage.py:92:20: W2901: Redefining 'app' from loop (line 88) (redefined-loop-name)
zulip/scripts/lib/check_rabbitmq_queue.py:142:8: W2901: Redefining 'line' from loop (line 141) (redefined-loop-name)
zulip/scripts/lib/setup_venv.py:110:12: W2901: Redefining 'package' from loop (line 104) (redefined-loop-name)
zulip/scripts/lib/setup_venv.py:114:16: W2901: Redefining 'package' from loop (line 104) (redefined-loop-name)
zulip/scripts/lib/setup_venv.py:116:8: W2901: Redefining 'package' from loop (line 104) (redefined-loop-name)
zulip/scripts/lib/zulip_tools.py:429:12: W2901: Redefining 'line' from loop (line 428) (redefined-loop-name)
zulip/tools/lib/capitalization.py:235:8: W2901: Redefining 'text' from loop (line 234) (redefined-loop-name)
zulip/tools/lib/html_branches.py:46:16: W2901: Redefining 'lst' from loop (line 42) (redefined-loop-name)
zulip/tools/lib/test_script.py:99:8: W2901: Redefining 'file' from loop (line 98) (redefined-loop-name)
zulip/zerver/data_import/rocketchat.py:363:8: W2901: Redefining 'reaction_dict' from loop (line 341) (redefined-loop-name)
zulip/zerver/lib/ccache.py:112:12: W2901: Redefining 'tlv' from loop (line 106) (redefined-loop-name)
zulip/zerver/lib/events.py:1084:24: W2901: Redefining 'sub' from loop (line 1081) (redefined-loop-name)
zulip/zerver/lib/markdown/__init__.py:1273:20: W2901: Redefining 'found_url' from loop (line 1244) (redefined-loop-name)
zulip/zerver/lib/narrow.py:643:16: W2901: Redefining 'term' from loop (line 641) (redefined-loop-name)
zulip/zerver/lib/narrow.py:644:16: W2901: Redefining 'term' from loop (line 641) (redefined-loop-name)
zulip/zerver/lib/test_runner.py:335:20: W2901: Redefining 'test_name' from loop (line 332) (redefined-loop-name)
zulip/zerver/lib/test_runner.py:348:28: W2901: Redefining 'test_name' from loop (line 332) (redefined-loop-name)
zulip/zerver/management/commands/enqueue_file.py:17:8: W2901: Redefining 'line' from loop (line 16) (redefined-loop-name)
zulip/zerver/management/commands/import.py:88:12: W2901: Redefining 'path' from loop (line 87) (redefined-loop-name)
zulip/zerver/management/commands/makemessages.py:168:16: W2901: Redefining 'match' from loop (line 167) (redefined-loop-name)
zulip/zerver/management/commands/makemessages.py:169:16: W2901: Redefining 'match' from loop (line 167) (redefined-loop-name)
zulip/zerver/migrations/0102_convert_muted_topic.py:32:12: W2901: Redefining 'stream_name' from loop (line 31) (redefined-loop-name)
zulip/zerver/migrations/0102_convert_muted_topic.py:53:12: W2901: Redefining 'stream_name' from loop (line 52) (redefined-loop-name)
zulip/zerver/migrations/0423_fix_email_gateway_attachment_owner.py:113:12: W2901: Redefining 'attachment' from loop (line 42) (redefined-loop-name)
zulip/zerver/openapi/openapi.py:468:16: W2901: Redefining 'error' from loop (line 447) (redefined-loop-name)
zulip/zerver/openapi/test_curl_examples.py:86:16: W2901: Redefining 'curl_command_text' from loop (line 85) (redefined-loop-name)
zulip/zerver/tests/test_decorators.py:2036:16: W2901: Redefining 'line' from loop (line 2035) (redefined-loop-name)
zulip/zerver/tests/test_openapi.py:376:12: W2901: Redefining 'element' from loop (line 374) (redefined-loop-name)
zulip/zerver/tests/test_openapi.py:380:12: W2901: Redefining 'element' from loop (line 374) (redefined-loop-name)
zulip/zerver/tests/test_openapi.py:439:12: W2901: Redefining 'defval' from loop (line 438) (redefined-loop-name)
zulip/zerver/views/invite.py:104:12: W2901: Redefining 'email' from loop (line 101) (redefined-loop-name)
zulip/zerver/webhooks/bitbucket2/view.py:437:12: W2901: Redefining 'change' from loop (line 433) (redefined-loop-name)

[charliermarsh]

Ok cool, I'm happy with that level of parity then. I'll move on to reviewing the actual code.

[matthewlloyd]

This raises a couple of questions in my mind learning Ruff's "best practices":

• Are rules in different sets allowed to overlap?
• Do reimplementations of rules from other packages need to exactly match them in semantics?

If rules can overlap, perhaps one way out of the dilemma would be to have two rules which share the same codebase:

• PLW2901, which exactly matches Pylint:
  • for loops only, no with
  • reuse as a for loop variable is disallowed (signal)
  • reuse in assignment is disallowed (noisy)
• RUF???, which offers a more useful and practical compromise:
  • for loops and with statements
  • reuse as a for loop or with variable is disallowed (enhanced signal)
  • reuse in assignment is allowed (cut the noise, but possibly with an option for pedants to disallow these too)

[charliermarsh]

Good questions!

Are rules in different sets allowed to overlap?

It's not strictly forbidden but we try to avoid this. In fact over time we've actually removed some rules that turned out to be duplicates.

Do reimplementations of rules from other packages need to exactly match them in semantics?

No, but we try to be intentional about deviations (so the upstream package is seen as the reference implementation, and any deviations should be intentional).

We often deviate in the case of clear bugs or Pareto improvements (strictly fewer false positives or false negatives).

There's also some precedent for deviating with Pylint rules specifically. For example, we changed the defaults in the magic-value checks to avoid flagging comparisons to string and bytes, which felt noisy. (That's another strategy: deviate by way of configuration.)

[charliermarsh]

Another approach would be to pass in a mutable vector, and have the various arms push to it. Would that work? Perhaps not as nice conceptually, but a bit simpler.

[matthewlloyd]

Certainly could, but may be less efficient overall because it will require an additional temporary vector for each call. This is because the outputs of these methods can't be pushed directly to the InnerForWithAssignTargetsVisitor.assignment_targets vector as they first need to be paired with the appropriate BindingKind variant by the caller, which depends on the call path; so the intermediate Vec<Expr> would be immediately discarded each time.

On the other hand, I suppose the Boxes also necessitate a heap allocation, albeit one that has a fixed size and doesn't get expanded...

An alternative would be to pass in both a &mut Vec, and also the BindingKind, to these methods, and have them push ExprWithBindingKind into the vector.

[matthewlloyd]

Thanks again for the thorough and educational review and the quick merge.

[charliermarsh]

Thank you for all the work! Have been really impressed by your thoroughness and attention to detail, not to mention the quality of your code.

[henryiii]

Was this intended to trigger when the with is outside the loop?

with open("something") as f:
    for line in f:
        line = line.strip()

Produces:

$ pipx run ruff --select PL tmp.py --isolated
tmp.py:3:9: PLW2901 Outer for loop variable `line` overwritten by inner assignment target
Found 1 error.

This does not produce a warning with pylint. From what I gathered above, this is deviating from pylint intentionally, but the with was supposed to be between the assignment and the loop?

I could go through and change the places where this happens, but it seems like a common and harmless pattern?

(If this was a unique warning number for with's, I'd probably ignore it, since very few blocks in Python make scope, including things that probably should. :) )

[matthewlloyd]

This does not produce a warning with pylint. From what I gathered above, this is deviating from pylint intentionally, but the with was supposed to be between the assignment and the loop?

This doesn't produce a warning with pylint by default because the entire W2901 check is optional and only enabled if the pylint.extensions.redefined_loop_name plugin is loaded. However, if you do load that plugin, you'll find that PyLint does produce an error on the third line here, yes, matching the behavior we decided to add to Ruff.

https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/redefined-loop-name.html

Note that the presence of the outer with is irrelevant here, and both PyLint and this version will produce an error on line = line.strip() even if you have only the for loop.

Ultimately it's a matter of personal preference. When I wrote this code, I was thinking I would add an option to disable the error for simple reassignments within a loop, since it seemed over the top. Then when I ran it on my codebase and weighed the pros and cons, I realized I would rather change my harmless habit (that wasn't really so harmless!), and instead write:

for raw_line in f:
    line = raw_line.strip()

The reasons being:

• Unless you always strip the line every time you iterate over the lines in a file, it's easy to misremember whether your line variable represents the line with or without the newline, and end up e.g. printing or writing the newline to another file when it is not wanted or vice versa. Establishing a convention (or some similar convention) where line is newline-stripped and raw_line contains the newline makes this error less likely.
• It's impossible for a linter tool to distinguish between the above "harmless" case, and more dangerous cases where the loop variable is reassigned accidentally:

for i in range(10):
    # ... many lines of code
    i = 10  # possibly buried in nested code
    # ... many lines of code
    print(i)   # developer expected `i` to contain the original iteration value

We'd have establish a whitelist of loop variable names that are allowed to be reassigned to, or a privileged position within the loop where it is permitted (e.g. the first line only). However that complicates the rule and is also fragile since it's impossible to cover every case.

IMHO it's better to just flag it as an error, and rename the variable or add a noqa as needed.

[matthewlloyd]

Practically speaking one option would be to separate out "assignment within a loop" from "reuse as another loop variable" into two separate rules, e.g. PLW2901A and PLW2901, so users could choose to ignore PLW2901A while still catching more obviously unintentional errors like this as PLW2901:

for i in range(10):
    for i in range(10):  # PLW2901
        pass

[henryiii]

Ahh, I missed that it was an optional pylint flag! I could have sworn I've seen that error before, and I'm pretty bad about digging up the optional pylint extensions.

Since it's not triggering due to the with, I'm fine with it. The wording of the error (with "outer") is a tiny bit confusing - if it just said it was a variable reassignment for the loop, I'd have understood it wasn't a false positive. I think. :)

[matthewlloyd]

if it just said it was a variable reassignment for the loop, I'd have understood it wasn't a false positive.

That's a great point, we should perhaps only use the word "outer" when there are two loops involved. Thanks for your feedback!

[henryiii]

we should perhaps only use the word "outer" when there are two loops involved

That would be great!