Skip to content

Commit abaa189

Browse files
authored
[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
1 parent 360ba09 commit abaa189

File tree

3 files changed

+74
-3
lines changed

3 files changed

+74
-3
lines changed

crates/red_knot_python_semantic/resources/mdtest/generics/classes.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,8 @@ class Sub(Base[Sub]): ...
162162
reveal_type(Sub) # revealed: Literal[Sub]
163163
```
164164

165+
A similar case can work in a non-stub file, if forward references are stringified:
166+
165167
`string_annotation.py`:
166168

167169
```py
@@ -174,6 +176,8 @@ class Sub(Base["Sub"]): ...
174176
reveal_type(Sub) # revealed: Literal[Sub]
175177
```
176178

179+
In a non-stub file, without stringified forward references, this raises a `NameError`:
180+
177181
`bare_annotation.py`:
178182

179183
```py
@@ -184,5 +188,13 @@ class Base[T]: ...
184188
class Sub(Base[Sub]): ...
185189
```
186190

191+
## Another cyclic case
192+
193+
```pyi
194+
# TODO no error (generics)
195+
# error: [invalid-base]
196+
class Derived[T](list[Derived[T]]): ...
197+
```
198+
187199
[crtp]: https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
188200
[f-bound]: https://en.wikipedia.org/wiki/Bounded_quantification#F-bounded_quantification

crates/red_knot_python_semantic/src/types.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,6 +1557,7 @@ impl<'db> Type<'db> {
15571557
/// of union and intersection types.
15581558
#[salsa::tracked]
15591559
fn class_member(self, db: &'db dyn Db, name: Name) -> SymbolAndQualifiers<'db> {
1560+
tracing::trace!("class_member: {}.{}", self.display(db), name);
15601561
match self {
15611562
Type::Union(union) => union
15621563
.map_with_boundness_and_qualifiers(db, |elem| elem.class_member(db, name.clone())),
@@ -1678,6 +1679,12 @@ impl<'db> Type<'db> {
16781679
instance: Type<'db>,
16791680
owner: Type<'db>,
16801681
) -> Option<(Type<'db>, AttributeKind)> {
1682+
tracing::trace!(
1683+
"try_call_dunder_get: {}, {}, {}",
1684+
self.display(db),
1685+
instance.display(db),
1686+
owner.display(db)
1687+
);
16811688
let descr_get = self.class_member(db, "__get__".into()).symbol;
16821689

16831690
if let Symbol::Type(descr_get, descr_get_boundness) = descr_get {
@@ -1910,6 +1917,7 @@ impl<'db> Type<'db> {
19101917
name: Name,
19111918
policy: MemberLookupPolicy,
19121919
) -> SymbolAndQualifiers<'db> {
1920+
tracing::trace!("member_lookup_with_policy: {}.{}", self.display(db), name);
19131921
if name == "__class__" {
19141922
return Symbol::bound(self.to_meta_type(db)).into();
19151923
}

crates/red_knot_python_semantic/src/types/class.rs

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,50 @@ pub struct Class<'db> {
4343
pub(crate) known: Option<KnownClass>,
4444
}
4545

46+
fn explicit_bases_cycle_recover<'db>(
47+
_db: &'db dyn Db,
48+
_value: &[Type<'db>],
49+
_count: u32,
50+
_self: Class<'db>,
51+
) -> salsa::CycleRecoveryAction<Box<[Type<'db>]>> {
52+
salsa::CycleRecoveryAction::Iterate
53+
}
54+
55+
fn explicit_bases_cycle_initial<'db>(_db: &'db dyn Db, _self: Class<'db>) -> Box<[Type<'db>]> {
56+
Box::default()
57+
}
58+
59+
fn try_mro_cycle_recover<'db>(
60+
_db: &'db dyn Db,
61+
_value: &Result<Mro<'db>, MroError<'db>>,
62+
_count: u32,
63+
_self: Class<'db>,
64+
) -> salsa::CycleRecoveryAction<Result<Mro<'db>, MroError<'db>>> {
65+
salsa::CycleRecoveryAction::Iterate
66+
}
67+
68+
#[allow(clippy::unnecessary_wraps)]
69+
fn try_mro_cycle_initial<'db>(
70+
db: &'db dyn Db,
71+
self_: Class<'db>,
72+
) -> Result<Mro<'db>, MroError<'db>> {
73+
Ok(Mro::from_error(db, self_))
74+
}
75+
76+
#[allow(clippy::ref_option, clippy::trivially_copy_pass_by_ref)]
77+
fn inheritance_cycle_recover<'db>(
78+
_db: &'db dyn Db,
79+
_value: &Option<InheritanceCycle>,
80+
_count: u32,
81+
_self: Class<'db>,
82+
) -> salsa::CycleRecoveryAction<Option<InheritanceCycle>> {
83+
salsa::CycleRecoveryAction::Iterate
84+
}
85+
86+
fn inheritance_cycle_initial<'db>(_db: &'db dyn Db, _self: Class<'db>) -> Option<InheritanceCycle> {
87+
None
88+
}
89+
4690
#[salsa::tracked]
4791
impl<'db> Class<'db> {
4892
/// Return `true` if this class represents `known_class`
@@ -81,8 +125,9 @@ impl<'db> Class<'db> {
81125
.map(|ClassLiteralType { class }| class)
82126
}
83127

84-
#[salsa::tracked(return_ref)]
128+
#[salsa::tracked(return_ref, cycle_fn=explicit_bases_cycle_recover, cycle_initial=explicit_bases_cycle_initial)]
85129
fn explicit_bases_query(self, db: &'db dyn Db) -> Box<[Type<'db>]> {
130+
tracing::trace!("Class::explicit_bases_query: {}", self.name(db));
86131
let class_stmt = self.node(db);
87132

88133
let class_definition = semantic_index(db, self.file(db)).definition(class_stmt);
@@ -110,6 +155,7 @@ impl<'db> Class<'db> {
110155
/// Return the types of the decorators on this class
111156
#[salsa::tracked(return_ref)]
112157
fn decorators(self, db: &'db dyn Db) -> Box<[Type<'db>]> {
158+
tracing::trace!("Class::decorators: {}", self.name(db));
113159
let class_stmt = self.node(db);
114160
if class_stmt.decorator_list.is_empty() {
115161
return Box::new([]);
@@ -141,8 +187,9 @@ impl<'db> Class<'db> {
141187
/// attribute on a class at runtime.
142188
///
143189
/// [method resolution order]: https://docs.python.org/3/glossary.html#term-method-resolution-order
144-
#[salsa::tracked(return_ref)]
190+
#[salsa::tracked(return_ref, cycle_fn=try_mro_cycle_recover, cycle_initial=try_mro_cycle_initial)]
145191
pub(super) fn try_mro(self, db: &'db dyn Db) -> Result<Mro<'db>, MroError<'db>> {
192+
tracing::trace!("Class::try_mro: {}", self.name(db));
146193
Mro::of_class(db, self)
147194
}
148195

@@ -199,6 +246,8 @@ impl<'db> Class<'db> {
199246
/// Return the metaclass of this class, or an error if the metaclass cannot be inferred.
200247
#[salsa::tracked]
201248
pub(super) fn try_metaclass(self, db: &'db dyn Db) -> Result<Type<'db>, MetaclassError<'db>> {
249+
tracing::trace!("Class::try_metaclass: {}", self.name(db));
250+
202251
// Identify the class's own metaclass (or take the first base class's metaclass).
203252
let mut base_classes = self.fully_static_explicit_bases(db).peekable();
204253

@@ -662,7 +711,7 @@ impl<'db> Class<'db> {
662711
///
663712
/// A class definition like this will fail at runtime,
664713
/// but we must be resilient to it or we could panic.
665-
#[salsa::tracked]
714+
#[salsa::tracked(cycle_fn=inheritance_cycle_recover, cycle_initial=inheritance_cycle_initial)]
666715
pub(super) fn inheritance_cycle(self, db: &'db dyn Db) -> Option<InheritanceCycle> {
667716
/// Return `true` if the class is cyclically defined.
668717
///
@@ -694,6 +743,8 @@ impl<'db> Class<'db> {
694743
result
695744
}
696745

746+
tracing::trace!("Class::inheritance_cycle: {}", self.name(db));
747+
697748
let visited_classes = &mut IndexSet::new();
698749
if !is_cyclically_defined_recursive(db, self, &mut IndexSet::new(), visited_classes) {
699750
None

0 commit comments

Comments
 (0)