mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-07-07 21:25:08 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[red-knot] use fixpoint iteration for all cycles (#14029)

Browse source 
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
This commit is contained in:
Carl Meyer 2025-03-12 05:41:40 -07:00 • committed by GitHub
parent a6572a57c4
commit a176c1ac80
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 115 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

13
Cargo.lock generated
View file

@ -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",

2
Cargo.toml
View file

@ -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"] }

21
crates/red_knot_project/tests/check.rs
View file

@ -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)] = &[];

1
crates/red_knot_python_semantic/resources/mdtest/generics/classes.md
View file

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

22
crates/red_knot_python_semantic/src/symbol.rs
View file

@ -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>,

102
crates/red_knot_python_semantic/src/types/infer.rs
View file

@ -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

2
fuzz/Cargo.toml
View file

@ -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" }