Simplifications

AlexWaygood · AlexWaygood · commit c167e476d284 · 2025-05-26T20:23:32.000+01:00
diff --git a/crates/ty_python_semantic/resources/mdtest/generics/legacy/classes.md b/crates/ty_python_semantic/resources/mdtest/generics/legacy/classes.md
@@ -37,7 +37,7 @@ class RepeatedTypevar(Generic[T, T]): ...
 You can only specialize `typing.Generic` with typevars (TODO: or param specs or typevar tuples).
 
 ```py
-# error: [invalid-argument-type] "`<class 'int'>` is not a valid argument to `typing.Generic`"
+# error: [invalid-argument-type] "`<class 'int'>` is not a valid argument to `Generic`"
 class GenericOfType(Generic[int]): ...
 ```
 
diff --git a/crates/ty_python_semantic/resources/mdtest/generics/pep695/classes.md b/crates/ty_python_semantic/resources/mdtest/generics/pep695/classes.md
@@ -67,6 +67,12 @@ T = TypeVar("T")
 
 # error: [invalid-generic-class] "Cannot both inherit from `typing.Generic` and use PEP 695 type variables"
 class BothGenericSyntaxes[U](Generic[T]): ...
+reveal_type(BothGenericSyntaxes.__mro__)  # revealed: tuple[<class 'BothGenericSyntaxes[Unknown]'>, Unknown, <class 'object'>]
+
+# error: [invalid-generic-class] "Cannot both inherit from `typing.Generic` and use PEP 695 type variables"
+# error: [invalid-base] "Cannot inherit from plain `Generic`"
+class DoublyInvalid[T](Generic): ...
+reveal_type(DoublyInvalid.__mro__)  # revealed: tuple[<class 'DoublyInvalid[Unknown]'>, Unknown, <class 'object'>]
 ```
 
 Generic classes implicitly inherit from `Generic`:
diff --git a/crates/ty_python_semantic/resources/mdtest/protocols.md b/crates/ty_python_semantic/resources/mdtest/protocols.md
@@ -58,9 +58,13 @@ class Bar1(Protocol[T], Generic[T]):
 class Bar2[T](Protocol):
     x: T
 
-# error: [invalid-generic-class] "Cannot both inherit from subscripted `typing.Protocol` and use PEP 695 type variables"
+# error: [invalid-generic-class] "Cannot both inherit from subscripted `Protocol` and use PEP 695 type variables"
 class Bar3[T](Protocol[T]):
     x: T
+
+# Note that this class definition *will* actually succeed at runtime,
+# unlike classes that combine PEP-695 type parameters with inheritance from `Generic[]`
+reveal_type(Bar3.__mro__)  # revealed: tuple[<class 'Bar3[Unknown]'>, typing.Protocol, typing.Generic, <class 'object'>]
 ```
 
 It's an error to include both bare `Protocol` and subscripted `Protocol[]` in the bases list
diff --git a/crates/ty_python_semantic/src/types/class.rs b/crates/ty_python_semantic/src/types/class.rs
@@ -580,10 +580,6 @@ impl<'db> ClassLiteral<'db> {
         self.pep695_generic_context(db).is_some()
     }
 
-    pub(crate) fn has_legacy_generic_context(self, db: &'db dyn Db) -> bool {
-        self.legacy_generic_context(db).is_some()
-    }
-
     #[salsa::tracked(cycle_fn=pep695_generic_context_cycle_recover, cycle_initial=pep695_generic_context_cycle_initial)]
     pub(crate) fn pep695_generic_context(self, db: &'db dyn Db) -> Option<GenericContext<'db>> {
         let scope = self.body_scope(db);
diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs
@@ -27,7 +27,6 @@ use crate::{Db, FxOrderSet};
 pub struct GenericContext<'db> {
     #[returns(ref)]
     pub(crate) variables: FxOrderSet<TypeVarInstance<'db>>,
-    pub(crate) origin: GenericContextOrigin,
 }
 
 impl<'db> GenericContext<'db> {
@@ -41,7 +40,7 @@ impl<'db> GenericContext<'db> {
             .iter()
             .filter_map(|type_param| Self::variable_from_type_param(db, index, type_param))
             .collect();
-        Self::new(db, variables, GenericContextOrigin::TypeParameterList)
+        Self::new(db, variables)
     }
 
     fn variable_from_type_param(
@@ -87,11 +86,7 @@ impl<'db> GenericContext<'db> {
         if variables.is_empty() {
             return None;
         }
-        Some(Self::new(
-            db,
-            variables,
-            GenericContextOrigin::LegacyGenericFunction,
-        ))
+        Some(Self::new(db, variables))
     }
 
     /// Creates a generic context from the legacy `TypeVar`s that appear in class's base class
@@ -107,7 +102,7 @@ impl<'db> GenericContext<'db> {
         if variables.is_empty() {
             return None;
         }
-        Some(Self::new(db, variables, GenericContextOrigin::Inherited))
+        Some(Self::new(db, variables))
     }
 
     pub(crate) fn len(self, db: &'db dyn Db) -> usize {
@@ -244,46 +239,21 @@ impl<'db> GenericContext<'db> {
             .iter()
             .map(|ty| ty.normalized(db))
             .collect();
-        Self::new(db, variables, self.origin(db))
-    }
-}
-
-#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
-pub enum GenericContextOrigin {
-    LegacyBase(LegacyGenericBase),
-    Inherited,
-    LegacyGenericFunction,
-    TypeParameterList,
-}
-
-impl GenericContextOrigin {
-    pub(crate) const fn as_str(self) -> &'static str {
-        match self {
-            Self::LegacyBase(base) => base.as_str(),
-            Self::Inherited => "inherited",
-            Self::LegacyGenericFunction => "legacy generic function",
-            Self::TypeParameterList => "type parameter list",
-        }
+        Self::new(db, variables)
     }
 }
 
-impl std::fmt::Display for GenericContextOrigin {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str(self.as_str())
-    }
-}
-
-#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
-pub enum LegacyGenericBase {
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub(super) enum LegacyGenericBase {
     Generic,
     Protocol,
 }
 
 impl LegacyGenericBase {
-    pub(crate) const fn as_str(self) -> &'static str {
+    const fn as_str(self) -> &'static str {
         match self {
-            Self::Generic => "`typing.Generic`",
-            Self::Protocol => "subscripted `typing.Protocol`",
+            Self::Generic => "Generic",
+            Self::Protocol => "Protocol",
         }
     }
 }
@@ -294,12 +264,6 @@ impl std::fmt::Display for LegacyGenericBase {
     }
 }
 
-impl From<LegacyGenericBase> for GenericContextOrigin {
-    fn from(base: LegacyGenericBase) -> Self {
-        Self::LegacyBase(base)
-    }
-}
-
 /// An assignment of a specific type to each type variable in a generic scope.
 ///
 /// TODO: Handle nested specializations better, with actual parent links to the specialization of
diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
@@ -108,7 +108,7 @@ use super::diagnostic::{
     report_runtime_check_against_non_runtime_checkable_protocol, report_slice_step_size_zero,
     report_unresolved_reference,
 };
-use super::generics::{GenericContextOrigin, LegacyGenericBase};
+use super::generics::LegacyGenericBase;
 use super::slots::check_class_slots;
 use super::string_annotation::{
     BYTE_STRING_TYPE_ANNOTATION, FSTRING_TYPE_ANNOTATION, parse_string_annotation,
@@ -856,6 +856,25 @@ impl<'db> TypeInferenceBuilder<'db> {
                         }
                         continue;
                     }
+                    // Note that unlike several of the other errors caught in this function,
+                    // this does not lead to the class creation failing at runtime,
+                    // but it is semantically invalid.
+                    Type::KnownInstance(KnownInstanceType::Protocol(Some(_))) => {
+                        if class_node.type_params.is_none() {
+                            continue;
+                        }
+                        let Some(builder) = self
+                            .context
+                            .report_lint(&INVALID_GENERIC_CLASS, &class_node.bases()[i])
+                        else {
+                            continue;
+                        };
+                        builder.into_diagnostic(
+                            "Cannot both inherit from subscripted `Protocol` \
+                            and use PEP 695 type variables",
+                        );
+                        continue;
+                    }
                     Type::ClassLiteral(class) => class,
                     // dynamic/unknown bases are never `@final`
                     _ => continue,
@@ -912,27 +931,28 @@ impl<'db> TypeInferenceBuilder<'db> {
                         }
                     }
                     MroErrorKind::UnresolvableMro { bases_list } => {
-                        let has_conflicting_generic_contexts = class
-                            .has_pep_695_type_params(self.db())
-                            && class.has_legacy_generic_context(self.db());
-
-                        // Skip reporting the error if the class has conflicting generic contexts.
-                        // We'll report an error below for that with a nicer error message;
-                        // there's not much need to have two diagnostics for the same underlying problem.
-                        if !has_conflicting_generic_contexts {
-                            if let Some(builder) =
-                                self.context.report_lint(&INCONSISTENT_MRO, class_node)
-                            {
-                                builder.into_diagnostic(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(", ")
-                                ));
-                            }
+                        if let Some(builder) =
+                            self.context.report_lint(&INCONSISTENT_MRO, class_node)
+                        {
+                            builder.into_diagnostic(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::Pep695ClassWithGenericInheritance => {
+                        if let Some(builder) =
+                            self.context.report_lint(&INVALID_GENERIC_CLASS, class_node)
+                        {
+                            builder.into_diagnostic(
+                                "Cannot both inherit from `typing.Generic` \
+                                and use PEP 695 type variables",
+                            );
                         }
                     }
                     MroErrorKind::InheritanceCycle => {
@@ -1031,21 +1051,6 @@ impl<'db> TypeInferenceBuilder<'db> {
                 }
             }
 
-            // (5) Check that a generic class does not have invalid or conflicting generic
-            // contexts.
-            if class.pep695_generic_context(self.db()).is_some() {
-                if let Some(legacy_context) = class.legacy_generic_context(self.db()) {
-                    if let Some(builder) =
-                        self.context.report_lint(&INVALID_GENERIC_CLASS, class_node)
-                    {
-                        builder.into_diagnostic(format_args!(
-                            "Cannot both inherit from {} and use PEP 695 type variables",
-                            legacy_context.origin(self.db())
-                        ));
-                    }
-                }
-            }
-
             if let (Some(legacy), Some(inherited)) = (
                 class.legacy_generic_context(self.db()),
                 class.inherited_legacy_generic_context(self.db()),
@@ -7631,17 +7636,15 @@ impl<'db> TypeInferenceBuilder<'db> {
                         self.context.report_lint(&INVALID_ARGUMENT_TYPE, value_node)
                     {
                         builder.into_diagnostic(format_args!(
-                            "`{}` is not a valid argument to {origin}",
+                            "`{}` is not a valid argument to `{origin}`",
                             typevar.display(self.db()),
                         ));
                     }
                     None
                 }
             })
             .collect();
-        typevars.map(|typevars| {
-            GenericContext::new(self.db(), typevars, GenericContextOrigin::from(origin))
-        })
+        typevars.map(|typevars| GenericContext::new(self.db(), typevars))
     }
 
     fn infer_slice_expression(&mut self, slice: &ast::ExprSlice) -> Type<'db> {
diff --git a/crates/ty_python_semantic/src/types/mro.rs b/crates/ty_python_semantic/src/types/mro.rs
@@ -72,8 +72,21 @@ impl<'db> Mro<'db> {
     ) -> Result<Self, MroErrorKind<'db>> {
         /// Possibly add `Generic` to the resolved bases list.
         ///
-        /// This emulates the behavior of `typing._GenericAlias.__mro_entries__` at
+        /// This function is called in two cases:
+        /// - If we encounter a subscripted `Generic` in the original bases list
+        ///   (`Generic[T]` or similar)
+        /// - If the class has PEP-695 type parameters,
+        ///   `Generic` is [implicitly appended] to the bases list at runtime
+        ///
+        /// Whether or not `Generic` is added to the bases list depends on:
+        /// - Whether `Protocol` is present in the original bases list
+        /// - Whether any of the bases yet to be visited in the original bases list
+        ///   is a generic alias (which would therefore have `Generic` in its MRO)
+        ///
+        /// This function emulates the behavior of `typing._GenericAlias.__mro_entries__` at
         /// <https://github.com/python/cpython/blob/ad42dc1909bdf8ec775b63fb22ed48ff42797a17/Lib/typing.py#L1487-L1500>.
+        ///
+        /// [implicitly inherits]: https://docs.python.org/3/reference/compound_stmts.html#generic-classes
         fn maybe_add_generic<'db>(
             resolved_bases: &mut Vec<ClassBase<'db>>,
             original_bases: &[Type<'db>],
@@ -161,7 +174,7 @@ impl<'db> Mro<'db> {
                 let mut invalid_bases = vec![];
 
                 for (i, base) in original_bases.iter().enumerate() {
-                    // Note that emit a diagnostic for inheriting from bare (unsubscripted) `Generic` elsewhere
+                    // Note that we emit a diagnostic for inheriting from bare (unsubscripted) `Generic` elsewhere
                     // (see `infer::TypeInferenceBuilder::check_class_definitions`),
                     // which is why we only care about `KnownInstanceType::Generic(Some(_))`,
                     // not `KnownInstanceType::Generic(None)`.
@@ -184,6 +197,7 @@ impl<'db> Mro<'db> {
                 }
 
                 // `Generic` is implicitly added to the bases list of a class that has PEP-695 type parameters
+                // (documented at https://docs.python.org/3/reference/compound_stmts.html#generic-classes)
                 if class.has_pep_695_type_params(db) {
                     maybe_add_generic(&mut resolved_bases, original_bases, &[]);
                 }
@@ -206,6 +220,18 @@ impl<'db> Mro<'db> {
                     return Ok(mro);
                 }
 
+                // We now know that the MRO is unresolvable through the C3-merge algorithm.
+                // The rest of this function is dedicated to figuring out the best error message
+                // to report to the user.
+
+                if class.has_pep_695_type_params(db)
+                    && original_bases.iter().any(|base| {
+                        matches!(base, Type::KnownInstance(KnownInstanceType::Generic(_)))
+                    })
+                {
+                    return Err(MroErrorKind::Pep695ClassWithGenericInheritance);
+                }
+
                 let mut duplicate_dynamic_bases = false;
 
                 let duplicate_bases: Vec<DuplicateBaseError<'db>> = {
@@ -430,6 +456,9 @@ pub(super) enum MroErrorKind<'db> {
     /// See [`DuplicateBaseError`] for more details.
     DuplicateBases(Box<[DuplicateBaseError<'db>]>),
 
+    /// The class uses PEP-695 parameters and also inherits from `Generic[]`.
+    Pep695ClassWithGenericInheritance,
+
     /// A cycle was encountered resolving the class' bases.
     InheritanceCycle,
 
