Skip to content

[red-knot] Improve symbol-lookup tracing #14907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sharkdp merged 1 commit into from
Jan 7, 2025
Merged

[red-knot] Improve symbol-lookup tracing #14907

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 63 additions & 60 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
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();
Copy link
Member

@MichaReiser MichaReiser Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to inline the entire symbol_by_id function into symbol to avoid "missing" symbol lookups in tracing because a symbol gets looked up by its id.

Copy link
Contributor Author

@sharkdp sharkdp Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symbol_by_id is now #[salsa::tracked], so I can't inline it trivially. Also, it feels a bit wrong to inline that nicely encapsulated into this lambda here:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 171 to 175 in 897c307

let table = symbol_table(db, scope);
table
.symbol_id_by_name(name)
.map(|symbol| symbol_by_id(db, scope, symbol))
.unwrap_or(Symbol::Unbound)

I now pushed a version where I nested symbol_by_id inside symbol. I'm not a huge fan, but it solves your concern as well. What do you think @MichaReiser?

(The diff is horrible. The only thing I did was to move symbol_by_id inside symbol and adapted the doc comments. No code has been changed except for the removed/added tracing::trace_span line.

Copy link
Member

@dcreager dcreager Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The diff is horrible. The only thing I did was to move symbol_by_id inside symbol and adapted the doc comments. No code has been changed except for the removed/added tracing::trace_span line.

fyi if you click the gear icon at the top of the diff view, you can check "Hide whitespace", which makes this particular diff much cleaner

Copy link
Contributor Author

@sharkdp sharkdp Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I know about hide-whitespace, but somehow didn't think of enabling it for this changeset 👍


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>,
Copy link
Contributor Author

@sharkdp sharkdp Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to remove this (and use scope from the outer scope), but it seems to be required in order to make this salsa_tracked.

Copy link
Member

@MichaReiser MichaReiser Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me. Thanks

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
Loading