[red-knot] use fixpoint iteration for all cycles

Cargo.toml

@@ -123,7 +123,7 @@ rayon = { version = "1.10.0" }
 regex = { version = "1.10.2" }
 rustc-hash = { version = "2.0.0" }
 # When updating salsa, make sure to also update the revision in `fuzz/Cargo.toml`
-salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "99be5d9917c3dd88e19735a82ef6bf39ba84bd7e" }
+salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9" }
 schemars = { version = "0.8.16" }
 seahash = { version = "4.1.0" }
 serde = { version = "1.0.197", features = ["derive"] }

crates/red_knot_project/tests/check.rs

@@ -279,23 +279,4 @@ impl SourceOrderVisitor<'_> for PullTypesVisitor<'_> {
 
 /// Whether or not the .py/.pyi version of this file is expected to fail
 #[rustfmt::skip]
-const KNOWN_FAILURES: &[(&str, bool, bool)] = &[
-    // related to circular references in nested functions
-    ("crates/ruff_linter/resources/test/fixtures/flake8_return/RET503.py", false, true),
-    // related to circular references in class definitions
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_27.py", true, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F811_19.py", true, false),
-    ("crates/ruff_linter/resources/test/fixtures/pyupgrade/UP039.py", true, false),
-    // related to circular references in type aliases (salsa cycle panic):
-    ("crates/ruff_python_parser/resources/inline/err/type_alias_invalid_value_expr.py", true, true),
-    ("crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008.py", true, true),
-    // related to circular references in f-string annotations (invalid syntax)
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_15.py", true, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_14.py", false, true),
-    // related to circular references in stub type annotations (salsa cycle panic):
-    ("crates/ruff_linter/resources/test/fixtures/pycodestyle/E501_4.py", false, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F401_0.py", false, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F401_12.py", false, true),
-    ("crates/ruff_linter/resources/test/fixtures/pyflakes/F401_14.py", false, true),
-];
+const KNOWN_FAILURES: &[(&str, bool, bool)] = &[];

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

@@ -180,6 +180,7 @@ reveal_type(Sub)  # revealed: Literal[Sub]
 class Base[T]: ...
 
 # TODO: error: [unresolved-reference]
+# error: [non-subscriptable]
 class Sub(Base[Sub]): ...
 ```
 

crates/red_knot_python_semantic/src/symbol.rs

@@ -488,7 +488,27 @@ impl<'db> From<Symbol<'db>> for SymbolAndQualifiers<'db> {
     }
 }
 
-#[salsa::tracked]
+fn symbol_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &SymbolAndQualifiers<'db>,
+    _count: u32,
+    _scope: ScopeId<'db>,
+    _symbol_id: ScopedSymbolId,
+    _requires_explicit_reexport: RequiresExplicitReExport,
+) -> salsa::CycleRecoveryAction<SymbolAndQualifiers<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
+
+fn symbol_cycle_initial<'db>(
+    _db: &'db dyn Db,
+    _scope: ScopeId<'db>,
+    _symbol_id: ScopedSymbolId,
+    _requires_explicit_reexport: RequiresExplicitReExport,
+) -> SymbolAndQualifiers<'db> {
+    Symbol::bound(Type::Never).into()
+}
+
+#[salsa::tracked(cycle_fn=symbol_cycle_recover, cycle_initial=symbol_cycle_initial)]
 fn symbol_by_id<'db>(
     db: &'db dyn Db,
     scope: ScopeId<'db>,

crates/red_knot_python_semantic/src/types/infer.rs

@@ -26,6 +26,13 @@
 //! stringified annotations. We have a fourth Salsa query for inferring the deferred types
 //! associated with a particular definition. Scope-level inference infers deferred types for all
 //! definitions once the rest of the types in the scope have been inferred.
+//!
+//! Many of our type inference Salsa queries implement cycle recovery via fixed-point iteration. In
+//! general, they initiate fixed-point iteration by returning a `TypeInference` that returns
+//! `Type::Never` for all expressions, bindings, and declarations, and then they continue iterating
+//! the query cycle until a fixed-point is reached. Salsa has a built-in fixed limit on the number
+//! of iterations, so if we fail to converge, Salsa will eventually panic. (This should of course
+//! be considered a bug.)
 use std::num::NonZeroU32;
 
 use itertools::{Either, Itertools};
@@ -100,7 +107,7 @@ use super::{CallDunderError, ParameterExpectation, ParameterExpectations};
 /// Infer all types for a [`ScopeId`], including all definitions and expressions in that scope.
 /// Use when checking a scope, or needing to provide a type for an arbitrary expression in the
 /// scope.
-#[salsa::tracked(return_ref)]
+#[salsa::tracked(return_ref, cycle_fn=scope_cycle_recover, cycle_initial=scope_cycle_initial)]
 pub(crate) fn infer_scope_types<'db>(db: &'db dyn Db, scope: ScopeId<'db>) -> TypeInference<'db> {
     let file = scope.file(db);
     let _span =
@@ -114,26 +121,22 @@ pub(crate) fn infer_scope_types<'db>(db: &'db dyn Db, scope: ScopeId<'db>) -> Ty
     TypeInferenceBuilder::new(db, InferenceRegion::Scope(scope), index).finish()
 }
 
-/// TODO temporary cycle recovery for [`infer_definition_types()`], pending fixpoint iteration
-fn infer_definition_types_cycle_recovery<'db>(
-    db: &'db dyn Db,
-    _cycle: &salsa::Cycle,
-    input: Definition<'db>,
-) -> TypeInference<'db> {
-    let file = input.file(db);
-    let _span = tracing::trace_span!(
-        "infer_definition_types_cycle_recovery",
-        range = ?input.kind(db).target_range(),
-        file = %file.path(db)
-    )
-    .entered();
+fn scope_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &TypeInference<'db>,
+    _count: u32,
+    _scope: ScopeId<'db>,
+) -> salsa::CycleRecoveryAction<TypeInference<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
 
-    TypeInference::cycle_fallback(input.scope(db), todo_type!("cycle recovery"))
+fn scope_cycle_initial<'db>(_db: &'db dyn Db, scope: ScopeId<'db>) -> TypeInference<'db> {
+    TypeInference::cycle_fallback(scope, Type::Never)
 }
 
 /// Infer all types for a [`Definition`] (including sub-expressions).
 /// Use when resolving a symbol name use or public type of a symbol.
-#[salsa::tracked(return_ref, recovery_fn=infer_definition_types_cycle_recovery)]
+#[salsa::tracked(return_ref, cycle_fn=definition_cycle_recover, cycle_initial=definition_cycle_initial)]
 pub(crate) fn infer_definition_types<'db>(
     db: &'db dyn Db,
     definition: Definition<'db>,
@@ -151,11 +154,27 @@ pub(crate) fn infer_definition_types<'db>(
     TypeInferenceBuilder::new(db, InferenceRegion::Definition(definition), index).finish()
 }
 
+fn definition_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &TypeInference<'db>,
+    _count: u32,
+    _definition: Definition<'db>,
+) -> salsa::CycleRecoveryAction<TypeInference<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
+
+fn definition_cycle_initial<'db>(
+    db: &'db dyn Db,
+    definition: Definition<'db>,
+) -> TypeInference<'db> {
+    TypeInference::cycle_fallback(definition.scope(db), Type::Never)
+}
+
 /// Infer types for all deferred type expressions in a [`Definition`].
 ///
 /// Deferred expressions are type expressions (annotations, base classes, aliases...) in a stub
 /// file, or in a file with `from __future__ import annotations`, or stringified annotations.
-#[salsa::tracked(return_ref)]
+#[salsa::tracked(return_ref, cycle_fn=deferred_cycle_recover, cycle_initial=deferred_cycle_initial)]
 pub(crate) fn infer_deferred_types<'db>(
     db: &'db dyn Db,
     definition: Definition<'db>,
@@ -174,11 +193,24 @@ pub(crate) fn infer_deferred_types<'db>(
     TypeInferenceBuilder::new(db, InferenceRegion::Deferred(definition), index).finish()
 }
 
+fn deferred_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &TypeInference<'db>,
+    _count: u32,
+    _definition: Definition<'db>,
+) -> salsa::CycleRecoveryAction<TypeInference<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
+
+fn deferred_cycle_initial<'db>(db: &'db dyn Db, definition: Definition<'db>) -> TypeInference<'db> {
+    TypeInference::cycle_fallback(definition.scope(db), Type::Never)
+}
+
 /// Infer all types for an [`Expression`] (including sub-expressions).
 /// Use rarely; only for cases where we'd otherwise risk double-inferring an expression: RHS of an
 /// assignment, which might be unpacking/multi-target and thus part of multiple definitions, or a
 /// type narrowing guard expression (e.g. if statement test node).
-#[salsa::tracked(return_ref)]
+#[salsa::tracked(return_ref, cycle_fn=expression_cycle_recover, cycle_initial=expression_cycle_initial)]
 pub(crate) fn infer_expression_types<'db>(
     db: &'db dyn Db,
     expression: Expression<'db>,
@@ -197,6 +229,22 @@ pub(crate) fn infer_expression_types<'db>(
     TypeInferenceBuilder::new(db, InferenceRegion::Expression(expression), index).finish()
 }
 
+fn expression_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &TypeInference<'db>,
+    _count: u32,
+    _expression: Expression<'db>,
+) -> salsa::CycleRecoveryAction<TypeInference<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
+
+fn expression_cycle_initial<'db>(
+    db: &'db dyn Db,
+    expression: Expression<'db>,
+) -> TypeInference<'db> {
+    TypeInference::cycle_fallback(expression.scope(db), Type::Never)
+}
+
 /// Infers the type of an `expression` that is guaranteed to be in the same file as the calling query.
 ///
 /// This is a small helper around [`infer_expression_types()`] to reduce the boilerplate.
@@ -218,7 +266,7 @@ pub(super) fn infer_same_file_expression_type<'db>(
 ///
 /// Use [`infer_same_file_expression_type`] if it is guaranteed that  `expression` is in the same
 /// to avoid unnecessary salsa ingredients. This is normally the case inside the `TypeInferenceBuilder`.
-#[salsa::tracked]
+#[salsa::tracked(cycle_fn=single_expression_cycle_recover, cycle_initial=single_expression_cycle_initial)]
 pub(crate) fn infer_expression_type<'db>(
     db: &'db dyn Db,
     expression: Expression<'db>,
@@ -227,6 +275,22 @@ pub(crate) fn infer_expression_type<'db>(
     infer_same_file_expression_type(db, expression)
 }
 
+fn single_expression_cycle_recover<'db>(
+    _db: &'db dyn Db,
+    _value: &Type<'db>,
+    _count: u32,
+    _expression: Expression<'db>,
+) -> salsa::CycleRecoveryAction<Type<'db>> {
+    salsa::CycleRecoveryAction::Iterate
+}
+
+fn single_expression_cycle_initial<'db>(
+    _db: &'db dyn Db,
+    _expression: Expression<'db>,
+) -> Type<'db> {
+    Type::Never
+}
+
 /// Infer the types for an [`Unpack`] operation.
 ///
 /// This infers the expression type and performs structural match against the target expression

fuzz/Cargo.toml

@@ -29,7 +29,7 @@ ruff_python_formatter = { path = "../crates/ruff_python_formatter" }
 ruff_text_size = { path = "../crates/ruff_text_size" }
 
 libfuzzer-sys = { git = "https://github.com/rust-fuzz/libfuzzer", default-features = false }
-salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "99be5d9917c3dd88e19735a82ef6bf39ba84bd7e" }
+salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9" }
 similar = { version = "2.5.0" }
 tracing = { version = "0.1.40" }