mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-09-30 13:51:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[ty] Do not carry the generic context of Protocol or Generic in the ClassBase enum (#17989)

Browse source 
## Summary

It doesn't seem to be necessary for our generics implementation to carry
the `GenericContext` in the `ClassBase` variants. Removing it simplifies
the code, fixes many TODOs about `Generic` or `Protocol` appearing
multiple times in MROs when each should only appear at most once, and
allows us to more accurately detect runtime errors that occur due to
`Generic` or `Protocol` appearing multiple times in a class's bases.

In order to remove the `GenericContext` from the `ClassBase` variant, it
turns out to be necessary to emulate
`typing._GenericAlias.__mro_entries__`, or we end up with a large number
of false-positive `inconsistent-mro` errors. This PR therefore also does
that.

Lastly, this PR fixes the inferred MROs of PEP-695 generic classes,
which implicitly inherit from `Generic` even if they have no explicit
bases.

## Test Plan

mdtests
This commit is contained in:
Alex Waygood 2025-05-22 21:37:03 -04:00 • committed by GitHub
parent 6c0a59ea78
commit d02c9ada5d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
15 changed files with 271 additions and 215 deletions
Download patch file Download diff file Expand all files Collapse all files

4
crates/ty_python_semantic/src/types.rs
View file

@ -598,6 +598,10 @@ impl<'db> Type<'db> {
matches!(self, Type::Dynamic(DynamicType::Todo(_)))
}
pub const fn is_generic_alias(&self) -> bool {
matches!(self, Type::GenericAlias(_))
}
/// Replace references to the class `class` with a self-reference marker. This is currently
/// used for recursive protocols, but could probably be extended to self-referential type-
/// aliases and similar.

17
crates/ty_python_semantic/src/types/class.rs
View file

@ -223,6 +223,10 @@ impl<'db> ClassType<'db> {
}
}
pub(super) const fn is_generic(self) -> bool {
matches!(self, Self::Generic(_))
}
/// Returns the class literal and specialization for this class. For a non-generic class, this
/// is the class itself. For a generic alias, this is the alias's origin.
pub(crate) fn class_literal(
@ -352,7 +356,7 @@ impl<'db> ClassType<'db> {
ClassBase::Dynamic(_) => false,
// Protocol and Generic are not represented by a ClassType.
ClassBase::Protocol(_) | ClassBase::Generic(_) => false,
ClassBase::Protocol | ClassBase::Generic => false,
ClassBase::Class(base) => match (base, other) {
(ClassType::NonGeneric(base), ClassType::NonGeneric(other)) => base == other,
@ -390,7 +394,7 @@ impl<'db> ClassType<'db> {
ClassBase::Dynamic(_) => false,
// Protocol and Generic are not represented by a ClassType.
ClassBase::Protocol(_) | ClassBase::Generic(_) => false,
ClassBase::Protocol | ClassBase::Generic => false,
ClassBase::Class(base) => match (base, other) {
(ClassType::NonGeneric(base), ClassType::NonGeneric(other)) => base == other,
@ -602,11 +606,6 @@ impl<'db> ClassLiteral<'db> {
)
}
/// Return `true` if this class represents the builtin class `object`
pub(crate) fn is_object(self, db: &'db dyn Db) -> bool {
self.is_known(db, KnownClass::Object)
}
fn file(self, db: &dyn Db) -> File {
self.body_scope(db).file(db)
}
@ -1068,7 +1067,7 @@ impl<'db> ClassLiteral<'db> {
for superclass in mro_iter {
match superclass {
ClassBase::Generic(_) | ClassBase::Protocol(_) => {
ClassBase::Generic | ClassBase::Protocol => {
// Skip over these very special class bases that aren't really classes.
}
ClassBase::Dynamic(_) => {
@ -1427,7 +1426,7 @@ impl<'db> ClassLiteral<'db> {
for superclass in self.iter_mro(db, specialization) {
match superclass {
ClassBase::Generic(_) | ClassBase::Protocol(_) => {
ClassBase::Generic | ClassBase::Protocol => {
// Skip over these very special class bases that aren't really classes.
}
ClassBase::Dynamic(_) => {

84
crates/ty_python_semantic/src/types/class_base.rs
View file

@ -1,5 +1,5 @@
use crate::Db;
use crate::types::generics::{GenericContext, Specialization};
use crate::types::generics::Specialization;
use crate::types::{
ClassType, DynamicType, KnownClass, KnownInstanceType, MroError, MroIterator, Type,
TypeMapping, todo_type,
@ -19,11 +19,11 @@ pub enum ClassBase<'db> {
Class(ClassType<'db>),
/// Although `Protocol` is not a class in typeshed's stubs, it is at runtime,
/// and can appear in the MRO of a class.
Protocol(Option<GenericContext<'db>>),
Protocol,
/// Bare `Generic` cannot be subclassed directly in user code,
/// but nonetheless appears in the MRO of classes that inherit from `Generic[T]`,
/// `Protocol[T]`, or bare `Protocol`.
Generic(Option<GenericContext<'db>>),
Generic,
}
impl<'db> ClassBase<'db> {
@ -35,60 +35,18 @@ impl<'db> ClassBase<'db> {
match self {
Self::Dynamic(dynamic) => Self::Dynamic(dynamic.normalized()),
Self::Class(class) => Self::Class(class.normalized(db)),
Self::Protocol(generic_context) => {
Self::Protocol(generic_context.map(|context| context.normalized(db)))
}
Self::Generic(generic_context) => {
Self::Generic(generic_context.map(|context| context.normalized(db)))
}
Self::Protocol | Self::Generic => self,
}
}
pub(crate) fn display(self, db: &'db dyn Db) -> impl std::fmt::Display + 'db {
struct Display<'db> {
base: ClassBase<'db>,
db: &'db dyn Db,
}
impl std::fmt::Display for Display<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self.base {
ClassBase::Dynamic(dynamic) => dynamic.fmt(f),
ClassBase::Class(class @ ClassType::NonGeneric(_)) => {
write!(f, "<class '{}'>", class.name(self.db))
}
ClassBase::Class(ClassType::Generic(alias)) => {
write!(f, "<class '{}'>", alias.display(self.db))
}
ClassBase::Protocol(generic_context) => {
f.write_str("typing.Protocol")?;
if let Some(generic_context) = generic_context {
generic_context.display(self.db).fmt(f)?;
}
Ok(())
}
ClassBase::Generic(generic_context) => {
f.write_str("typing.Generic")?;
if let Some(generic_context) = generic_context {
generic_context.display(self.db).fmt(f)?;
}
Ok(())
}
}
}
}
Display { base: self, db }
}
pub(crate) fn name(self, db: &'db dyn Db) -> &'db str {
match self {
ClassBase::Class(class) => class.name(db),
ClassBase::Dynamic(DynamicType::Any) => "Any",
ClassBase::Dynamic(DynamicType::Unknown) => "Unknown",
ClassBase::Dynamic(DynamicType::Todo(_) | DynamicType::TodoPEP695ParamSpec) => "@Todo",
ClassBase::Protocol(_) => "Protocol",
ClassBase::Generic(_) => "Generic",
ClassBase::Protocol => "Protocol",
ClassBase::Generic => "Generic",
}
}
@ -255,12 +213,8 @@ impl<'db> ClassBase<'db> {
KnownInstanceType::Callable => {
Self::try_from_type(db, todo_type!("Support for Callable as a base class"))
}
KnownInstanceType::Protocol(generic_context) => {
Some(ClassBase::Protocol(generic_context))
}
KnownInstanceType::Generic(generic_context) => {
Some(ClassBase::Generic(generic_context))
}
KnownInstanceType::Protocol(_) => Some(ClassBase::Protocol),
KnownInstanceType::Generic(_) => Some(ClassBase::Generic),
},
}
}
@ -268,14 +222,14 @@ impl<'db> ClassBase<'db> {
pub(super) fn into_class(self) -> Option<ClassType<'db>> {
match self {
Self::Class(class) => Some(class),
Self::Dynamic(_) | Self::Generic(_) | Self::Protocol(_) => None,
Self::Dynamic(_) | Self::Generic | Self::Protocol => None,
}
}
fn apply_type_mapping<'a>(self, db: &'db dyn Db, type_mapping: &TypeMapping<'a, 'db>) -> Self {
match self {
Self::Class(class) => Self::Class(class.apply_type_mapping(db, type_mapping)),
Self::Dynamic(_) | Self::Generic(_) | Self::Protocol(_) => self,
Self::Dynamic(_) | Self::Generic | Self::Protocol => self,
}
}
@ -299,7 +253,7 @@ impl<'db> ClassBase<'db> {
.try_mro(db, specialization)
.is_err_and(MroError::is_cycle)
}
ClassBase::Dynamic(_) | ClassBase::Generic(_) | ClassBase::Protocol(_) => false,
ClassBase::Dynamic(_) | ClassBase::Generic | ClassBase::Protocol => false,
}
}
@ -310,12 +264,8 @@ impl<'db> ClassBase<'db> {
additional_specialization: Option<Specialization<'db>>,
) -> impl Iterator<Item = ClassBase<'db>> {
match self {
ClassBase::Protocol(context) => {
ClassBaseMroIterator::length_3(db, self, ClassBase::Generic(context))
}
ClassBase::Dynamic(_) | ClassBase::Generic(_) => {
ClassBaseMroIterator::length_2(db, self)
}
ClassBase::Protocol => ClassBaseMroIterator::length_3(db, self, ClassBase::Generic),
ClassBase::Dynamic(_) | ClassBase::Generic => ClassBaseMroIterator::length_2(db, self),
ClassBase::Class(class) => {
ClassBaseMroIterator::from_class(db, class, additional_specialization)
}
@ -338,12 +288,8 @@ impl<'db> From<ClassBase<'db>> for Type<'db> {
match value {
ClassBase::Dynamic(dynamic) => Type::Dynamic(dynamic),
ClassBase::Class(class) => class.into(),
ClassBase::Protocol(generic_context) => {
Type::KnownInstance(KnownInstanceType::Protocol(generic_context))
}
ClassBase::Generic(generic_context) => {
Type::KnownInstance(KnownInstanceType::Generic(generic_context))
}
ClassBase::Protocol => Type::KnownInstance(KnownInstanceType::Protocol(None)),
ClassBase::Generic => Type::KnownInstance(KnownInstanceType::Generic(None)),
}
}
}

6
crates/ty_python_semantic/src/types/diagnostic.rs
View file

@ -13,6 +13,7 @@ use crate::types::string_annotation::{
};
use crate::types::{KnownFunction, KnownInstanceType, Type, protocol_class::ProtocolClassLiteral};
use crate::{Program, PythonVersionWithSource, declare_lint};
use itertools::Itertools;
use ruff_db::diagnostic::{Annotation, Diagnostic, Severity, Span, SubDiagnostic};
use ruff_db::files::system_path_to_file;
use ruff_python_ast::{self as ast, AnyNodeRef};
@ -1698,10 +1699,7 @@ pub(super) fn report_implicit_return_type(
let Some(class) = enclosing_class_of_method else {
return;
};
if class
.iter_mro(db, None)
.any(|base| matches!(base, ClassBase::Protocol(_)))
{
if class.iter_mro(db, None).contains(&ClassBase::Protocol) {
diagnostic.info(
"Only functions in stub files, methods on protocol classes, \
or methods with `@abstractmethod` are permitted to have empty bodies",

2
crates/ty_python_semantic/src/types/infer.rs
View file

@ -7529,7 +7529,7 @@ impl<'db> TypeInferenceBuilder<'db> {
if !value_ty.into_class_literal().is_some_and(|class| {
class
.iter_mro(self.db(), None)
.any(|base| matches!(base, ClassBase::Generic(_)))
.contains(&ClassBase::Generic)
}) {
report_non_subscriptable(
&self.context,

140
crates/ty_python_semantic/src/types/mro.rs
View file

@ -7,7 +7,7 @@ use rustc_hash::FxBuildHasher;
use crate::Db;
use crate::types::class_base::ClassBase;
use crate::types::generics::Specialization;
use crate::types::{ClassLiteral, ClassType, Type};
use crate::types::{ClassLiteral, ClassType, KnownInstanceType, Type};
/// The inferred method resolution order of a given class.
///
@ -48,12 +48,12 @@ impl<'db> Mro<'db> {
/// [`super::infer::TypeInferenceBuilder::infer_region_scope`].)
pub(super) fn of_class(
db: &'db dyn Db,
class: ClassLiteral<'db>,
class_literal: ClassLiteral<'db>,
specialization: Option<Specialization<'db>>,
) -> Result<Self, MroError<'db>> {
Self::of_class_impl(db, class, specialization).map_err(|err| {
err.into_mro_error(db, class.apply_optional_specialization(db, specialization))
})
let class = class_literal.apply_optional_specialization(db, specialization);
Self::of_class_impl(db, class, class_literal.explicit_bases(db), specialization)
.map_err(|err| err.into_mro_error(db, class))
}
pub(super) fn from_error(db: &'db dyn Db, class: ClassType<'db>) -> Self {
@ -66,17 +66,16 @@ impl<'db> Mro<'db> {
fn of_class_impl(
db: &'db dyn Db,
class: ClassLiteral<'db>,
class: ClassType<'db>,
bases: &[Type<'db>],
specialization: Option<Specialization<'db>>,
) -> Result<Self, MroErrorKind<'db>> {
let class_type = class.apply_optional_specialization(db, specialization);
match class.explicit_bases(db) {
match bases {
// `builtins.object` is the special case:
// the only class in Python that has an MRO with length <2
[] if class.is_object(db) => Ok(Self::from([
// object is not generic, so the default specialization should be a no-op
ClassBase::Class(class_type),
ClassBase::Class(class),
])),
// All other classes in Python have an MRO with length >=2.
@ -92,44 +91,82 @@ impl<'db> Mro<'db> {
// >>> Foo.__mro__
// (<class '__main__.Foo'>, <class 'object'>)
// ```
[] => Ok(Self::from([
ClassBase::Class(class_type),
ClassBase::object(db),
])),
[] => {
// e.g. `class Foo[T]: ...` implicitly has `Generic` inserted into its bases
if class.is_generic() {
Ok(Self::from([
ClassBase::Class(class),
ClassBase::Generic,
ClassBase::object(db),
]))
} else {
Ok(Self::from([ClassBase::Class(class), ClassBase::object(db)]))
}
}
// Fast path for a class that has only a single explicit base.
//
// This *could* theoretically be handled by the final branch below,
// but it's a common case (i.e., worth optimizing for),
// and the `c3_merge` function requires lots of allocations.
[single_base] => ClassBase::try_from_type(db, *single_base).map_or_else(
|| Err(MroErrorKind::InvalidBases(Box::from([(0, *single_base)]))),
|single_base| {
if single_base.has_cyclic_mro(db) {
Err(MroErrorKind::InheritanceCycle)
} else {
Ok(std::iter::once(ClassBase::Class(
class.apply_optional_specialization(db, specialization),
))
.chain(single_base.mro(db, specialization))
.collect())
}
},
),
[single_base]
if !matches!(
single_base,
Type::GenericAlias(_)
| Type::KnownInstance(
KnownInstanceType::Generic(_) | KnownInstanceType::Protocol(_)
)
) =>
{
ClassBase::try_from_type(db, *single_base).map_or_else(
|| Err(MroErrorKind::InvalidBases(Box::from([(0, *single_base)]))),
|single_base| {
if single_base.has_cyclic_mro(db) {
Err(MroErrorKind::InheritanceCycle)
} else {
Ok(std::iter::once(ClassBase::Class(class))
.chain(single_base.mro(db, specialization))
.collect())
}
},
)
}
// The class has multiple explicit bases.
//
// We'll fallback to a full implementation of the C3-merge algorithm to determine
// what MRO Python will give this class at runtime
// (if an MRO is indeed resolvable at all!)
multiple_bases => {
let mut valid_bases = vec![];
original_bases => {
let mut resolved_bases = vec![];
let mut invalid_bases = vec![];
for (i, base) in multiple_bases.iter().enumerate() {
match ClassBase::try_from_type(db, *base) {
Some(valid_base) => valid_bases.push(valid_base),
None => invalid_bases.push((i, *base)),
for (i, base) in original_bases.iter().enumerate() {
// This emulates the behavior of `typing._GenericAlias.__mro_entries__` at
// <https://github.com/python/cpython/blob/ad42dc1909bdf8ec775b63fb22ed48ff42797a17/Lib/typing.py#L1487-L1500>.
//
// Note that 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)`.
if let Type::KnownInstance(KnownInstanceType::Generic(Some(_))) = base {
if original_bases
.contains(&Type::KnownInstance(KnownInstanceType::Protocol(None)))
{
continue;
}
if original_bases[i + 1..]
.iter()
.any(|b| b.is_generic_alias() && b != base)
{
continue;
}
resolved_bases.push(ClassBase::Generic);
} else {
match ClassBase::try_from_type(db, *base) {
Some(valid_base) => resolved_bases.push(valid_base),
None => invalid_bases.push((i, *base)),
}
}
}
@ -137,15 +174,15 @@ impl<'db> Mro<'db> {
return Err(MroErrorKind::InvalidBases(invalid_bases.into_boxed_slice()));
}
let mut seqs = vec![VecDeque::from([ClassBase::Class(class_type)])];
for base in &valid_bases {
let mut seqs = vec![VecDeque::from([ClassBase::Class(class)])];
for base in &resolved_bases {
if base.has_cyclic_mro(db) {
return Err(MroErrorKind::InheritanceCycle);
}
seqs.push(base.mro(db, specialization).collect());
}
seqs.push(
valid_bases
resolved_bases
.iter()
.map(|base| base.apply_optional_specialization(db, specialization))
.collect(),
@ -161,8 +198,20 @@ impl<'db> Mro<'db> {
let mut base_to_indices: IndexMap<ClassBase<'db>, Vec<usize>, FxBuildHasher> =
IndexMap::default();
for (index, base) in valid_bases.iter().enumerate() {
base_to_indices.entry(*base).or_default().push(index);
// We need to iterate over `original_bases` here rather than `resolved_bases`
// so that we get the correct index of the duplicate bases if there were any
// (`resolved_bases` may be a longer list than `original_bases`!). However, we
// need to use a `ClassBase` rather than a `Type` as the key type for the
// `base_to_indices` map so that a class such as
// `class Foo(Protocol[T], Protocol): ...` correctly causes us to emit a
// `duplicate-base` diagnostic (matching the runtime behaviour) rather than an
// `inconsistent-mro` diagnostic (which would be accurate -- but not nearly as
// precise!).
for (index, base) in original_bases.iter().enumerate() {
let Some(base) = ClassBase::try_from_type(db, *base) else {
continue;
};
base_to_indices.entry(base).or_default().push(index);
}
let mut errors = vec![];
@ -175,9 +224,7 @@ impl<'db> Mro<'db> {
continue;
}
match base {
ClassBase::Class(_)
| ClassBase::Generic(_)
| ClassBase::Protocol(_) => {
ClassBase::Class(_) | ClassBase::Generic | ClassBase::Protocol => {
errors.push(DuplicateBaseError {
duplicate_base: base,
first_index: *first_index,
@ -193,13 +240,10 @@ impl<'db> Mro<'db> {
if duplicate_bases.is_empty() {
if duplicate_dynamic_bases {
Ok(Mro::from_error(
db,
class.apply_optional_specialization(db, specialization),
))
Ok(Mro::from_error(db, class))
} else {
Err(MroErrorKind::UnresolvableMro {
bases_list: valid_bases.into_boxed_slice(),
bases_list: original_bases.iter().copied().collect(),
})
}
} else {
@ -378,7 +422,7 @@ pub(super) enum MroErrorKind<'db> {
/// The MRO is otherwise unresolvable through the C3-merge algorithm.
///
/// See [`c3_merge`] for more details.
UnresolvableMro { bases_list: Box<[ClassBase<'db>]> },
UnresolvableMro { bases_list: Box<[Type<'db>]> },
}
impl<'db> MroErrorKind<'db> {

10
crates/ty_python_semantic/src/types/type_ordering.rs
View file

@ -152,13 +152,11 @@ pub(super) fn union_or_intersection_elements_ordering<'db>(
(ClassBase::Class(_), _) => Ordering::Less,
(_, ClassBase::Class(_)) => Ordering::Greater,
(ClassBase::Protocol(left), ClassBase::Protocol(right)) => left.cmp(&right),
(ClassBase::Protocol(_), _) => Ordering::Less,
(_, ClassBase::Protocol(_)) => Ordering::Greater,
(ClassBase::Protocol, _) => Ordering::Less,
(_, ClassBase::Protocol) => Ordering::Greater,
(ClassBase::Generic(left), ClassBase::Generic(right)) => left.cmp(&right),
(ClassBase::Generic(_), _) => Ordering::Less,
(_, ClassBase::Generic(_)) => Ordering::Greater,
(ClassBase::Generic, _) => Ordering::Less,
(_, ClassBase::Generic) => Ordering::Greater,
(ClassBase::Dynamic(left), ClassBase::Dynamic(right)) => {
dynamic_elements_ordering(left, right)