-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[pylint
] Include name of base class in message for redefined-slots-in-subclass
(W0244
)
#15559
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for following up quickly on this!
I just have one question about whether to use depth-first or breath-first traversal otherwise this looks good to go.
crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs
Outdated
Show resolved
Hide resolved
...int/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap
Outdated
Show resolved
Hide resolved
/// The traversal of the class hierarchy is breadth-first. | ||
pub fn iter_super_class<'stmt>( | ||
class_def: &'stmt ast::StmtClassDef, | ||
semantic: &'stmt SemanticModel, | ||
) -> impl Iterator<Item = &'stmt ast::StmtClassDef> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for doing breadth-first? I think the original implementation did depth-first traversal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my intuition is that the superclass graph will usually be deeper than it is wide (but maybe that assumption is off), and that the width is often pretty small in any event. Does that seem right to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To save you the re-review, I'm gonna merge this in. But if you wake up in the middle of the night shouting "depth first!" I will be happy to change it back 😄
In the following situation:
the message for
W0244
now specifies thata
is overwriting a slot fromGrandparent
.To implement this, we introduce a helper function
iter_super_classes
which does a breadth-first traversal of the superclasses of a given class (as long as they are defined in the same file, due to the usual limitations of the semantic model).Note: Python does not allow conflicting slots definitions under multiple inheritance. Unless I'm misunderstanding something, I believe It follows that the subposet of superclasses of a given class that redefine a given slot is in fact totally ordered. There is therefore a unique nearest superclass whose slot is being overwritten. So, you know, in case anyone was super worried about that... you can just chill.
This is a followup to #9640 .