diff --git a/crates/red_knot_python_semantic/resources/mdtest/slots.md b/crates/red_knot_python_semantic/resources/mdtest/slots.md new file mode 100644 index 00000000000000..4065c5d2f58612 --- /dev/null +++ b/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): ... +``` diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 4514e136dfa775..547ce40c703aca 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/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; diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 4d4fb738e4ce76..65f85c5f61880b 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/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 "", line 1, in + /// 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__`"), + ); +} diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 4547246b432091..0f7a83d69eb616 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/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 mro_error.reason() { - MroErrorKind::DuplicateBases(duplicates) => { - let base_nodes = class_node.bases(); - for (index, duplicate) in duplicates { - self.context.report_lint( - &DUPLICATE_BASE, - (&base_nodes[*index]).into(), - format_args!("Duplicate base class `{}`", duplicate.name(self.db())), - ); + match class.try_mro(self.db()).as_ref() { + Err(mro_error) => { + match mro_error.reason() { + MroErrorKind::DuplicateBases(duplicates) => { + let base_nodes = class_node.bases(); + for (index, duplicate) in duplicates { + self.context.report_lint( + &DUPLICATE_BASE, + (&base_nodes[*index]).into(), + format_args!("Duplicate base class `{}`", duplicate.name(self.db())), + ); + } } - } - MroErrorKind::InvalidBases(bases) => { - let base_nodes = class_node.bases(); - for (index, base_ty) in bases { - self.context.report_lint( - &INVALID_BASE, - (&base_nodes[*index]).into(), - format_args!( - "Invalid class base with type `{}` (all bases must be a class, `Any`, `Unknown` or `Todo`)", - base_ty.display(self.db()) - ), - ); + MroErrorKind::InvalidBases(bases) => { + let base_nodes = class_node.bases(); + for (index, base_ty) in bases { + self.context.report_lint( + &INVALID_BASE, + (&base_nodes[*index]).into(), + format_args!( + "Invalid class base with type `{}` (all bases must be a class, `Any`, `Unknown` or `Todo`)", + 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. diff --git a/crates/red_knot_python_semantic/src/types/slots.rs b/crates/red_knot_python_semantic/src/types/slots.rs new file mode 100644 index 00000000000000..fe6ce48b777a7b --- /dev/null +++ b/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); + }; + } +}