From abaa18993b285a183ba326cf92ab4e5ba0199cea Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 13 Mar 2025 08:16:03 -0700 Subject: [PATCH] [red-knot] handle cycles in MRO/bases resolution (#16693) There can be semi-cyclic inheritance patterns (e.g. recursive generics) that are not technically inheritance cycles, but that can cause us to hit Salsa query cycles in evaluating a type's MRO. Add fixed-point handling to these MRO-related queries so we don't panic on these cycles. The details of what queries we hit in what order in this case will change as we implement support for generics, but ultimately we will probably need cycle handling for all queries that can re-enter type inference, otherwise we are susceptible to small changes in query execution order causing panics. Fixes #14333 Further reduces the panicking set of seeds in #14737 --- .../resources/mdtest/generics/classes.md | 12 ++++ crates/red_knot_python_semantic/src/types.rs | 8 +++ .../src/types/class.rs | 57 ++++++++++++++++++- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md b/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md index da9626323b..d7944b1369 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md +++ b/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md @@ -162,6 +162,8 @@ class Sub(Base[Sub]): ... reveal_type(Sub) # revealed: Literal[Sub] ``` +A similar case can work in a non-stub file, if forward references are stringified: + `string_annotation.py`: ```py @@ -174,6 +176,8 @@ class Sub(Base["Sub"]): ... reveal_type(Sub) # revealed: Literal[Sub] ``` +In a non-stub file, without stringified forward references, this raises a `NameError`: + `bare_annotation.py`: ```py @@ -184,5 +188,13 @@ class Base[T]: ... class Sub(Base[Sub]): ... ``` +## Another cyclic case + +```pyi +# TODO no error (generics) +# error: [invalid-base] +class Derived[T](list[Derived[T]]): ... +``` + [crtp]: https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern [f-bound]: https://en.wikipedia.org/wiki/Bounded_quantification#F-bounded_quantification diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 2d5f05740a..673eb000de 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1557,6 +1557,7 @@ impl<'db> Type<'db> { /// of union and intersection types. #[salsa::tracked] fn class_member(self, db: &'db dyn Db, name: Name) -> SymbolAndQualifiers<'db> { + tracing::trace!("class_member: {}.{}", self.display(db), name); match self { Type::Union(union) => union .map_with_boundness_and_qualifiers(db, |elem| elem.class_member(db, name.clone())), @@ -1678,6 +1679,12 @@ impl<'db> Type<'db> { instance: Type<'db>, owner: Type<'db>, ) -> Option<(Type<'db>, AttributeKind)> { + tracing::trace!( + "try_call_dunder_get: {}, {}, {}", + self.display(db), + instance.display(db), + owner.display(db) + ); let descr_get = self.class_member(db, "__get__".into()).symbol; if let Symbol::Type(descr_get, descr_get_boundness) = descr_get { @@ -1910,6 +1917,7 @@ impl<'db> Type<'db> { name: Name, policy: MemberLookupPolicy, ) -> SymbolAndQualifiers<'db> { + tracing::trace!("member_lookup_with_policy: {}.{}", self.display(db), name); if name == "__class__" { return Symbol::bound(self.to_meta_type(db)).into(); } diff --git a/crates/red_knot_python_semantic/src/types/class.rs b/crates/red_knot_python_semantic/src/types/class.rs index cd2f76f922..02e56a3b35 100644 --- a/crates/red_knot_python_semantic/src/types/class.rs +++ b/crates/red_knot_python_semantic/src/types/class.rs @@ -43,6 +43,50 @@ pub struct Class<'db> { pub(crate) known: Option, } +fn explicit_bases_cycle_recover<'db>( + _db: &'db dyn Db, + _value: &[Type<'db>], + _count: u32, + _self: Class<'db>, +) -> salsa::CycleRecoveryAction]>> { + salsa::CycleRecoveryAction::Iterate +} + +fn explicit_bases_cycle_initial<'db>(_db: &'db dyn Db, _self: Class<'db>) -> Box<[Type<'db>]> { + Box::default() +} + +fn try_mro_cycle_recover<'db>( + _db: &'db dyn Db, + _value: &Result, MroError<'db>>, + _count: u32, + _self: Class<'db>, +) -> salsa::CycleRecoveryAction, MroError<'db>>> { + salsa::CycleRecoveryAction::Iterate +} + +#[allow(clippy::unnecessary_wraps)] +fn try_mro_cycle_initial<'db>( + db: &'db dyn Db, + self_: Class<'db>, +) -> Result, MroError<'db>> { + Ok(Mro::from_error(db, self_)) +} + +#[allow(clippy::ref_option, clippy::trivially_copy_pass_by_ref)] +fn inheritance_cycle_recover<'db>( + _db: &'db dyn Db, + _value: &Option, + _count: u32, + _self: Class<'db>, +) -> salsa::CycleRecoveryAction> { + salsa::CycleRecoveryAction::Iterate +} + +fn inheritance_cycle_initial<'db>(_db: &'db dyn Db, _self: Class<'db>) -> Option { + None +} + #[salsa::tracked] impl<'db> Class<'db> { /// Return `true` if this class represents `known_class` @@ -81,8 +125,9 @@ impl<'db> Class<'db> { .map(|ClassLiteralType { class }| class) } - #[salsa::tracked(return_ref)] + #[salsa::tracked(return_ref, cycle_fn=explicit_bases_cycle_recover, cycle_initial=explicit_bases_cycle_initial)] fn explicit_bases_query(self, db: &'db dyn Db) -> Box<[Type<'db>]> { + tracing::trace!("Class::explicit_bases_query: {}", self.name(db)); let class_stmt = self.node(db); let class_definition = semantic_index(db, self.file(db)).definition(class_stmt); @@ -110,6 +155,7 @@ impl<'db> Class<'db> { /// Return the types of the decorators on this class #[salsa::tracked(return_ref)] fn decorators(self, db: &'db dyn Db) -> Box<[Type<'db>]> { + tracing::trace!("Class::decorators: {}", self.name(db)); let class_stmt = self.node(db); if class_stmt.decorator_list.is_empty() { return Box::new([]); @@ -141,8 +187,9 @@ impl<'db> Class<'db> { /// attribute on a class at runtime. /// /// [method resolution order]: https://docs.python.org/3/glossary.html#term-method-resolution-order - #[salsa::tracked(return_ref)] + #[salsa::tracked(return_ref, cycle_fn=try_mro_cycle_recover, cycle_initial=try_mro_cycle_initial)] pub(super) fn try_mro(self, db: &'db dyn Db) -> Result, MroError<'db>> { + tracing::trace!("Class::try_mro: {}", self.name(db)); Mro::of_class(db, self) } @@ -199,6 +246,8 @@ impl<'db> Class<'db> { /// Return the metaclass of this class, or an error if the metaclass cannot be inferred. #[salsa::tracked] pub(super) fn try_metaclass(self, db: &'db dyn Db) -> Result, MetaclassError<'db>> { + tracing::trace!("Class::try_metaclass: {}", self.name(db)); + // Identify the class's own metaclass (or take the first base class's metaclass). let mut base_classes = self.fully_static_explicit_bases(db).peekable(); @@ -662,7 +711,7 @@ impl<'db> Class<'db> { /// /// A class definition like this will fail at runtime, /// but we must be resilient to it or we could panic. - #[salsa::tracked] + #[salsa::tracked(cycle_fn=inheritance_cycle_recover, cycle_initial=inheritance_cycle_initial)] pub(super) fn inheritance_cycle(self, db: &'db dyn Db) -> Option { /// Return `true` if the class is cyclically defined. /// @@ -694,6 +743,8 @@ impl<'db> Class<'db> { result } + tracing::trace!("Class::inheritance_cycle: {}", self.name(db)); + let visited_classes = &mut IndexSet::new(); if !is_cyclically_defined_recursive(db, self, &mut IndexSet::new(), visited_classes) { None