[red-knot] Report classes inheriting from bases with incompatible __slots__ (#15129)

crates/red_knot_python_semantic/resources/mdtest/slots.md

@@ -0,0 +1,184 @@
+# `__slots__`
+
+## Not specified and empty
+
+```py
+class A: ...
+
+class B:
+    __slots__ = ()
+
+class C:
+    __slots__ = ("lorem", "ipsum")
+
+class AB(A, B): ...  # fine
+class AC(A, C): ...  # fine
+class BC(B, C): ...  # fine
+class ABC(A, B, C): ...  # fine
+```
+
+## Incompatible tuples
+
+```py
+class A:
+    __slots__ = ("a", "b")
+
+class B:
+    __slots__ = ("c", "d")
+
+class C(
+    A,  # error: [incompatible-slots]
+    B,  # error: [incompatible-slots]
+): ...
+```
+
+## Same value
+
+```py
+class A:
+    __slots__ = ("a", "b")
+
+class B:
+    __slots__ = ("a", "b")
+
+class C(
+    A,  # error: [incompatible-slots]
+    B,  # error: [incompatible-slots]
+): ...
+```
+
+## Strings
+
+```py
+class A:
+    __slots__ = "abc"
+
+class B:
+    __slots__ = ("abc",)
+
+class AB(
+    A,  # error: [incompatible-slots]
+    B,  # error: [incompatible-slots]
+): ...
+```
+
+## Invalid
+
+TODO: Emit diagnostics
+
+```py
+class NonString1:
+    __slots__ = 42
+
+class NonString2:
+    __slots__ = b"ar"
+
+class NonIdentifier1:
+    __slots__ = "42"
+
+class NonIdentifier2:
+    __slots__ = ("lorem", "42")
+
+class NonIdentifier3:
+    __slots__ = (e for e in ("lorem", "42"))
+```
+
+## Inheritance
+
+```py
+class A:
+    __slots__ = ("a", "b")
+
+class B(A): ...
+
+class C:
+    __slots__ = ("c", "d")
+
+class D(C): ...
+class E(
+    B,  # error: [incompatible-slots]
+    D,  # error: [incompatible-slots]
+): ...
+```
+
+## Single solid base
+
+```py
+class A:
+    __slots__ = ("a", "b")
+
+class B(A): ...
+class C(A): ...
+class D(B, A): ...  # fine
+class E(B, C, A): ...  # fine
+```
+
+## False negatives
+
+### Possibly unbound
+
+```py
+def _(flag: bool):
+    class A:
+        if flag:
+            __slots__ = ("a", "b")
+
+    class B:
+        __slots__ = ("c", "d")
+
+    # Might or might not be fine at runtime
+    class C(A, B): ...
+```
+
+### Bound but with different types
+
+```py
+def _(flag: bool):
+    class A:
+        if flag:
+            __slots__ = ("a", "b")
+        else:
+            __slots__ = ()
+
+    class B:
+        __slots__ = ("c", "d")
+
+    # Might or might not be fine at runtime
+    class C(A, B): ...
+```
+
+### Non-tuples
+
+```py
+class A:
+    __slots__ = ["a", "b"]  # This is treated as "dynamic"
+
+class B:
+    __slots__ = ("c", "d")
+
+# False negative: [incompatible-slots]
+class C(A, B): ...
+```
+
+### Post-hoc modifications
+
+```py
+class A:
+    __slots__ = ()
+    __slots__ += ("a", "b")
+
+reveal_type(A.__slots__)  # revealed: @Todo(Support for more binary expressions)
+
+class B:
+    __slots__ = ("c", "d")
+
+# False negative: [incompatible-slots]
+class C(A, B): ...
+```
+
+### Built-ins with implicit layouts
+
+```py
+# False negative: [incompatible-slots]
+class A(int, str): ...
+```

crates/red_knot_python_semantic/src/types.rs

@@ -46,6 +46,7 @@ mod infer;
 mod mro;
 mod narrow;
 mod signatures;
+mod slots;
 mod string_annotation;
 mod unpacker;
 

crates/red_knot_python_semantic/src/types/diagnostic.rs

@@ -27,6 +27,7 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) {
     registry.register_lint(&CYCLIC_CLASS_DEFINITION);
     registry.register_lint(&DIVISION_BY_ZERO);
     registry.register_lint(&DUPLICATE_BASE);
+    registry.register_lint(&INCOMPATIBLE_SLOTS);
     registry.register_lint(&INCONSISTENT_MRO);
     registry.register_lint(&INDEX_OUT_OF_BOUNDS);
     registry.register_lint(&INVALID_ASSIGNMENT);
@@ -148,6 +149,64 @@ declare_lint! {
     }
 }
 
+declare_lint! {
+    /// ## What it does
+    /// Checks for classes whose bases define incompatible `__slots__`.
+    ///
+    /// ## Why is this bad?
+    /// Inheriting from bases with incompatible `__slots__`s
+    /// will lead to a `TypeError` at runtime.
+    ///
+    /// Classes with no or empty `__slots__` are always compatible:
+    ///
+    /// ```python
+    /// class A: ...
+    /// class B:
+    ///     __slots__ = ()
+    /// class C:
+    ///     __slots__ = ("a", "b")
+    ///
+    /// # fine
+    /// class D(A, B, C): ...
+    /// ```
+    ///
+    /// Multiple inheritance from more than one different class
+    /// defining non-empty `__slots__` is not allowed:
+    ///
+    /// ```python
+    /// class A:
+    ///     __slots__ = ("a", "b")
+    ///
+    /// class B:
+    ///     __slots__ = ("a", "b")  # Even if the values are the same
+    ///
+    /// # TypeError: multiple bases have instance lay-out conflict
+    /// class C(A, B): ...
+    /// ```
+    ///
+    /// ## Known problems
+    /// Dynamic (not tuple or string literal) `__slots__` are not checked.
+    /// Additionally, classes inheriting from built-in classes with implicit layouts
+    /// like `str` or `int` are also not checked.
+    ///
+    /// ```pycon
+    /// >>> hasattr(int, "__slots__")
+    /// False
+    /// >>> hasattr(str, "__slots__")
+    /// False
+    /// >>> class A(int, str): ...
+    /// Traceback (most recent call last):
+    ///   File "<python-input-0>", line 1, in <module>
+    ///     class A(int, str): ...
+    /// TypeError: multiple bases have instance lay-out conflict
+    /// ```
+    pub(crate) static INCOMPATIBLE_SLOTS = {
+        summary: "detects class definitions whose MRO has conflicting `__slots__`",
+        status: LintStatus::preview("1.0.0"),
+        default_level: Level::Error,
+    }
+}
+
 declare_lint! {
     /// TODO #14889
     pub(crate) static INCONSISTENT_MRO = {
@@ -813,3 +872,11 @@ pub(crate) fn report_invalid_exception_cause(context: &InferContext, node: &ast:
         ),
     );
 }
+
+pub(crate) fn report_base_with_incompatible_slots(context: &InferContext, node: &ast::Expr) {
+    context.report_lint(
+        &INCOMPATIBLE_SLOTS,
+        node.into(),
+        format_args!("Class base has incompatible `__slots__`"),
+    );
+}

crates/red_knot_python_semantic/src/types/infer.rs

@@ -79,6 +79,7 @@ use super::diagnostic::{
     report_possibly_unresolved_reference, report_slice_step_size_zero, report_unresolved_reference,
     SUBCLASS_OF_FINAL_CLASS,
 };
+use super::slots::check_class_slots;
 use super::string_annotation::{
     parse_string_annotation, BYTE_STRING_TYPE_ANNOTATION, FSTRING_TYPE_ANNOTATION,
 };
@@ -585,41 +586,44 @@ impl<'db> TypeInferenceBuilder<'db> {
             }
 
             // (3) Check that the class's MRO is resolvable
-            if let Err(mro_error) = class.try_mro(self.db()).as_ref() {
+            match class.try_mro(self.db()).as_ref() {
-                match mro_error.reason() {
+                Err(mro_error) => {
-                    MroErrorKind::DuplicateBases(duplicates) => {
+                    match mro_error.reason() {
-                        let base_nodes = class_node.bases();
+                        MroErrorKind::DuplicateBases(duplicates) => {
-                        for (index, duplicate) in duplicates {
+                            let base_nodes = class_node.bases();
-                            self.context.report_lint(
+                            for (index, duplicate) in duplicates {
-                                &DUPLICATE_BASE,
+                                self.context.report_lint(
-                                (&base_nodes[*index]).into(),
+                                    &DUPLICATE_BASE,
-                                format_args!("Duplicate base class `{}`", duplicate.name(self.db())),
+                                    (&base_nodes[*index]).into(),
-                            );
+                                    format_args!("Duplicate base class `{}`", duplicate.name(self.db())),
+                                );
+                            }
                         }
-                    }
+                        MroErrorKind::InvalidBases(bases) => {
-                    MroErrorKind::InvalidBases(bases) => {
+                            let base_nodes = class_node.bases();
-                        let base_nodes = class_node.bases();
+                            for (index, base_ty) in bases {
-                        for (index, base_ty) in bases {
+                                self.context.report_lint(
-                            self.context.report_lint(
+                                    &INVALID_BASE,
-                                &INVALID_BASE,
+                                    (&base_nodes[*index]).into(),
-                                (&base_nodes[*index]).into(),
+                                    format_args!(
-                                format_args!(
+                                        "Invalid class base with type `{}` (all bases must be a class, `Any`, `Unknown` or `Todo`)",
-                                    "Invalid class base with type `{}` (all bases must be a class, `Any`, `Unknown` or `Todo`)",
+                                        base_ty.display(self.db())
-                                    base_ty.display(self.db())
+                                    ),
-                                ),
+                                );
-                            );
+                            }
                         }
+                        MroErrorKind::UnresolvableMro { bases_list } => self.context.report_lint(
+                            &INCONSISTENT_MRO,
+                            class_node.into(),
+                            format_args!(
+                                "Cannot create a consistent method resolution order (MRO) for class `{}` with bases list `[{}]`",
+                                class.name(self.db()),
+                                bases_list.iter().map(|base| base.display(self.db())).join(", ")
+                            ),
+                        )
                     }
-                    MroErrorKind::UnresolvableMro { bases_list } => self.context.report_lint(
-                        &INCONSISTENT_MRO,
-                        class_node.into(),
-                        format_args!(
-                            "Cannot create a consistent method resolution order (MRO) for class `{}` with bases list `[{}]`",
-                            class.name(self.db()),
-                            bases_list.iter().map(|base| base.display(self.db())).join(", ")
-                        ),
-                    )
                 }
+                Ok(_) => check_class_slots(&self.context, class, class_node)
             }
 
             // (4) Check that the class's metaclass can be determined without error.

crates/red_knot_python_semantic/src/types/slots.rs

@@ -0,0 +1,103 @@
+use ruff_python_ast as ast;
+
+use crate::db::Db;
+use crate::symbol::{Boundness, Symbol};
+use crate::types::class_base::ClassBase;
+use crate::types::diagnostic::report_base_with_incompatible_slots;
+use crate::types::{Class, ClassLiteralType, Type};
+
+use super::InferContext;
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum SlotsKind {
+    /// `__slots__` is not found in the class.
+    NotSpecified,
+    /// `__slots__` is defined but empty: `__slots__ = ()`.
+    Empty,
+    /// `__slots__` is defined and is not empty: `__slots__ = ("a", "b")`.
+    NotEmpty,
+    /// `__slots__` is defined but its value is dynamic:
+    /// * `__slots__ = tuple(a for a in b)`
+    /// * `__slots__ = ["a", "b"]`
+    Dynamic,
+}
+
+impl SlotsKind {
+    fn from(db: &dyn Db, base: Class) -> Self {
+        let Symbol::Type(slots_ty, bound) = base.own_class_member(db, "__slots__") else {
+            return Self::NotSpecified;
+        };
+
+        if matches!(bound, Boundness::PossiblyUnbound) {
+            return Self::Dynamic;
+        };
+
+        match slots_ty {
+            // __slots__ = ("a", "b")
+            Type::Tuple(tuple) => {
+                if tuple.elements(db).is_empty() {
+                    Self::Empty
+                } else {
+                    Self::NotEmpty
+                }
+            }
+
+            // __slots__ = "abc"  # Same as `("abc",)`
+            Type::StringLiteral(_) => Self::NotEmpty,
+
+            _ => Self::Dynamic,
+        }
+    }
+}
+
+pub(super) fn check_class_slots(context: &InferContext, class: Class, node: &ast::StmtClassDef) {
+    let db = context.db();
+
+    let mut first_with_solid_base = None;
+    let mut common_solid_base = None;
+    let mut found_second = false;
+
+    for (index, base) in class.explicit_bases(db).iter().enumerate() {
+        let Type::ClassLiteral(ClassLiteralType { class: base }) = base else {
+            continue;
+        };
+
+        let solid_base = base.iter_mro(db).find_map(|current| {
+            let ClassBase::Class(current) = current else {
+                return None;
+            };
+
+            match SlotsKind::from(db, current) {
+                SlotsKind::NotEmpty => Some(current),
+                SlotsKind::NotSpecified | SlotsKind::Empty => None,
+                SlotsKind::Dynamic => None,
+            }
+        });
+
+        if solid_base.is_none() {
+            continue;
+        }
+
+        let base_node = &node.bases()[index];
+
+        if first_with_solid_base.is_none() {
+            first_with_solid_base = Some(index);
+            common_solid_base = solid_base;
+            continue;
+        }
+
+        if solid_base == common_solid_base {
+            continue;
+        }
+
+        found_second = true;
+        report_base_with_incompatible_slots(context, base_node);
+    }
+
+    if found_second {
+        if let Some(index) = first_with_solid_base {
+            let base_node = &node.bases()[index];
+            report_base_with_incompatible_slots(context, base_node);
+        };
+    }
+}