diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/redefined_slots_in_subclass.py b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_slots_in_subclass.py new file mode 100644 index 0000000000..00896ee4e1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_slots_in_subclass.py @@ -0,0 +1,23 @@ +class Base: + __slots__ = ("a", "b") + + +class Subclass(Base): + __slots__ = ("a", "d") # [redefined-slots-in-subclass] + +class Grandparent: + __slots__ = ("a", "b") + + +class Parent(Grandparent): + pass + + +class Child(Parent): + __slots__ = ("c", "a") + +class AnotherBase: + __slots__ = ["a","b","c","d"] + +class AnotherChild(AnotherBase): + __slots__ = ["a","b","e","f"] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index ec27786cff..5611a307f4 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -424,6 +424,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ClassAsDataStructure) { flake8_bugbear::rules::class_as_data_structure(checker, class_def); } + if checker.enabled(Rule::RedefinedSlotsInSubclass) { + pylint::rules::redefined_slots_in_subclass(checker, class_def); + } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( checker, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 96331e878d..91123b2724 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -279,6 +279,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0131") => (RuleGroup::Stable, rules::pylint::rules::NamedExprWithoutContext), (Pylint, "W0133") => (RuleGroup::Stable, rules::pylint::rules::UselessExceptionStatement), (Pylint, "W0211") => (RuleGroup::Stable, rules::pylint::rules::BadStaticmethodArgument), + (Pylint, "W0244") => (RuleGroup::Preview, rules::pylint::rules::RedefinedSlotsInSubclass), (Pylint, "W0245") => (RuleGroup::Stable, rules::pylint::rules::SuperWithoutBrackets), (Pylint, "W0406") => (RuleGroup::Stable, rules::pylint::rules::ImportSelf), (Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 784f3f8995..fa3a69a75e 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -118,6 +118,10 @@ mod tests { Path::new("named_expr_without_context.py") )] #[test_case(Rule::NonlocalAndGlobal, Path::new("nonlocal_and_global.py"))] + #[test_case( + Rule::RedefinedSlotsInSubclass, + Path::new("redefined_slots_in_subclass.py") + )] #[test_case(Rule::NonlocalWithoutBinding, Path::new("nonlocal_without_binding.py"))] #[test_case(Rule::NonSlotAssignment, Path::new("non_slot_assignment.py"))] #[test_case(Rule::PropertyWithParameters, Path::new("property_with_parameters.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index f0f809ec42..3ddc469656 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -63,6 +63,7 @@ pub(crate) use property_with_parameters::*; pub(crate) use redeclared_assigned_name::*; pub(crate) use redefined_argument_from_local::*; pub(crate) use redefined_loop_name::*; +pub(crate) use redefined_slots_in_subclass::*; pub(crate) use repeated_equality_comparison::*; pub(crate) use repeated_isinstance_calls::*; pub(crate) use repeated_keyword_argument::*; @@ -171,6 +172,7 @@ mod property_with_parameters; mod redeclared_assigned_name; mod redefined_argument_from_local; mod redefined_loop_name; +mod redefined_slots_in_subclass; mod repeated_equality_comparison; mod repeated_isinstance_calls; mod repeated_keyword_argument; 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 new file mode 100644 index 0000000000..bd1dd0e0a6 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/redefined_slots_in_subclass.rs @@ -0,0 +1,213 @@ +use std::hash::Hash; + +use ruff_python_semantic::{analyze::class::any_super_class, SemanticModel}; +use rustc_hash::FxHashSet; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::{self as ast, Expr, Stmt}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for a re-defined slot in a subclass. +/// +/// ## Why is this bad? +/// If a class defines a slot also defined in a base class, the +/// instance variable defined by the base class slot is inaccessible +/// (except by retrieving its descriptor directly from the base class). +/// +/// ## Example +/// ```python +/// class Base: +/// __slots__ = ("a", "b") +/// +/// +/// class Subclass(Base): +/// __slots__ = ("a", "d") # slot "a" redefined +/// ``` +/// +/// Use instead: +/// ```python +/// class Base: +/// __slots__ = ("a", "b") +/// +/// +/// class Subclass(Base): +/// __slots__ = "d" +/// ``` +#[derive(ViolationMetadata)] +pub(crate) struct RedefinedSlotsInSubclass { + name: String, +} + +impl Violation for RedefinedSlotsInSubclass { + #[derive_message_formats] + fn message(&self) -> String { + let RedefinedSlotsInSubclass { name } = self; + format!("Redefined slot '{name}' in subclass") + } +} + +// PLW0244 +pub(crate) fn redefined_slots_in_subclass(checker: &mut Checker, class_def: &ast::StmtClassDef) { + // Early return if this is not a subclass + if class_def.bases().is_empty() { + return; + } + + let ast::StmtClassDef { body, .. } = class_def; + let class_slots = slots_members(body); + + // If there are no slots, we're safe + if class_slots.is_empty() { + return; + } + + 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(), + ) + }) + .collect(); + checker.diagnostics.append(&mut diagnostics); +} + +#[derive(Clone, Debug)] +struct Slot<'a> { + name: &'a str, + range: TextRange, +} + +impl std::cmp::PartialEq for Slot<'_> { + // We will only ever be comparing slots + // within a class and with the slots of + // a super class. In that context, we + // want to compare names and not ranges. + fn eq(&self, other: &Self) -> bool { + self.name == other.name + } +} + +impl std::cmp::Eq for Slot<'_> {} + +impl Hash for Slot<'_> { + fn hash(&self, state: &mut H) { + self.name.hash(state); + } +} + +impl Ranged for Slot<'_> { + fn range(&self) -> TextRange { + self.range + } +} + +fn contained_in_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) + }) +} + +fn slots_members(body: &[Stmt]) -> FxHashSet { + let mut members = FxHashSet::default(); + for stmt in body { + match stmt { + // Ex) `__slots__ = ("name",)` + Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { + let [Expr::Name(ast::ExprName { id, .. })] = targets.as_slice() else { + continue; + }; + + if id == "__slots__" { + members.extend(slots_attributes(value)); + } + } + + // Ex) `__slots__: Tuple[str, ...] = ("name",)` + Stmt::AnnAssign(ast::StmtAnnAssign { + target, + value: Some(value), + .. + }) => { + let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() else { + continue; + }; + + if id == "__slots__" { + members.extend(slots_attributes(value)); + } + } + + // Ex) `__slots__ += ("name",)` + Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { + let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() else { + continue; + }; + + if id == "__slots__" { + members.extend(slots_attributes(value)); + } + } + _ => {} + } + } + members +} + +fn slots_attributes(expr: &Expr) -> impl Iterator { + // Ex) `__slots__ = ("name",)` + let elts_iter = match expr { + Expr::Tuple(ast::ExprTuple { elts, .. }) + | Expr::List(ast::ExprList { elts, .. }) + | Expr::Set(ast::ExprSet { elts, .. }) => Some(elts.iter().filter_map(|elt| match elt { + Expr::StringLiteral(ast::ExprStringLiteral { value, range }) => Some(Slot { + name: value.to_str(), + range: *range, + }), + _ => None, + })), + _ => None, + }; + + // Ex) `__slots__ = {"name": ...}` + let keys_iter = match expr { + Expr::Dict(ast::ExprDict { .. }) => Some( + expr.as_dict_expr() + .unwrap() + .iter_keys() + .filter_map(|key| match key { + Some(Expr::StringLiteral(ast::ExprStringLiteral { value, range })) => { + Some(Slot { + name: value.to_str(), + range: *range, + }) + } + _ => None, + }), + ), + _ => None, + }; + + elts_iter + .into_iter() + .flatten() + .chain(keys_iter.into_iter().flatten()) +} 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 new file mode 100644 index 0000000000..55274f0ab4 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0244_redefined_slots_in_subclass.py.snap @@ -0,0 +1,34 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +redefined_slots_in_subclass.py:6:18: PLW0244 Redefined slot 'a' in subclass + | +5 | class Subclass(Base): +6 | __slots__ = ("a", "d") # [redefined-slots-in-subclass] + | ^^^ PLW0244 +7 | +8 | class Grandparent: + | + +redefined_slots_in_subclass.py:17:23: PLW0244 Redefined slot 'a' in subclass + | +16 | class Child(Parent): +17 | __slots__ = ("c", "a") + | ^^^ PLW0244 +18 | +19 | class AnotherBase: + | + +redefined_slots_in_subclass.py:23:18: PLW0244 Redefined slot 'a' in subclass + | +22 | class AnotherChild(AnotherBase): +23 | __slots__ = ["a","b","e","f"] + | ^^^ PLW0244 + | + +redefined_slots_in_subclass.py:23:22: PLW0244 Redefined slot 'b' in subclass + | +22 | class AnotherChild(AnotherBase): +23 | __slots__ = ["a","b","e","f"] + | ^^^ PLW0244 + | diff --git a/ruff.schema.json b/ruff.schema.json index c676763d8e..7469867d01 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3669,6 +3669,7 @@ "PLW021", "PLW0211", "PLW024", + "PLW0244", "PLW0245", "PLW04", "PLW040",