From a176c1ac809843ca609bb5191002c9d45e84dfd1 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 12 Mar 2025 05:41:40 -0700 Subject: [PATCH] [red-knot] use fixpoint iteration for all cycles (#14029) Pulls in the latest Salsa main branch, which supports fixpoint iteration, and uses it to handle all query cycles. With this, we no longer need to skip any corpus files to avoid panics. Latest perf results show a 6% incremental and 1% cold-check regression. This is not a "no cycles" regression, as tomllib and typeshed do trigger some definition cycles (previously handled by our old `infer_definition_types` fallback to `Unknown`). We don't currently have a benchmark we can use to measure the pure no-cycles regression, though I expect there would still be some regression; the fixpoint iteration feature in Salsa does add some overhead even for non-cyclic queries. I think this regression is within the reasonable range for this feature. We can do further optimization work later, but I don't think it's the top priority right now. So going ahead and acknowledging the regression on CodSpeed. Mypy primer is happy, so this doesn't regress anything on our currently-checked projects. I expect it probably unlocks adding a number of new projects to our ecosystem check that previously would have panicked. Fixes #13792 Fixes #14672 --- Cargo.lock | 13 +-- Cargo.toml | 2 +- crates/red_knot_project/tests/check.rs | 21 +--- .../resources/mdtest/generics/classes.md | 1 + crates/red_knot_python_semantic/src/symbol.rs | 22 +++- .../src/types/infer.rs | 102 ++++++++++++++---- fuzz/Cargo.toml | 2 +- 7 files changed, 115 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cdc790fb13..ba9030af5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3327,8 +3327,8 @@ checksum = "6ea1a2d0a644769cc99faa24c3ad26b379b786fe7c36fd3c546254801650e6dd" [[package]] name = "salsa" -version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=99be5d9917c3dd88e19735a82ef6bf39ba84bd7e#99be5d9917c3dd88e19735a82ef6bf39ba84bd7e" +version = "0.19.0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9#095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9" dependencies = [ "boxcar", "compact_str", @@ -3338,6 +3338,7 @@ dependencies = [ "hashlink", "indexmap", "parking_lot", + "portable-atomic", "rayon", "rustc-hash 2.1.1", "salsa-macro-rules", @@ -3348,13 +3349,13 @@ dependencies = [ [[package]] name = "salsa-macro-rules" -version = "0.1.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=99be5d9917c3dd88e19735a82ef6bf39ba84bd7e#99be5d9917c3dd88e19735a82ef6bf39ba84bd7e" +version = "0.19.0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9#095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9" [[package]] name = "salsa-macros" -version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=99be5d9917c3dd88e19735a82ef6bf39ba84bd7e#99be5d9917c3dd88e19735a82ef6bf39ba84bd7e" +version = "0.19.0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9#095d8b2b8115c3cf8bf31914dd9ea74648bb7cf9" dependencies = [ "heck", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index af94be8bd3..f3782c5f14 100644 --- a/Cargo.toml +++ b/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"] } diff --git a/crates/red_knot_project/tests/check.rs b/crates/red_knot_project/tests/check.rs index 2f3d9b5172..091908cf0c 100644 --- a/crates/red_knot_project/tests/check.rs +++ b/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)] = &[]; 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 3ae0095c57..da9626323b 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/generics/classes.md +++ b/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]): ... ``` diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 7459e37f28..3ba9bc3692 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -488,7 +488,27 @@ impl<'db> From> 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> { + 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>, diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 8a64d0e053..5bcac004df 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/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> { + 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> { + 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> { + 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> { + 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> { + 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 diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 3e27db0c5f..64e8dc0f89 100644 --- a/fuzz/Cargo.toml +++ b/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" }