Skip to content

Commit

Permalink
[red-knot] Improve symbol-lookup tracing (#14907)
Browse files Browse the repository at this point in the history 
## 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
```
  • Loading branch information
@sharkdp
sharkdp authored Jan 7, 2025
1 parent 066239f commit ce9c496
Showing 1 changed file with 63 additions and 60 deletions.
123 changes: 63 additions & 60 deletions crates/red_knot_python_semantic/src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

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);
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 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)
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);

// 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);

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)
}
// 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"
Expand Down

0 comments on commit ce9c496

Please sign in to comment.