From fd9882a1f4fbd66b87095b5fede5eaee56876ceb Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 10 Apr 2025 09:59:38 -0400 Subject: [PATCH] [red-knot] avoid unnecessary evaluation of visibility constraint on definitely-unbound symbol (#17326) This causes spurious query cycles. This PR also includes an update to Salsa, which gives us db events on cycle iteration, so we can write tests asserting the absence of a cycle. --- Cargo.lock | 6 +-- Cargo.toml | 2 +- crates/red_knot_python_semantic/src/symbol.rs | 23 +++++++--- .../src/types/infer.rs | 44 +++++++++++++++++++ fuzz/Cargo.toml | 2 +- 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 02761e11cc..e5c4999768 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3442,7 +3442,7 @@ checksum = "28d3b2b1366ec20994f1fd18c3c594f05c5dd4bc44d8bb0c1c632c8d6829481f" [[package]] name = "salsa" version = "0.19.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=b165ba7bd1b2a0112ce574a082ab8ea5102252ac#b165ba7bd1b2a0112ce574a082ab8ea5102252ac" +source = "git+https://github.com/salsa-rs/salsa.git?rev=87bf6b6c2d5f6479741271da73bd9d30c2580c26#87bf6b6c2d5f6479741271da73bd9d30c2580c26" dependencies = [ "boxcar", "compact_str 0.8.1", @@ -3465,12 +3465,12 @@ dependencies = [ [[package]] name = "salsa-macro-rules" version = "0.19.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=b165ba7bd1b2a0112ce574a082ab8ea5102252ac#b165ba7bd1b2a0112ce574a082ab8ea5102252ac" +source = "git+https://github.com/salsa-rs/salsa.git?rev=87bf6b6c2d5f6479741271da73bd9d30c2580c26#87bf6b6c2d5f6479741271da73bd9d30c2580c26" [[package]] name = "salsa-macros" version = "0.19.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=b165ba7bd1b2a0112ce574a082ab8ea5102252ac#b165ba7bd1b2a0112ce574a082ab8ea5102252ac" +source = "git+https://github.com/salsa-rs/salsa.git?rev=87bf6b6c2d5f6479741271da73bd9d30c2580c26#87bf6b6c2d5f6479741271da73bd9d30c2580c26" dependencies = [ "heck", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 0e4efb9004..9bdb2b3e22 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,7 +124,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 = "b165ba7bd1b2a0112ce574a082ab8ea5102252ac" } +salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "87bf6b6c2d5f6479741271da73bd9d30c2580c26" } schemars = { version = "0.8.16" } seahash = { version = "4.1.0" } serde = { version = "1.0.197", features = ["derive"] } diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 437f2dd3c2..4503264bf9 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -663,15 +663,24 @@ fn symbol_from_bindings_impl<'db>( requires_explicit_reexport.is_yes() && !binding.is_reexported(db) }; - let unbound_visibility = match bindings_with_constraints.peek() { + let unbound_visibility_constraint = match bindings_with_constraints.peek() { Some(BindingWithConstraints { binding, visibility_constraint, narrowing_constraint: _, - }) if binding.is_none_or(is_non_exported) => { - visibility_constraints.evaluate(db, predicates, *visibility_constraint) - } - _ => Truthiness::AlwaysFalse, + }) if binding.is_none_or(is_non_exported) => Some(*visibility_constraint), + _ => None, + }; + + // Evaluate this lazily because we don't always need it (for example, if there are no visible + // bindings at all, we don't need it), and it can cause us to evaluate visibility constraint + // expressions, which is extra work and can lead to cycles. + let unbound_visibility = || { + unbound_visibility_constraint + .map(|visibility_constraint| { + visibility_constraints.evaluate(db, predicates, visibility_constraint) + }) + .unwrap_or(Truthiness::AlwaysFalse) }; let mut types = bindings_with_constraints.filter_map( @@ -733,7 +742,7 @@ fn symbol_from_bindings_impl<'db>( // code. However, it is still okay to return `Never` in this case, because we will // union the types of all bindings, and `Never` will be eliminated automatically. - if unbound_visibility.is_always_false() { + if unbound_visibility().is_always_false() { // The scope-start is not visible return Some(Type::Never); } @@ -762,7 +771,7 @@ fn symbol_from_bindings_impl<'db>( ); if let Some(first) = types.next() { - let boundness = match unbound_visibility { + let boundness = match unbound_visibility() { Truthiness::AlwaysTrue => { unreachable!("If we have at least one binding, the scope-start should not be definitely visible") } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index d6e8cc1ed2..8f8d9b2f46 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -7849,6 +7849,50 @@ mod tests { check_typevar("Y", None, None, None); } + /// Test that a symbol known to be unbound in a scope does not still trigger cycle-causing + /// visibility-constraint checks in that scope. + #[test] + fn unbound_symbol_no_visibility_constraint_check() { + let mut db = setup_db(); + + // If the bug we are testing for is not fixed, what happens is that when inferring the + // `flag: bool = True` definitions, we look up `bool` as a deferred name (thus from end of + // scope), and because of the early return its "unbound" binding has a visibility + // constraint of `~flag`, which we evaluate, meaning we have to evaluate the definition of + // `flag` -- and we are in a cycle. With the fix, we short-circuit evaluating visibility + // constraints on "unbound" if a symbol is otherwise not bound. + db.write_dedented( + "src/a.py", + " + from __future__ import annotations + + def f(): + flag: bool = True + if flag: + return True + ", + ) + .unwrap(); + + db.clear_salsa_events(); + assert_file_diagnostics(&db, "src/a.py", &[]); + let events = db.take_salsa_events(); + let cycles = salsa::plumbing::attach(&db, || { + events + .iter() + .filter_map(|event| { + if let salsa::EventKind::WillIterateCycle { database_key, .. } = event.kind { + Some(format!("{database_key:?}")) + } else { + None + } + }) + .collect::>() + }); + let expected: Vec = vec![]; + assert_eq!(cycles, expected); + } + // Incremental inference tests #[track_caller] fn first_public_binding<'db>(db: &'db TestDb, file: File, name: &str) -> Definition<'db> { diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 862f67e67e..2027caaefb 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 = "b165ba7bd1b2a0112ce574a082ab8ea5102252ac" } +salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "87bf6b6c2d5f6479741271da73bd9d30c2580c26" } similar = { version = "2.5.0" } tracing = { version = "0.1.40" }