Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/types/bound_super.rs (Rust)

Code before:
            SuperOwnerKind::Class(class) => Some(SuperOwnerKind::Class(
                class.recursive_type_normalized_impl(db, div, nested)?,
            )),
            SuperOwnerKind::Instance(instance) => Some(SuperOwnerKind::Instance(
                instance.recursive_type_normalized_impl(db, div, nested)?,
            )),
            SuperOwnerKind::InstanceTypeVar(_) | SuperOwnerKind::ClassTypeVar(_) => Some(self),
        }
    }

    fn iter_mro(self, db: &'db dyn Db) -> impl Iterator<Item = ClassBase<'db>> {
        match self {
            SuperOwnerKind::Dynamic(dynamic) => {
                Either::Left(ClassBase::Dynamic(dynamic).mro(db, None))
            }
            SuperOwnerKind::Class(class) => Either::Right(Either::Left(class.iter_mro(db))),
            SuperOwnerKind::Instance(instance) => {
                Either::Right(Either::Left(instance.class(db).iter_mro(db)))
            }
            SuperOwnerKind::InstanceTypeVar(bound_typevar)
            | SuperOwnerKind::ClassTypeVar(bound_typevar) => {
                // Use the upper bound's class for MRO lookup.
                let class = match bound_typevar.typevar(db).bound_or_constraints(db) {
                    Some(TypeVarBoundOrConstraints::UpperBound(bound)) => match bound {
                        Type::NominalInstance(instance) => Some(instance.class(db)),
                        _ => bound.to_class_type(db),
                    },
                    _ => KnownClass::Object.to_class_literal(db).to_class_type(db),
                };
                match class {
                    Some(c) => Either::Right(Either::Left(c.iter_mro(db))),
                    None => Either::Right(Either::Right(std::iter::empty())),
                }
            }
        }
    }

    fn into_class(self, db: &'db dyn Db) -> Option<ClassType<'db>> {
        match self {
            SuperOwnerKind::Dynamic(_) => None,
            SuperOwnerKind::Class(class) => Some(class),
            SuperOwnerKind::Instance(instance) => Some(instance.class(db)),
            SuperOwnerKind::InstanceTypeVar(bound_typevar)
            | SuperOwnerKind::ClassTypeVar(bound_typevar) => {
                // Return the upper bound's class for subclass checking.
                match bound_typevar.typevar(db).bound_or_constraints(db) {
                    Some(TypeVarBoundOrConstraints::UpperBound(bound)) => match bound {
                        Type::NominalInstance(instance) => Some(instance.class(db)),
                        _ => bound.to_class_type(db),
                    },
                    _ => KnownClass::Object.to_class_literal(db).to_class_type(db),

Review comment (bug):
I feel like the logic here is a bit "loose" in a way that could bite us in the future (and is causing at least one undesirable behavior change in the tests). If I'm understanding right, the `NominalInstance` branch here should only ever occur if we have `SuperOwnerKind::InstanceTypeVar`, and the fallback branch should only ever occur if we have `SuperOwnerKind::ClassTypeVar`. If we had, for example, a `SuperOwnerKind::InstanceTypeVar` with a `SubclassOf` or `ClassLiteral` type in the upper bound, it would be wrong to treat that as if it were an instance-type upper bound class.

It feels to me like maybe we should be eagerly checking the upper bound when we create a typevar super, erroring if it doesn't look valid (this would be the `None` case below returning an empty MRO -- it seems a bit questionable to silently fall back when faced with a typevar upper bound that makes no sense for `super` -- this is causing the behavior change in `method10` in the tests), and maybe even just storing the extracted class then, so we don't have to re-extract it in multiple different places and deal with all the possibilities that we already checked up-front?


Code after:
        nested: bool,
    ) -> Option<Self> {
        match self {
            SuperOwnerKind::Dynamic(dynamic) => {
                Some(SuperOwnerKind::Dynamic(dynamic.recursive_type_normalized()))
            }
            SuperOwnerKind::Class(class) => Some(SuperOwnerKind::Class(
                class.recursive_type_normalized_impl(db, div, nested)?,
            )),
            SuperOwnerKind::Instance(instance) => Some(SuperOwnerKind::Instance(
                instance.recursive_type_normalized_impl(db, div, nested)?,
            )),
            SuperOwnerKind::InstanceTypeVar(_, _) | SuperOwnerKind::ClassTypeVar(_, _) => {
                // TODO: we might need to normalize the nested class here?
                Some(self)
            }
        }
    }

    fn iter_mro(self, db: &'db dyn Db) -> impl Iterator<Item = ClassBase<'db>> {
        match self {
            SuperOwnerKind::Dynamic(dynamic) => {
                Either::Left(ClassBase::Dynamic(dynamic).mro(db, None))
            }
            SuperOwnerKind::Class(class) => Either::Right(class.iter_mro(db)),
            SuperOwnerKind::Instance(instance) => Either::Right(instance.class(db).iter_mro(db)),
            SuperOwnerKind::InstanceTypeVar(_, class) | SuperOwnerKind::ClassTypeVar(_, class) => {
                Either::Right(class.iter_mro(db))
            }
        }
    }

    fn into_class(self, db: &'db dyn Db) -> Option<ClassType<'db>> {
        match self {
            SuperOwnerKind::Dynamic(_) => None,
            SuperOwnerKind::Class(class) => Some(class),
            SuperOwnerKind::Instance(instance) => Some(instance.class(db)),
            SuperOwnerKind::InstanceTypeVar(_, class) | SuperOwnerKind::ClassTypeVar(_, class) => {
                Some(class)
            }
        }
    }

    /// Returns the `TypeVar` instance if this owner is a `TypeVar` variant.
    fn typevar(self, db: &'db dyn Db) -> Option<TypeVarInstance<'db>> {
        match self {
            SuperOwnerKind::InstanceTypeVar(bound_typevar, _)
            | SuperOwnerKind::ClassTypeVar(bound_typevar, _) => Some(bound_typevar.typevar(db)),
            _ => None,
        }
    }