Repository navigation
[flake8-builtins] Expand checks in class scopes (A001) - #29076
Merged
Merged
Conversation
…gs (`A001`) `builtin-variable-shadowing` skipped every stored name in a class body on the grounds that `builtin-attribute-shadowing` (A003) covers class attributes. A003 only considers plain assignment and annotation bindings, so loop targets, `with ... as` targets and walrus targets in a class body were reported by neither rule. Narrow the class-scope skip to the binding kinds A003 actually handles. Fixes astral-sh#20179
Cover `for`, `:=` and `with ... as` targets in a class body, plus the attribute and method bindings that stay with A003, and refresh the snapshot. Refs astral-sh#20179
ntBre
reviewed
Oct 2, 2026
ntBre
left a comment
ntBre
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this looks good to me logic-wise. Let's just move the check into the rule code.
Comment on lines
+389
to
+400
| // In class scope, attribute bindings (e.g., `id = 1`) are covered by | ||
| // `builtin-attribute-shadowing` (A003), but other bindings (e.g., a | ||
| // `for` target or `:=`) are not and still shadow the builtin. | ||
| let scope = checker.semantic.current_scope(); | ||
| let is_class_attribute = scope.kind.is_class() | ||
| && scope.get(id).is_none_or(|binding_id| { | ||
| matches!( | ||
| checker.semantic.binding(binding_id).kind, | ||
| BindingKind::Assignment | BindingKind::Annotation | ||
| ) | ||
| }); | ||
| if !is_class_attribute { |
Contributor
There was a problem hiding this comment.
I initially missed that there was an existing scope check here. Now that the logic is more involved, I think it's a better fit to move it into the rule function itself.
ntBre
changed the title
flake8-builtins] Detect class-scope for, with, and := bindings (A001)flake8-builtins] Expand checks in class scopes (A001)
ntBre
added
the
bug
|
…_shadowing` Keep the decision about which class-scope bindings A003 already covers inside the rule rather than at the call site.
The rule now decides this itself.
Contributor
Author
|
I moved the check into the rule code; thanks for pointing that out. |
ntBre
approved these changes
Oct 2, 2026
ntBre
enabled auto-merge (squash)
October 2, 2026 18:33
Contributor
Author
|
Rerequested review by accident, nothing new. Thanks for being speedy! |
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
Inside a class body,
flake8-builtinsreports afor,with ... as, or:=target that shadows a builtin, but A001 does not. The A001 call site skipped every stored name in class scope because A003 handles class attributes, but A003 only considers assignment and annotation bindings, so those other binding kinds were reported by neither rule. The class-scope skip now applies only to the binding kinds A003 handles, by checking the stored name'sBindingKind. Attribute and method bindings in a class still go to A003, so there's no double-reporting.This is the gap left after #20178, which I picked up because no pull request was opened for it.
Fixes #20179
Test Plan
I have added new class-scope cases in the A001 fixture covering both the three bindings that now fire and the attribute and method bindings that stay silent, updated the snapshot with three new diagnostics, and ran the
flake8-builtinstests,clippy, andfmt.