[pylint] Implement redefined-slots-in-subclass (W0244) (astral-…

…sh#9640)

## Summary

- Implementation of [redefined-slots-in-subclass /
W0244](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/redefined-slots-in-subclass.html).
- Related to astral-sh#970

---------

Co-authored-by: Akira Noda <[email protected]>
Co-authored-by: dylwil3 <[email protected]>

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"]

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,

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),

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"))]

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;

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<H: std::hash::Hasher>(&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<Slot> {
+    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<Item = Slot> {
+    // 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())
+}

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
+   |