Conversation
cooperlees
approved these changes
Oct 1, 2026
cooperlees
left a comment
Collaborator
There was a problem hiding this comment.
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.pyfixture. - 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,_iexemption): all behave as documented — intermediate non-loop nodes don‘t break ancestor detection, and function/class/lambda/comprehension scopes correctly reset via the per-scopenode_stack(since they’re all inCONTEXTFUL_NODES). - The
child in ancestor.bodyguard correctly handles theelsesuite (outerelsedoesn’t count as active, inner-loopelsestill does), and reusingnames_from_assignmentsneatly 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_bindingsto something liketest_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 specificNamenode (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 insidefor i …would emit a duplicate B045 for the same inner loop sincenames_from_assignmentscan yield duplicates — trivial dedup if you want it.outer_names = set()could be annotatedset[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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #360 with B045: report a nested
fororasync fortarget 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'selsesuite 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 ... astargets or ordinary assignments.Validation
Developed with AI assistance.