TEST: Make symbol_by_id not a salsa query

crates/red_knot_python_semantic/src/types.rs

@@ -15,7 +15,8 @@ pub(crate) use self::diagnostic::register_lints;
 pub use self::diagnostic::{TypeCheckDiagnostic, TypeCheckDiagnostics};
 pub(crate) use self::display::TypeArrayDisplay;
 pub(crate) use self::infer::{
-    infer_deferred_types, infer_definition_types, infer_expression_types, infer_scope_types,
+    infer_deferred_types, infer_definition_types, infer_expression_type, infer_expression_types,
+    infer_scope_types,
 };
 pub use self::narrow::KnownConstraintFunction;
 pub(crate) use self::signatures::Signature;
@@ -25,8 +26,7 @@ use crate::module_resolver::{file_to_module, resolve_module, KnownModule};
 use crate::semantic_index::ast_ids::HasScopedExpressionId;
 use crate::semantic_index::attribute_assignment::AttributeAssignment;
 use crate::semantic_index::definition::Definition;
-use crate::semantic_index::expression::Expression;
-use crate::semantic_index::symbol::{self as symbol, ScopeId, ScopedSymbolId};
+use crate::semantic_index::symbol::{self as symbol, ScopeId};
 use crate::semantic_index::{
     attribute_assignments, global_scope, imported_modules, semantic_index, symbol_table,
     use_def_map, BindingWithConstraints, BindingWithConstraintsIterator, DeclarationWithConstraint,
@@ -108,77 +108,6 @@ fn widen_type_for_undeclared_public_symbol<'db>(
 
 /// Infer the public type of a symbol (its type as seen from outside its scope).
 fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> {
-    #[salsa::tracked]
-    fn symbol_by_id<'db>(
-        db: &'db dyn Db,
-        scope: ScopeId<'db>,
-        is_dunder_slots: bool,
-        symbol_id: ScopedSymbolId,
-    ) -> Symbol<'db> {
-        let use_def = use_def_map(db, scope);
-
-        // If the symbol is declared, the public type is based on declarations; otherwise, it's based
-        // on inference from bindings.
-
-        let declarations = use_def.public_declarations(symbol_id);
-        let declared = symbol_from_declarations(db, declarations);
-        let is_final = declared.as_ref().is_ok_and(SymbolAndQualifiers::is_final);
-        let declared = declared.map(|SymbolAndQualifiers(symbol, _)| symbol);
-
-        match declared {
-            // Symbol is declared, trust the declared type
-            Ok(symbol @ Symbol::Type(_, Boundness::Bound)) => symbol,
-            // Symbol is possibly declared
-            Ok(Symbol::Type(declared_ty, Boundness::PossiblyUnbound)) => {
-                let bindings = use_def.public_bindings(symbol_id);
-                let inferred = symbol_from_bindings(db, bindings);
-
-                match inferred {
-                    // Symbol is possibly undeclared and definitely unbound
-                    Symbol::Unbound => {
-                        // TODO: We probably don't want to report `Bound` here. This requires a bit of
-                        // design work though as we might want a different behavior for stubs and for
-                        // normal modules.
-                        Symbol::Type(declared_ty, Boundness::Bound)
-                    }
-                    // Symbol is possibly undeclared and (possibly) bound
-                    Symbol::Type(inferred_ty, boundness) => Symbol::Type(
-                        UnionType::from_elements(db, [inferred_ty, declared_ty]),
-                        boundness,
-                    ),
-                }
-            }
-            // Symbol is undeclared, return the union of `Unknown` with the inferred type
-            Ok(Symbol::Unbound) => {
-                let bindings = use_def.public_bindings(symbol_id);
-                let inferred = symbol_from_bindings(db, bindings);
-
-                widen_type_for_undeclared_public_symbol(db, inferred, is_dunder_slots || is_final)
-            }
-            // Symbol has conflicting declared types
-            Err((declared_ty, _)) => {
-                // Intentionally ignore conflicting declared types; that's not our problem,
-                // it's the problem of the module we are importing from.
-                Symbol::bound(declared_ty.inner_type())
-            }
-        }
-
-        // TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
-        // currently only depends on bindings, and ignores declarations. This is inconsistent, since
-        // we only look at bindings if the symbol may be undeclared. Consider the following example:
-        // ```py
-        // x: int
-        //
-        // if flag:
-        //     y: int
-        // else
-        //     y = 3
-        // ```
-        // If we import from this module, we will currently report `x` as a definitely-bound symbol
-        // (even though it has no bindings at all!) but report `y` as possibly-unbound (even though
-        // every path has either a binding or a declaration for it.)
-    }
-
     let _span = tracing::trace_span!("symbol", ?name).entered();
 
     // We don't need to check for `typing_extensions` here, because `typing_extensions.TYPE_CHECKING`
@@ -204,16 +133,80 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db>
     }
 
     let table = symbol_table(db, scope);
+
     // `__slots__` is a symbol with special behavior in Python's runtime. It can be
     // modified externally, but those changes do not take effect. We therefore issue
     // a diagnostic if we see it being modified externally. In type inference, we
     // can assign a "narrow" type to it even if it is not *declared*. This means, we
     // do not have to call [`widen_type_for_undeclared_public_symbol`].
     let is_dunder_slots = name == "__slots__";
-    table
-        .symbol_id_by_name(name)
-        .map(|symbol| symbol_by_id(db, scope, is_dunder_slots, symbol))
-        .unwrap_or(Symbol::Unbound)
+
+    let Some(symbol_id) = table.symbol_id_by_name(name) else {
+        return Symbol::Unbound;
+    };
+
+    let use_def = use_def_map(db, scope);
+
+    // If the symbol is declared, the public type is based on declarations; otherwise, it's based
+    // on inference from bindings.
+
+    let declarations = use_def.public_declarations(symbol_id);
+    let declared = symbol_from_declarations(db, declarations);
+    let is_final = declared.as_ref().is_ok_and(SymbolAndQualifiers::is_final);
+    let declared = declared.map(|SymbolAndQualifiers(symbol, _)| symbol);
+
+    match declared {
+        // Symbol is declared, trust the declared type
+        Ok(symbol @ Symbol::Type(_, Boundness::Bound)) => symbol,
+        // Symbol is possibly declared
+        Ok(Symbol::Type(declared_ty, Boundness::PossiblyUnbound)) => {
+            let bindings = use_def.public_bindings(symbol_id);
+            let inferred = symbol_from_bindings(db, bindings);
+
+            match inferred {
+                // Symbol is possibly undeclared and definitely unbound
+                Symbol::Unbound => {
+                    // TODO: We probably don't want to report `Bound` here. This requires a bit of
+                    // design work though as we might want a different behavior for stubs and for
+                    // normal modules.
+                    Symbol::Type(declared_ty, Boundness::Bound)
+                }
+                // Symbol is possibly undeclared and (possibly) bound
+                Symbol::Type(inferred_ty, boundness) => Symbol::Type(
+                    UnionType::from_elements(db, [inferred_ty, declared_ty]),
+                    boundness,
+                ),
+            }
+        }
+        // Symbol is undeclared, return the union of `Unknown` with the inferred type
+        Ok(Symbol::Unbound) => {
+            let bindings = use_def.public_bindings(symbol_id);
+            let inferred = symbol_from_bindings(db, bindings);
+
+            widen_type_for_undeclared_public_symbol(db, inferred, is_dunder_slots || is_final)
+        }
+        // Symbol has conflicting declared types
+        Err((declared_ty, _)) => {
+            // Intentionally ignore conflicting declared types; that's not our problem,
+            // it's the problem of the module we are importing from.
+            Symbol::bound(declared_ty.inner_type())
+        }
+    }
+
+    // TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
+    // currently only depends on bindings, and ignores declarations. This is inconsistent, since
+    // we only look at bindings if the symbol may be undeclared. Consider the following example:
+    // ```py
+    // x: int
+    //
+    // if flag:
+    //     y: int
+    // else
+    //     y = 3
+    // ```
+    // If we import from this module, we will currently report `x` as a definitely-bound symbol
+    // (even though it has no bindings at all!) but report `y` as possibly-unbound (even though
+    // every path has either a binding or a declaration for it.)
 }
 
 /// Return a list of the symbols that typeshed declares in the body scope of
@@ -4189,16 +4182,6 @@ impl<'db> Class<'db> {
         name: &str,
         inferred_type_from_class_body: Option<Type<'db>>,
     ) -> Symbol<'db> {
-        // We use a separate salsa query here to prevent unrelated changes in the AST of an external
-        // file from triggering re-evaluations of downstream queries.
-        // See the `dependency_implicit_instance_attribute` test for more information.
-        #[salsa::tracked]
-        fn infer_expression_type<'db>(db: &'db dyn Db, expression: Expression<'db>) -> Type<'db> {
-            let inference = infer_expression_types(db, expression);
-            let expr_scope = expression.scope(db);
-            inference.expression_type(expression.node_ref(db).scoped_expression_id(db, expr_scope))
-        }
-
         // If we do not see any declarations of an attribute, neither in the class body nor in
         // any method, we build a union of `Unknown` with the inferred types of all bindings of
         // that attribute. We include `Unknown` in that union to account for the fact that the

crates/red_knot_python_semantic/src/types/infer.rs

@@ -194,6 +194,20 @@ pub(crate) fn infer_expression_types<'db>(
     TypeInferenceBuilder::new(db, InferenceRegion::Expression(expression), index).finish()
 }
 
+// Similar to `infer_expression_types` (with the same restrictions). Directly returns the
+// type of the overall expression. This is a salsa query because it accesses `node_ref`,
+// which is sensitive to changes in the AST. Making it a query allows downstream queries
+// to short-circuit if the result type has not changed.
+#[salsa::tracked]
+pub(crate) fn infer_expression_type<'db>(
+    db: &'db dyn Db,
+    expression: Expression<'db>,
+) -> Type<'db> {
+    let inference = infer_expression_types(db, expression);
+    let expr_scope = expression.scope(db);
+    inference.expression_type(expression.node_ref(db).scoped_expression_id(db, expr_scope))
+}
+
 /// Infer the types for an [`Unpack`] operation.
 ///
 /// This infers the expression type and performs structural match against the target expression

crates/red_knot_python_semantic/src/types/unpacker.rs

@@ -7,7 +7,7 @@ use ruff_python_ast::{self as ast, AnyNodeRef};
 
 use crate::semantic_index::ast_ids::{HasScopedExpressionId, ScopedExpressionId};
 use crate::semantic_index::symbol::ScopeId;
-use crate::types::{infer_expression_types, todo_type, Type, TypeCheckDiagnostics};
+use crate::types::{infer_expression_type, todo_type, Type, TypeCheckDiagnostics};
 use crate::unpack::UnpackValue;
 use crate::Db;
 
@@ -42,8 +42,7 @@ impl<'db> Unpacker<'db> {
             "Unpacking target must be a list or tuple expression"
         );
 
-        let mut value_ty = infer_expression_types(self.db(), value.expression())
-            .expression_type(value.scoped_expression_id(self.db(), self.scope));
+        let mut value_ty = infer_expression_type(self.db(), value.expression());
 
         if value.is_assign()
             && self.context.in_stub()

crates/red_knot_python_semantic/src/unpack.rs

@@ -3,7 +3,6 @@ use ruff_python_ast::{self as ast, AnyNodeRef};
 use ruff_text_size::{Ranged, TextRange};
 
 use crate::ast_node_ref::AstNodeRef;
-use crate::semantic_index::ast_ids::{HasScopedExpressionId, ScopedExpressionId};
 use crate::semantic_index::expression::Expression;
 use crate::semantic_index::symbol::{FileScopeId, ScopeId};
 use crate::Db;
@@ -86,17 +85,6 @@ impl<'db> UnpackValue<'db> {
         }
     }
 
-    /// Returns the [`ScopedExpressionId`] of the underlying expression.
-    pub(crate) fn scoped_expression_id(
-        self,
-        db: &'db dyn Db,
-        scope: ScopeId<'db>,
-    ) -> ScopedExpressionId {
-        self.expression()
-            .node_ref(db)
-            .scoped_expression_id(db, scope)
-    }
-
     /// Returns the expression as an [`AnyNodeRef`].
     pub(crate) fn as_any_node_ref(self, db: &'db dyn Db) -> AnyNodeRef<'db> {
         self.expression().node_ref(db).node().into()

crates/red_knot_python_semantic/src/visibility_constraints.rs

@@ -178,11 +178,8 @@ use std::cmp::Ordering;
 use ruff_index::{Idx, IndexVec};
 use rustc_hash::FxHashMap;
 
-use crate::semantic_index::{
-    ast_ids::HasScopedExpressionId,
-    constraint::{Constraint, ConstraintNode, PatternConstraintKind},
-};
-use crate::types::{infer_expression_types, Truthiness};
+use crate::semantic_index::constraint::{Constraint, ConstraintNode, PatternConstraintKind};
+use crate::types::{infer_expression_type, Truthiness};
 use crate::Db;
 
 /// A ternary formula that defines under what conditions a binding is visible. (A ternary formula
@@ -617,28 +614,15 @@ impl<'db> VisibilityConstraints<'db> {
     fn analyze_single(db: &dyn Db, constraint: &Constraint) -> Truthiness {
         match constraint.node {
             ConstraintNode::Expression(test_expr) => {
-                let inference = infer_expression_types(db, test_expr);
-                let scope = test_expr.scope(db);
-                let ty = inference
-                    .expression_type(test_expr.node_ref(db).scoped_expression_id(db, scope));
+                let ty = infer_expression_type(db, test_expr);
 
                 ty.bool(db).negate_if(!constraint.is_positive)
             }
             ConstraintNode::Pattern(inner) => match inner.kind(db) {
                 PatternConstraintKind::Value(value, guard) => {
                     let subject_expression = inner.subject(db);
-                    let inference = infer_expression_types(db, subject_expression);
-                    let scope = subject_expression.scope(db);
-                    let subject_ty = inference.expression_type(
-                        subject_expression
-                            .node_ref(db)
-                            .scoped_expression_id(db, scope),
-                    );
-
-                    let inference = infer_expression_types(db, *value);
-                    let scope = value.scope(db);
-                    let value_ty = inference
-                        .expression_type(value.node_ref(db).scoped_expression_id(db, scope));
+                    let subject_ty = infer_expression_type(db, subject_expression);
+                    let value_ty = infer_expression_type(db, *value);
 
                     if subject_ty.is_single_valued(db) {
                         let truthiness =

crates/ruff_python_parser/src/token.rs

@@ -72,7 +72,8 @@ impl Token {
             .unwrap_or_else(|| panic!("token to be a string"))
     }
 
-    /// Returns true if the current token is a string and it is raw.
+    /// Returns the [`AnyStringFlags`] style for the current token of any string kind, or [`None`]
+    /// if the token is not a string.
     pub fn string_flags(self) -> Option<AnyStringFlags> {
         if self.is_any_string() {
             Some(self.flags.as_any_string_flags())