From 4caeeb8d9872ca7143020266bcc67ac8f1451b5e Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Sat, 18 Jan 2025 09:50:27 -0600 Subject: [PATCH] [`pylint`] Include name of base class in message for `redefined-slots-in-subclass` (`W0244`) (#15559) In the following situation: ```python class Grandparent: __slots__ = "a" class Parent(Grandparent): ... class Child(Parent): __slots__ = "a" ``` the message for `W0244` now specifies that `a` is overwriting a slot from `Grandparent`. 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 . --- .../rules/redefined_slots_in_subclass.rs | 45 +++++----- ...LW0244_redefined_slots_in_subclass.py.snap | 8 +- .../ruff_python_semantic/src/analyze/class.rs | 88 +++++++++++-------- 3 files changed, 79 insertions(+), 62 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs b/crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs index bd1dd0e0a6..3817e8e888 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs @@ -1,6 +1,6 @@ use std::hash::Hash; -use ruff_python_semantic::{analyze::class::any_super_class, SemanticModel}; +use ruff_python_semantic::{analyze::class::iter_super_class, SemanticModel}; use rustc_hash::FxHashSet; use ruff_diagnostics::{Diagnostic, Violation}; @@ -39,14 +39,15 @@ use crate::checkers::ast::Checker; /// ``` #[derive(ViolationMetadata)] pub(crate) struct RedefinedSlotsInSubclass { - name: String, + base: String, + slot_name: String, } impl Violation for RedefinedSlotsInSubclass { #[derive_message_formats] fn message(&self) -> String { - let RedefinedSlotsInSubclass { name } = self; - format!("Redefined slot '{name}' in subclass") + let RedefinedSlotsInSubclass { base, slot_name } = self; + format!("Slot `{slot_name}` redefined from base class `{base}`") } } @@ -68,15 +69,7 @@ pub(crate) fn redefined_slots_in_subclass(checker: &mut Checker, class_def: &ast let semantic = checker.semantic(); let mut diagnostics: Vec<_> = class_slots .iter() - .filter(|&slot| contained_in_super_slots(class_def, semantic, slot)) - .map(|slot| { - Diagnostic::new( - RedefinedSlotsInSubclass { - name: slot.name.to_string(), - }, - slot.range(), - ) - }) + .filter_map(|slot| check_super_slots(class_def, semantic, slot)) .collect(); checker.diagnostics.append(&mut diagnostics); } @@ -111,19 +104,25 @@ impl Ranged for Slot<'_> { } } -fn contained_in_super_slots( +fn check_super_slots( class_def: &ast::StmtClassDef, semantic: &SemanticModel, slot: &Slot, -) -> bool { - any_super_class(class_def, semantic, &|super_class| { - // This function checks every super class - // but we want every _strict_ super class, hence: - if class_def.name == super_class.name { - return false; - } - slots_members(&super_class.body).contains(slot) - }) +) -> Option { + iter_super_class(class_def, semantic) + .skip(1) + .find_map(&|super_class: &ast::StmtClassDef| { + if slots_members(&super_class.body).contains(slot) { + return Some(Diagnostic::new( + RedefinedSlotsInSubclass { + base: super_class.name.to_string(), + slot_name: slot.name.to_string(), + }, + slot.range(), + )); + } + None + }) } fn slots_members(body: &[Stmt]) -> FxHashSet { diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap index 55274f0ab4..8077c6868d 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -redefined_slots_in_subclass.py:6:18: PLW0244 Redefined slot 'a' in subclass +redefined_slots_in_subclass.py:6:18: PLW0244 Slot `a` redefined from base class `Base` | 5 | class Subclass(Base): 6 | __slots__ = ("a", "d") # [redefined-slots-in-subclass] @@ -10,7 +10,7 @@ redefined_slots_in_subclass.py:6:18: PLW0244 Redefined slot 'a' in subclass 8 | class Grandparent: | -redefined_slots_in_subclass.py:17:23: PLW0244 Redefined slot 'a' in subclass +redefined_slots_in_subclass.py:17:23: PLW0244 Slot `a` redefined from base class `Grandparent` | 16 | class Child(Parent): 17 | __slots__ = ("c", "a") @@ -19,14 +19,14 @@ redefined_slots_in_subclass.py:17:23: PLW0244 Redefined slot 'a' in subclass 19 | class AnotherBase: | -redefined_slots_in_subclass.py:23:18: PLW0244 Redefined slot 'a' in subclass +redefined_slots_in_subclass.py:23:18: PLW0244 Slot `a` redefined from base class `AnotherBase` | 22 | class AnotherChild(AnotherBase): 23 | __slots__ = ["a","b","e","f"] | ^^^ PLW0244 | -redefined_slots_in_subclass.py:23:22: PLW0244 Redefined slot 'b' in subclass +redefined_slots_in_subclass.py:23:22: PLW0244 Slot `b` redefined from base class `AnotherBase` | 22 | class AnotherChild(AnotherBase): 23 | __slots__ = ["a","b","e","f"] diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs index 34f1f058c1..0cb3a8a7f6 100644 --- a/crates/ruff_python_semantic/src/analyze/class.rs +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use rustc_hash::FxHashSet; use crate::analyze::typing; @@ -70,46 +72,62 @@ pub fn any_base_class( inner(class_def, semantic, func, &mut FxHashSet::default()) } -/// Return `true` if any base class matches an [`ast::StmtClassDef`] predicate. +/// Returns an iterator over all base classes, beginning with the +/// given class. +/// +/// The traversal of the class hierarchy is breadth-first, since +/// this graph tends to have small width but could be rather deep. +pub fn iter_super_class<'stmt>( + class_def: &'stmt ast::StmtClassDef, + semantic: &'stmt SemanticModel, +) -> impl Iterator { + struct SuperClassIterator<'stmt> { + semantic: &'stmt SemanticModel<'stmt>, + to_visit: VecDeque<&'stmt ast::StmtClassDef>, + seen: FxHashSet, + } + + impl<'a> Iterator for SuperClassIterator<'a> { + type Item = &'a ast::StmtClassDef; + + fn next(&mut self) -> Option { + let current = self.to_visit.pop_front()?; + + // Add all base classes to the to_visit list + for expr in current.bases() { + if let Some(id) = self.semantic.lookup_attribute(map_subscript(expr)) { + if self.seen.insert(id) { + let binding = self.semantic.binding(id); + if let Some(base_class) = binding + .kind + .as_class_definition() + .map(|id| &self.semantic.scopes[*id]) + .and_then(|scope| scope.kind.as_class()) + { + self.to_visit.push_back(base_class); + } + } + } + } + Some(current) + } + } + + SuperClassIterator { + semantic, + to_visit: VecDeque::from([class_def]), + seen: FxHashSet::default(), + } +} + +/// Return `true` if any base class, including the given class, +/// matches an [`ast::StmtClassDef`] predicate. pub fn any_super_class( class_def: &ast::StmtClassDef, semantic: &SemanticModel, func: &dyn Fn(&ast::StmtClassDef) -> bool, ) -> bool { - fn inner( - class_def: &ast::StmtClassDef, - semantic: &SemanticModel, - func: &dyn Fn(&ast::StmtClassDef) -> bool, - seen: &mut FxHashSet, - ) -> bool { - // If the function itself matches the pattern, then this does too. - if func(class_def) { - return true; - } - - // Otherwise, check every base class. - class_def.bases().iter().any(|expr| { - // If the base class extends a class that matches the pattern, then this does too. - if let Some(id) = semantic.lookup_attribute(map_subscript(expr)) { - if seen.insert(id) { - let binding = semantic.binding(id); - if let Some(base_class) = binding - .kind - .as_class_definition() - .map(|id| &semantic.scopes[*id]) - .and_then(|scope| scope.kind.as_class()) - { - if inner(base_class, semantic, func, seen) { - return true; - } - } - } - } - false - }) - } - - inner(class_def, semantic, func, &mut FxHashSet::default()) + iter_super_class(class_def, semantic).any(func) } #[derive(Clone, Debug)]