[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

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

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

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

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::<Vec<_>>()
+        });
+        let expected: Vec<String> = vec![];
+        assert_eq!(cycles, expected);
+    }
+
     // Incremental inference tests
     #[track_caller]
     fn first_public_binding<'db>(db: &'db TestDb, file: File, name: &str) -> Definition<'db> {

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