[ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822)

Generic classes are not allowed to bind or reference a typevar from an
enclosing scope:

```py
def f[T](x: T, y: T) -> None:
    class Ok[S]: ...
    # error: [invalid-generic-class]
    class Bad1[T]: ...
    # error: [invalid-generic-class]
    class Bad2(Iterable[T]): ...

class C[T]:
    class Ok1[S]: ...
    # error: [invalid-generic-class]
    class Bad1[T]: ...
    # error: [invalid-generic-class]
    class Bad2(Iterable[T]): ...
```

It does not matter if the class uses PEP 695 or legacy syntax. It does
not matter if the enclosing scope is a generic class or function. The
generic class cannot even _reference_ an enclosing typevar in its base
class list.

This PR adds diagnostics for these cases.

In addition, the PR adds better fallback behavior for generic classes
that violate this rule: any enclosing typevars are not included in the
class's generic context. (That ensures that we don't inadvertently try
to infer specializations for those typevars in places where we
shouldn't.) The `dulwich` ecosystem project has [examples of
this](d912eaaffd/dulwich/config.py (L251))
that were causing new false positives on #20677.

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

crates/ty_python_semantic/src/types.rs

@@ -8210,12 +8210,16 @@ impl<'db> From<Definition<'db>> for BindingContext<'db> {
 }
 
 impl<'db> BindingContext<'db> {
-    fn name(self, db: &'db dyn Db) -> Option<String> {
+    pub(crate) fn definition(self) -> Option<Definition<'db>> {
         match self {
-            BindingContext::Definition(definition) => definition.name(db),
+            BindingContext::Definition(definition) => Some(definition),
             BindingContext::Synthetic => None,
         }
     }
+
+    fn name(self, db: &'db dyn Db) -> Option<String> {
+        self.definition().and_then(|definition| definition.name(db))
+    }
 }
 
 /// A type variable that has been bound to a generic context, and which can be specialized to a

crates/ty_python_semantic/src/types/class.rs

@@ -1,3 +1,4 @@
+use std::cell::RefCell;
 use std::sync::{LazyLock, Mutex};
 
 use super::TypeVarVariance;
@@ -20,12 +21,15 @@ use crate::types::context::InferContext;
 use crate::types::diagnostic::INVALID_TYPE_ALIAS_TYPE;
 use crate::types::enums::enum_metadata;
 use crate::types::function::{DataclassTransformerParams, KnownFunction};
-use crate::types::generics::{GenericContext, Specialization, walk_specialization};
+use crate::types::generics::{
+    GenericContext, Specialization, walk_generic_context, walk_specialization,
+};
 use crate::types::infer::nearest_enclosing_class;
 use crate::types::member::{Member, class_member};
 use crate::types::signatures::{CallableSignature, Parameter, Parameters, Signature};
 use crate::types::tuple::{TupleSpec, TupleType};
 use crate::types::typed_dict::typed_dict_params_from_class_def;
+use crate::types::visitor::{NonAtomicType, TypeKind, TypeVisitor, walk_non_atomic_type};
 use crate::types::{
     ApplyTypeMappingVisitor, Binding, BoundSuperType, CallableType, DataclassParams,
     DeprecatedInstance, FindLegacyTypeVarsVisitor, HasRelationToVisitor, IsDisjointVisitor,
@@ -35,7 +39,7 @@ use crate::types::{
     determine_upper_bound, infer_definition_types,
 };
 use crate::{
-    Db, FxIndexMap, FxOrderSet, Program,
+    Db, FxIndexMap, FxIndexSet, FxOrderSet, Program,
     module_resolver::file_to_module,
     place::{
         Boundness, LookupError, LookupResult, Place, PlaceAndQualifiers, known_module_symbol,
@@ -1382,7 +1386,7 @@ impl get_size2::GetSize for ClassLiteral<'_> {}
 
 #[expect(clippy::ref_option)]
 #[allow(clippy::trivially_copy_pass_by_ref)]
-fn pep695_generic_context_cycle_recover<'db>(
+fn generic_context_cycle_recover<'db>(
     _db: &'db dyn Db,
     _value: &Option<GenericContext<'db>>,
     _count: u32,
@@ -1391,7 +1395,7 @@ fn pep695_generic_context_cycle_recover<'db>(
     salsa::CycleRecoveryAction::Iterate
 }
 
-fn pep695_generic_context_cycle_initial<'db>(
+fn generic_context_cycle_initial<'db>(
     _db: &'db dyn Db,
     _self: ClassLiteral<'db>,
 ) -> Option<GenericContext<'db>> {
@@ -1431,7 +1435,11 @@ impl<'db> ClassLiteral<'db> {
         self.pep695_generic_context(db).is_some()
     }
 
-    #[salsa::tracked(cycle_fn=pep695_generic_context_cycle_recover, cycle_initial=pep695_generic_context_cycle_initial, heap_size=ruff_memory_usage::heap_size)]
+    #[salsa::tracked(
+        cycle_fn=generic_context_cycle_recover,
+        cycle_initial=generic_context_cycle_initial,
+        heap_size=ruff_memory_usage::heap_size,
+    )]
     pub(crate) fn pep695_generic_context(self, db: &'db dyn Db) -> Option<GenericContext<'db>> {
         let scope = self.body_scope(db);
         let file = scope.file(db);
@@ -1454,12 +1462,18 @@ impl<'db> ClassLiteral<'db> {
         })
     }
 
+    #[salsa::tracked(
+        cycle_fn=generic_context_cycle_recover,
+        cycle_initial=generic_context_cycle_initial,
+        heap_size=ruff_memory_usage::heap_size,
+    )]
     pub(crate) fn inherited_legacy_generic_context(
         self,
         db: &'db dyn Db,
     ) -> Option<GenericContext<'db>> {
         GenericContext::from_base_classes(
             db,
+            self.definition(db),
             self.explicit_bases(db)
                 .iter()
                 .copied()
@@ -1467,6 +1481,57 @@ impl<'db> ClassLiteral<'db> {
         )
     }
 
+    /// Returns all of the typevars that are referenced in this class's definition. This includes
+    /// any typevars bound in its generic context, as well as any typevars mentioned in its base
+    /// class list. (This is used to ensure that classes do not bind or reference typevars from
+    /// enclosing generic contexts.)
+    pub(crate) fn typevars_referenced_in_definition(
+        self,
+        db: &'db dyn Db,
+    ) -> FxIndexSet<BoundTypeVarInstance<'db>> {
+        #[derive(Default)]
+        struct CollectTypeVars<'db> {
+            typevars: RefCell<FxIndexSet<BoundTypeVarInstance<'db>>>,
+            seen_types: RefCell<FxIndexSet<NonAtomicType<'db>>>,
+        }
+
+        impl<'db> TypeVisitor<'db> for CollectTypeVars<'db> {
+            fn should_visit_lazy_type_attributes(&self) -> bool {
+                false
+            }
+
+            fn visit_bound_type_var_type(
+                &self,
+                _db: &'db dyn Db,
+                bound_typevar: BoundTypeVarInstance<'db>,
+            ) {
+                self.typevars.borrow_mut().insert(bound_typevar);
+            }
+
+            fn visit_type(&self, db: &'db dyn Db, ty: Type<'db>) {
+                match TypeKind::from(ty) {
+                    TypeKind::Atomic => {}
+                    TypeKind::NonAtomic(non_atomic_type) => {
+                        if !self.seen_types.borrow_mut().insert(non_atomic_type) {
+                            // If we have already seen this type, we can skip it.
+                            return;
+                        }
+                        walk_non_atomic_type(db, non_atomic_type, self);
+                    }
+                }
+            }
+        }
+
+        let visitor = CollectTypeVars::default();
+        if let Some(generic_context) = self.generic_context(db) {
+            walk_generic_context(db, generic_context, &visitor);
+        }
+        for base in self.explicit_bases(db) {
+            visitor.visit_type(db, *base);
+        }
+        visitor.typevars.into_inner()
+    }
+
     /// Returns the generic context that should be inherited by any constructor methods of this
     /// class.
     ///

crates/ty_python_semantic/src/types/context.rs

@@ -100,6 +100,10 @@ impl<'db, 'ast> InferContext<'db, 'ast> {
         self.diagnostics.get_mut().extend(other);
     }
 
+    pub(super) fn is_lint_enabled(&self, lint: &'static LintMetadata) -> bool {
+        LintDiagnosticGuardBuilder::severity_and_source(self, lint).is_some()
+    }
+
     /// Optionally return a builder for a lint diagnostic guard.
     ///
     /// If the current context believes a diagnostic should be reported for
@@ -396,11 +400,10 @@ pub(super) struct LintDiagnosticGuardBuilder<'db, 'ctx> {
 }
 
 impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> {
-    fn new(
+    fn severity_and_source(
         ctx: &'ctx InferContext<'db, 'ctx>,
         lint: &'static LintMetadata,
-        range: TextRange,
-    ) -> Option<LintDiagnosticGuardBuilder<'db, 'ctx>> {
+    ) -> Option<(Severity, LintSource)> {
         // The comment below was copied from the original
         // implementation of diagnostic reporting. The code
         // has been refactored, but this still kind of looked
@@ -429,14 +432,25 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> {
         if ctx.is_in_multi_inference() {
             return None;
         }
-        let id = DiagnosticId::Lint(lint.name());
+
+        Some((severity, source))
+    }
+
+    fn new(
+        ctx: &'ctx InferContext<'db, 'ctx>,
+        lint: &'static LintMetadata,
+        range: TextRange,
+    ) -> Option<LintDiagnosticGuardBuilder<'db, 'ctx>> {
+        let (severity, source) = Self::severity_and_source(ctx, lint)?;
 
         let suppressions = suppressions(ctx.db(), ctx.file());
+        let lint_id = LintId::of(lint);
         if let Some(suppression) = suppressions.find_suppression(range, lint_id) {
             ctx.diagnostics.borrow_mut().mark_used(suppression.id());
             return None;
         }
 
+        let id = DiagnosticId::Lint(lint.name());
         let primary_span = Span::from(ctx.file()).with_range(range);
         Some(LintDiagnosticGuardBuilder {
             ctx,

crates/ty_python_semantic/src/types/diagnostic.rs

@@ -18,10 +18,10 @@ use crate::types::string_annotation::{
     RAW_STRING_TYPE_ANNOTATION,
 };
 use crate::types::{
-    ClassType, DynamicType, LintDiagnosticGuard, Protocol, ProtocolInstanceType, SubclassOfInner,
-    TypeContext, binding_type, infer_isolated_expression,
+    BoundTypeVarInstance, ClassType, DynamicType, LintDiagnosticGuard, Protocol,
+    ProtocolInstanceType, SpecialFormType, SubclassOfInner, Type, TypeContext, binding_type,
+    infer_isolated_expression, protocol_class::ProtocolClass,
 };
-use crate::types::{SpecialFormType, Type, protocol_class::ProtocolClass};
 use crate::util::diagnostics::format_enumeration;
 use crate::{
     Db, DisplaySettings, FxIndexMap, FxOrderMap, Module, ModuleName, Program, declare_lint,
@@ -3055,6 +3055,39 @@ pub(crate) fn report_cannot_pop_required_field_on_typed_dict<'db>(
     }
 }
 
+pub(crate) fn report_rebound_typevar<'db>(
+    context: &InferContext<'db, '_>,
+    typevar_name: &ast::name::Name,
+    class: ClassLiteral<'db>,
+    class_node: &ast::StmtClassDef,
+    other_typevar: BoundTypeVarInstance<'db>,
+) {
+    let db = context.db();
+    let Some(builder) = context.report_lint(&INVALID_GENERIC_CLASS, class.header_range(db)) else {
+        return;
+    };
+    let mut diagnostic = builder.into_diagnostic(format_args!(
+        "Generic class `{}` must not reference type variables bound in an enclosing scope",
+        class_node.name,
+    ));
+    diagnostic.set_primary_message(format_args!(
+        "`{typevar_name}` referenced in class definition here"
+    ));
+    let Some(other_definition) = other_typevar.binding_context(db).definition() else {
+        return;
+    };
+    let span = match binding_type(db, other_definition) {
+        Type::ClassLiteral(class) => Some(class.header_span(db)),
+        Type::FunctionLiteral(function) => function.spans(db).map(|spans| spans.signature),
+        _ => return,
+    };
+    if let Some(span) = span {
+        diagnostic.annotate(Annotation::secondary(span).message(format_args!(
+            "Type variable `{typevar_name}` is bound in this enclosing scope",
+        )));
+    }
+}
+
 /// This function receives an unresolved `from foo import bar` import,
 /// where `foo` can be resolved to a module but that module does not
 /// have a `bar` member or submodule.

crates/ty_python_semantic/src/types/generics.rs

@@ -24,7 +24,7 @@ use crate::{Db, FxOrderMap, FxOrderSet};
 
 /// Returns an iterator of any generic context introduced by the given scope or any enclosing
 /// scope.
-fn enclosing_generic_contexts<'db>(
+pub(crate) fn enclosing_generic_contexts<'db>(
     db: &'db dyn Db,
     index: &SemanticIndex<'db>,
     scope: FileScopeId,
@@ -317,11 +317,12 @@ impl<'db> GenericContext<'db> {
     /// list.
     pub(crate) fn from_base_classes(
         db: &'db dyn Db,
+        definition: Definition<'db>,
         bases: impl Iterator<Item = Type<'db>>,
     ) -> Option<Self> {
         let mut variables = FxOrderSet::default();
         for base in bases {
-            base.find_legacy_typevars(db, None, &mut variables);
+            base.find_legacy_typevars(db, Some(definition), &mut variables);
         }
         if variables.is_empty() {
             return None;
@@ -413,6 +414,15 @@ impl<'db> GenericContext<'db> {
             .all(|bound_typevar| other_variables.contains_key(&bound_typevar))
     }
 
+    pub(crate) fn binds_named_typevar(
+        self,
+        db: &'db dyn Db,
+        name: &'db ast::name::Name,
+    ) -> Option<BoundTypeVarInstance<'db>> {
+        self.variables(db)
+            .find(|self_bound_typevar| self_bound_typevar.typevar(db).name(db) == name)
+    }
+
     pub(crate) fn binds_typevar(
         self,
         db: &'db dyn Db,

crates/ty_python_semantic/src/types/infer/builder.rs

@@ -53,31 +53,29 @@ use crate::types::diagnostic::{
     CALL_NON_CALLABLE, CONFLICTING_DECLARATIONS, CONFLICTING_METACLASS, CYCLIC_CLASS_DEFINITION,
     DIVISION_BY_ZERO, DUPLICATE_KW_ONLY, INCONSISTENT_MRO, INVALID_ARGUMENT_TYPE,
     INVALID_ASSIGNMENT, INVALID_ATTRIBUTE_ACCESS, INVALID_BASE, INVALID_DECLARATION,
-    INVALID_GENERIC_CLASS, INVALID_KEY, INVALID_LEGACY_TYPE_VARIABLE, INVALID_NAMED_TUPLE,
-    INVALID_PARAMETER_DEFAULT, INVALID_TYPE_FORM, INVALID_TYPE_GUARD_CALL,
-    INVALID_TYPE_VARIABLE_CONSTRAINTS, IncompatibleBases, NON_SUBSCRIPTABLE,
-    POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT, UNDEFINED_REVEAL,
-    UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL, UNRESOLVED_IMPORT, UNRESOLVED_REFERENCE,
-    UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY, report_bad_dunder_set_call,
-    report_cannot_pop_required_field_on_typed_dict, report_implicit_return_type,
-    report_instance_layout_conflict, report_invalid_assignment,
-    report_invalid_attribute_assignment, report_invalid_generator_function_return_type,
-    report_invalid_key_on_typed_dict, report_invalid_return_type,
-    report_namedtuple_field_without_default_after_field_with_default,
-    report_possibly_missing_attribute,
-};
-use crate::types::diagnostic::{
-    INVALID_METACLASS, INVALID_OVERLOAD, INVALID_PROTOCOL, SUBCLASS_OF_FINAL_CLASS,
+    INVALID_GENERIC_CLASS, INVALID_KEY, INVALID_LEGACY_TYPE_VARIABLE, INVALID_METACLASS,
+    INVALID_NAMED_TUPLE, INVALID_OVERLOAD, INVALID_PARAMETER_DEFAULT, INVALID_PROTOCOL,
+    INVALID_TYPE_FORM, INVALID_TYPE_GUARD_CALL, INVALID_TYPE_VARIABLE_CONSTRAINTS,
+    IncompatibleBases, NON_SUBSCRIPTABLE, POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT,
+    SUBCLASS_OF_FINAL_CLASS, UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL,
+    UNRESOLVED_IMPORT, UNRESOLVED_REFERENCE, UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY,
     hint_if_stdlib_submodule_exists_on_other_versions, report_attempted_protocol_instantiation,
-    report_duplicate_bases, report_index_out_of_bounds, report_invalid_exception_caught,
+    report_bad_dunder_set_call, report_cannot_pop_required_field_on_typed_dict,
+    report_duplicate_bases, report_implicit_return_type, report_index_out_of_bounds,
+    report_instance_layout_conflict, report_invalid_assignment,
+    report_invalid_attribute_assignment, report_invalid_exception_caught,
     report_invalid_exception_cause, report_invalid_exception_raised,
-    report_invalid_or_unsupported_base, report_invalid_type_checking_constant,
-    report_non_subscriptable, report_possibly_unresolved_reference, report_slice_step_size_zero,
+    report_invalid_generator_function_return_type, report_invalid_key_on_typed_dict,
+    report_invalid_or_unsupported_base, report_invalid_return_type,
+    report_invalid_type_checking_constant,
+    report_namedtuple_field_without_default_after_field_with_default, report_non_subscriptable,
+    report_possibly_missing_attribute, report_possibly_unresolved_reference,
+    report_rebound_typevar, report_slice_step_size_zero,
 };
 use crate::types::function::{
     FunctionDecorators, FunctionLiteral, FunctionType, KnownFunction, OverloadLiteral,
 };
-use crate::types::generics::{GenericContext, bind_typevar};
+use crate::types::generics::{GenericContext, bind_typevar, enclosing_generic_contexts};
 use crate::types::generics::{LegacyGenericBase, SpecializationBuilder};
 use crate::types::infer::nearest_enclosing_function;
 use crate::types::instance::SliceLiteral;
@@ -838,6 +836,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 }
             }
 
+            // (6) If the class is generic, verify that its generic context does not violate any of
+            // the typevar scoping rules.
             if let (Some(legacy), Some(inherited)) = (
                 class.legacy_generic_context(self.db()),
                 class.inherited_legacy_generic_context(self.db()),
@@ -854,7 +854,29 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 }
             }
 
-            // (6) Check that a dataclass does not have more than one `KW_ONLY`.
+            let scope = class.body_scope(self.db()).scope(self.db());
+            if self.context.is_lint_enabled(&INVALID_GENERIC_CLASS)
+                && let Some(parent) = scope.parent()
+            {
+                for self_typevar in class.typevars_referenced_in_definition(self.db()) {
+                    let self_typevar_name = self_typevar.typevar(self.db()).name(self.db());
+                    for enclosing in enclosing_generic_contexts(self.db(), self.index, parent) {
+                        if let Some(other_typevar) =
+                            enclosing.binds_named_typevar(self.db(), self_typevar_name)
+                        {
+                            report_rebound_typevar(
+                                &self.context,
+                                self_typevar_name,
+                                class,
+                                class_node,
+                                other_typevar,
+                            );
+                        }
+                    }
+                }
+            }
+
+            // (7) Check that a dataclass does not have more than one `KW_ONLY`.
             if let Some(field_policy @ CodeGeneratorKind::DataclassLike(_)) =
                 CodeGeneratorKind::from_class(self.db(), class)
             {