From ce9c4968ae750108e6f896c43cceea63ec3c39fb Mon Sep 17 00:00:00 2001 From: David Peter Date: Tue, 7 Jan 2025 10:41:27 +0100 Subject: [PATCH] [red-knot] Improve symbol-lookup tracing (#14907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary When debugging, I frequently want to know which symbols are being looked up. `symbol_by_id` adds tracing information, but it only shows the `ScopedSymbolId`. Since `symbol_by_id` is only called from `symbol`, it seems reasonable to move the tracing call one level up from `symbol_by_id` to `symbol`, where we can also show the name of the symbol. **Before**: ``` 6 └─┐red_knot_python_semantic::types::infer::infer_expression_types{expression=Id(60de), file=/home/shark/tomllib_modified/_parser.py} 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(33)} 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(123)} 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(54)} 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(122)} 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(165)} 6 ┌─┘ 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(32)} 6 ┌─┘ 6 └─┐red_knot_python_semantic::types::symbol_by_id{symbol=ScopedSymbolId(232)} 6 ┌─┘ 6 ┌─┘ 6 ┌─┘ 6┌─┘ ``` **After**: ``` 5 └─┐red_knot_python_semantic::types::infer::infer_expression_types{expression=Id(60de), file=/home/shark/tomllib_modified/_parser.py} 5 └─┐red_knot_python_semantic::types::symbol{name="dict"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="dict"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="list"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="list"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="isinstance"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="isinstance"} 5 ┌─┘ 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="ValueError"} 5 ┌─┘ 5 └─┐red_knot_python_semantic::types::symbol{name="ValueError"} 5 ┌─┘ 5 ┌─┘ 5 ┌─┘ 5┌─┘ ``` ## Test Plan ``` cargo run --bin red_knot -- --current-directory path/to/tomllib -vvv ``` --- crates/red_knot_python_semantic/src/types.rs | 115 ++++++++++--------- 1 file changed, 59 insertions(+), 56 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 269b754919..f3356b9320 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -74,72 +74,75 @@ pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { } /// Infer the public type of a symbol (its type as seen from outside its scope). -#[salsa::tracked] -fn symbol_by_id<'db>(db: &'db dyn Db, scope: ScopeId<'db>, symbol: ScopedSymbolId) -> Symbol<'db> { - let _span = tracing::trace_span!("symbol_by_id", ?symbol).entered(); +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>, + symbol: ScopedSymbolId, + ) -> Symbol<'db> { + let use_def = use_def_map(db, scope); - 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. - // 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); + let declared = declarations_ty(db, declarations); - let declarations = use_def.public_declarations(symbol); - let declared = declarations_ty(db, declarations); + 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); + let inferred = bindings_ty(db, bindings); - 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); - let inferred = bindings_ty(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) + 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].iter().copied()), + boundness, + ), } - // Symbol is possibly undeclared and (possibly) bound - Symbol::Type(inferred_ty, boundness) => Symbol::Type( - UnionType::from_elements(db, [inferred_ty, declared_ty].iter().copied()), - boundness, - ), + } + // Symbol is undeclared, return the inferred type + Ok(Symbol::Unbound) => { + let bindings = use_def.public_bindings(symbol); + bindings_ty(db, bindings) + } + // Symbol is possibly undeclared + Err((declared_ty, _)) => { + // Intentionally ignore conflicting declared types; that's not our problem, + // it's the problem of the module we are importing from. + declared_ty.into() } } - // Symbol is undeclared, return the inferred type - Ok(Symbol::Unbound) => { - let bindings = use_def.public_bindings(symbol); - bindings_ty(db, bindings) - } - // Symbol is possibly undeclared - Err((declared_ty, _)) => { - // Intentionally ignore conflicting declared types; that's not our problem, - // it's the problem of the module we are importing from. - declared_ty.into() - } + + // 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.) } - // 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(); -/// Shorthand for `symbol_by_id` that takes a symbol name instead of an ID. -fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> { // We don't need to check for `typing_extensions` here, because `typing_extensions.TYPE_CHECKING` // is just a re-export of `typing.TYPE_CHECKING`. if name == "TYPE_CHECKING"