From 953e862aca1449651d96036fcd35f7fab4ccda51 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 8 Nov 2024 11:58:57 +0000 Subject: [PATCH] [red-knot] Improve error message for metaclass conflict (#14174) --- .../resources/mdtest/metaclass.md | 6 +- crates/red_knot_python_semantic/src/types.rs | 58 ++++++++++++++---- .../src/types/infer.rs | 60 ++++++++++++++----- 3 files changed, 94 insertions(+), 30 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/metaclass.md b/crates/red_knot_python_semantic/resources/mdtest/metaclass.md index 0630b266b3..1e9d726622 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/metaclass.md +++ b/crates/red_knot_python_semantic/resources/mdtest/metaclass.md @@ -65,7 +65,7 @@ class M2(type): ... class A(metaclass=M1): ... class B(metaclass=M2): ... -# error: [conflicting-metaclass] "The metaclass of a derived class (`C`) must be a subclass of the metaclasses of all its bases, but `M1` and `M2` have no subclass relationship" +# error: [conflicting-metaclass] "The metaclass of a derived class (`C`) must be a subclass of the metaclasses of all its bases, but `M1` (metaclass of base class `A`) and `M2` (metaclass of base class `B`) have no subclass relationship" class C(A, B): ... reveal_type(C.__class__) # revealed: Unknown @@ -82,7 +82,7 @@ class M1(type): ... class M2(type): ... class A(metaclass=M1): ... -# error: [conflicting-metaclass] "The metaclass of a derived class (`B`) must be a subclass of the metaclasses of all its bases, but `M2` and `M1` have no subclass relationship" +# error: [conflicting-metaclass] "The metaclass of a derived class (`B`) must be a subclass of the metaclasses of all its bases, but `M2` (metaclass of `B`) and `M1` (metaclass of base class `A`) have no subclass relationship" class B(A, metaclass=M2): ... reveal_type(B.__class__) # revealed: Unknown @@ -126,7 +126,7 @@ class A(metaclass=M1): ... class B(metaclass=M2): ... class C(metaclass=M12): ... -# error: [conflicting-metaclass] "The metaclass of a derived class (`D`) must be a subclass of the metaclasses of all its bases, but `M1` and `M2` have no subclass relationship" +# error: [conflicting-metaclass] "The metaclass of a derived class (`D`) must be a subclass of the metaclasses of all its bases, but `M1` (metaclass of base class `A`) and `M2` (metaclass of base class `B`) have no subclass relationship" class D(A, B, C): ... reveal_type(D.__class__) # revealed: Unknown diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index f06ed8ab36..a20a05c537 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -2345,15 +2345,22 @@ impl<'db> Class<'db> { .filter_map(Type::into_class_literal); // Identify the class's own metaclass (or take the first base class's metaclass). - let metaclass = if let Some(metaclass) = class.explicit_metaclass(db) { - metaclass + let explicit_metaclass = class.explicit_metaclass(db); + let (metaclass, class_metaclass_was_from) = if let Some(metaclass) = explicit_metaclass + { + (metaclass, class) } else if let Some(base_class) = base_classes.next() { - safe_recurse(base_class.class)? + (safe_recurse(base_class.class)?, base_class.class) } else { - KnownClass::Type.to_class(db) + (KnownClass::Type.to_class(db), class) }; - let Type::ClassLiteral(mut candidate) = metaclass else { + let mut candidate = if let Type::ClassLiteral(metaclass_ty) = metaclass { + MetaclassCandidate { + metaclass: metaclass_ty.class, + explicit_metaclass_of: class_metaclass_was_from, + } + } else { // TODO: If the metaclass is not a class, we should verify that it's a callable // which accepts the same arguments as `type.__new__` (otherwise error), and return // the meta-type of its return type. (And validate that is a class type?) @@ -2370,22 +2377,31 @@ impl<'db> Class<'db> { let Type::ClassLiteral(metaclass) = metaclass else { continue; }; - if metaclass.class.is_subclass_of(db, candidate.class) { - candidate = metaclass; + if metaclass.class.is_subclass_of(db, candidate.metaclass) { + candidate = MetaclassCandidate { + metaclass: metaclass.class, + explicit_metaclass_of: base_class.class, + }; continue; } - if candidate.class.is_subclass_of(db, metaclass.class) { + if candidate.metaclass.is_subclass_of(db, metaclass.class) { continue; } return Err(MetaclassError { kind: MetaclassErrorKind::Conflict { - metaclass1: candidate.class, - metaclass2: metaclass.class, + candidate1: candidate, + candidate2: MetaclassCandidate { + metaclass: metaclass.class, + explicit_metaclass_of: base_class.class, + }, + candidate1_is_base_class: explicit_metaclass.is_none(), }, }); } - Ok(Type::ClassLiteral(candidate)) + Ok(Type::ClassLiteral(ClassLiteralType { + class: candidate.metaclass, + })) } infer(db, self, &mut SeenSet::new(self)) @@ -2437,6 +2453,13 @@ impl<'db> Class<'db> { } } +/// Either the explicit `metaclass=` keyword of the class, or the inferred metaclass of one of its base classes. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct MetaclassCandidate<'db> { + metaclass: Class<'db>, + explicit_metaclass_of: Class<'db>, +} + /// A utility struct for detecting duplicates in class hierarchies while storing the initial /// entry on the stack. #[derive(Debug, Clone, PartialEq, Eq)] @@ -2541,9 +2564,18 @@ pub(super) enum MetaclassErrorKind<'db> { /// The metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all /// its bases. Conflict { - metaclass1: Class<'db>, - metaclass2: Class<'db>, + /// `candidate1` will either be the explicit `metaclass=` keyword in the class definition, + /// or the inferred metaclass of a base class + candidate1: MetaclassCandidate<'db>, + + /// `candidate2` will always be the inferred metaclass of a base class + candidate2: MetaclassCandidate<'db>, + + /// Flag to indicate whether `candidate1` is the explicit `metaclass=` keyword or the + /// inferred metaclass of a base class. This helps us give better error messages in diagnostics. + candidate1_is_base_class: bool, }, + /// The class inherits from itself! /// /// This is very unlikely to happen in working real-world code, diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index c716f6ff0b..f68f502e25 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -58,8 +58,8 @@ use crate::types::{ bindings_ty, builtins_symbol, declarations_ty, global_symbol, symbol, typing_extensions_symbol, Boundness, BytesLiteralType, Class, ClassLiteralType, FunctionType, InstanceType, IntersectionBuilder, IntersectionType, IterationOutcome, KnownClass, KnownFunction, - KnownInstanceType, MetaclassErrorKind, SliceLiteralType, StringLiteralType, Symbol, Truthiness, - TupleType, Type, TypeArrayDisplay, UnionBuilder, UnionType, + KnownInstanceType, MetaclassCandidate, MetaclassErrorKind, SliceLiteralType, StringLiteralType, + Symbol, Truthiness, TupleType, Type, TypeArrayDisplay, UnionBuilder, UnionType, }; use crate::unpack::Unpack; use crate::util::subscript::{PyIndex, PySlice}; @@ -520,18 +520,50 @@ impl<'db> TypeInferenceBuilder<'db> { if let Err(metaclass_error) = class.try_metaclass(self.db) { match metaclass_error.reason() { MetaclassErrorKind::Conflict { - metaclass1, - metaclass2 - } => self.diagnostics.add( - class.node(self.db).into(), - "conflicting-metaclass", - format_args!( - "The metaclass of a derived class (`{}`) must be a subclass of the metaclasses of all its bases, but `{}` and `{}` have no subclass relationship", - class.name(self.db), - metaclass1.name(self.db), - metaclass2.name(self.db), - ), - ), + candidate1: + MetaclassCandidate { + metaclass: metaclass1, + explicit_metaclass_of: class1, + }, + candidate2: + MetaclassCandidate { + metaclass: metaclass2, + explicit_metaclass_of: class2, + }, + candidate1_is_base_class, + } => { + let node = class.node(self.db).into(); + if *candidate1_is_base_class { + self.diagnostics.add( + node, + "conflicting-metaclass", + format_args!( + "The metaclass of a derived class (`{class}`) must be a subclass of the metaclasses of all its bases, \ + but `{metaclass1}` (metaclass of base class `{base1}`) and `{metaclass2}` (metaclass of base class `{base2}`) \ + have no subclass relationship", + class = class.name(self.db), + metaclass1 = metaclass1.name(self.db), + base1 = class1.name(self.db), + metaclass2 = metaclass2.name(self.db), + base2 = class2.name(self.db), + ) + ); + } else { + self.diagnostics.add( + node, + "conflicting-metaclass", + format_args!( + "The metaclass of a derived class (`{class}`) must be a subclass of the metaclasses of all its bases, \ + but `{metaclass_of_class}` (metaclass of `{class}`) and `{metaclass_of_base}` (metaclass of base class `{base}`) \ + have no subclass relationship", + class = class.name(self.db), + metaclass_of_class = metaclass1.name(self.db), + metaclass_of_base = metaclass2.name(self.db), + base = class2.name(self.db), + ) + ); + } + } MetaclassErrorKind::CyclicDefinition => { // Cyclic class definition diagnostic will already have been emitted above // in MRO calculation.