Skip to content

Add B045 for reused control variables in nested loops - #584

Open
Eljees wants to merge 1 commit into
PyCQA:mainfrom
Eljees:n/b045-nested-loop
Open

Eljees wants to merge 1 commit into
PyCQA:mainfrom
Eljees:n/b045-nested-loop

Conversation

@Eljees

@Eljees Eljees commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses #360 with B045: report a nested for or async for target that rebinds a control variable of an active enclosing loop in the same scope.

The check reuses names_from_assignments, so tuple/list/starred unpacking is covered without treating attribute/subscript bases (or comprehensions within subscript indices) as bindings. It uses the existing per-scope node stack and only considers ancestors whose body contains the current path. An enclosing loop's else suite therefore does not count as that loop still being active.

Names starting with _ are ignored for intentional discards. Separate functions, classes and comprehensions are not reported. Triple nesting reports each inner rebinding once, rather than once per matching ancestor.

This proposal is limited to loop targets; it does not add a check for with ... as targets or ordinary assignments.

Validation

  • Added 23 parameterized cases plus a normal diagnostic fixture for line/column reporting.
  • Before implementation, 9 positive regression cases failed because no diagnostic was emitted; 12 initial negative cases passed. Two more negative cases cover comprehension bindings inside subscript targets.
  • Python 3.12: 108 passed, 4 skipped.
  • Python 3.14: 110 passed, 2 skipped.
  • Tested with the local checkout installed as the flake8 plugin; the CLI reports B045 at the expected fixture locations.
  • All pre-commit hooks passed (isort, black, flake8, rstcheck).

Developed with AI assistance.

@cooperlees cooperlees left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed on behalf of @cooperlees.

Thanks for this addition, @Eljees — addressing #360 with a new B045 is a genuinely useful check, and the implementation is nicely scoped and well tested.

What I verified:

  • Pulled the PR branch and ran the suite locally on Python 3.14: 110 passed, 2 skipped, including the 23 new parametrized cases plus the new tests/eval_files/b045.py fixture.
  • CI is green across 3.10–3.15 and pre-commit.ci is passing.
  • Manually probed edge cases beyond the parametrized matrix (loops nested via while/try/with, while-wrapped nesting, tuple-target column reporting, _i exemption): all behave as documented — intermediate non-loop nodes don‘t break ancestor detection, and function/class/lambda/comprehension scopes correctly reset via the per-scope node_stack (since they’re all in CONTEXTFUL_NODES).
  • The child in ancestor.body guard correctly handles the else suite (outer else doesn’t count as active, inner-loop else still does), and reusing names_from_assignments neatly covers tuple/list/starred unpacking without misfiring on attribute/subscript targets or comprehensions inside subscript indices.

Non-blocking nits (fine to land as-is, follow-ups welcome):

  • Consider renaming test_nested_loop_bindings to something like test_b045_* for consistency/discoverability with the other B0xx tests.
  • add_error("B045", node.target, …) reports tuple-unpacking rebindings at the whole-target location rather than the specific Name node (both errors share one col). Pointing at the individual name would give more precise columns, similar to how B007 reports per-name.
  • for i, i in … nested inside for i … would emit a duplicate B045 for the same inner loop since names_from_assignments can yield duplicates — trivial dedup if you want it.
  • outer_names = set() could be annotated set[str].

None of the above blocks merging. Scope limitation to loop targets (not with … as or plain assignments) is reasonable and clearly documented in the PR description and README.

Approving — happy for this to merge. Thanks again for the thorough validation notes in the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants