[red-knot] Improve error message for metaclass conflict (#14174)

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

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) {
+            let explicit_metaclass = class.explicit_metaclass(db);
-                metaclass
+            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) {
+                if metaclass.class.is_subclass_of(db, candidate.metaclass) {
-                    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,
+                        candidate1: candidate,
-                        metaclass2: metaclass.class,
+                        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>,
+        /// `candidate1` will either be the explicit `metaclass=` keyword in the class definition,
-        metaclass2: Class<'db>,
+        /// 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,

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,
+    KnownInstanceType, MetaclassCandidate, MetaclassErrorKind, SliceLiteralType, StringLiteralType,
-    TupleType, Type, TypeArrayDisplay, UnionBuilder, UnionType,
+    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,
+                        candidate1:
-                        metaclass2
+                            MetaclassCandidate {
-                    } => self.diagnostics.add(
+                                metaclass: metaclass1,
-                        class.node(self.db).into(),
+                                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 (`{}`) must be a subclass of the metaclasses of all its bases, but `{}` and `{}` have no subclass relationship",
+                                    "The metaclass of a derived class (`{class}`) must be a subclass of the metaclasses of all its bases, \
-                            class.name(self.db),
+                                    but `{metaclass1}` (metaclass of base class `{base1}`) and `{metaclass2}` (metaclass of base class `{base2}`) \
-                            metaclass1.name(self.db),
+                                    have no subclass relationship",
-                            metaclass2.name(self.db),
+                                    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.