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

sharkdp · 2024-12-11T08:41:18Z

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

MichaReiser · 2024-12-11T08:44:41Z

crates/red_knot_python_semantic/src/types.rs

@@ -64,8 +64,6 @@ 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).
 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();


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.

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.

(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

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

github-actions · 2024-12-11T08:47:27Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

AlexWaygood

I agree with Micha's comment, but LGTM otherwise. (Wow, that output is so much more useful!)

carljm

So much better, thank you!

When debugging, I frequently want to know which symbols are be 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 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┌─┘ ```

sharkdp · 2025-01-07T09:35:20Z

crates/red_knot_python_semantic/src/types.rs

+    #[salsa::tracked]
+    fn symbol_by_id<'db>(
+        db: &'db dyn Db,
+        scope: ScopeId<'db>,


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.

This works for me. Thanks

* main: Use uv consistently throughout the documentation (#15302) [red-knot] Eagerly normalize `type[]` types (#15272) [`pyupgrade`] Split `UP007` to two individual rules for `Union` and `Optional` (`UP007`, `UP045`) (#15313) [red-knot] Improve symbol-lookup tracing (#14907) [red-knot] improve type shrinking coverage in red-knot property tests (#15297) [`flake8-return`] Recognize functions returning `Never` as non-returning (`RET503`) (#15298) [`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601) Avoid treating newline-separated sections as sub-sections (#15311) Remove call when removing final argument from `format` (#15309) Don't enforce `object-without-hash-method` in stubs (#15310) Don't special-case class instances in binary expression inference (#15161) Upgrade zizmor to the latest version in CI (#15300)

sharkdp added the red-knot Multi-file analysis & type inference label Dec 11, 2024

sharkdp requested review from carljm, MichaReiser and AlexWaygood as code owners December 11, 2024 08:41

MichaReiser approved these changes Dec 11, 2024

View reviewed changes

AlexWaygood approved these changes Dec 11, 2024

View reviewed changes

carljm approved these changes Dec 11, 2024

View reviewed changes

sharkdp force-pushed the david/tracing-symbol-lookups branch from c16c17e to 5529405 Compare January 7, 2025 09:25

sharkdp force-pushed the david/tracing-symbol-lookups branch from 5529405 to 897c307 Compare January 7, 2025 09:30

sharkdp commented Jan 7, 2025

View reviewed changes

sharkdp merged commit ce9c496 into main Jan 7, 2025
21 checks passed

sharkdp deleted the david/tracing-symbol-lookups branch January 7, 2025 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sharkdp commented Dec 11, 2024 •

edited

Loading

MichaReiser Dec 11, 2024

sharkdp Jan 7, 2025 •

edited

Loading

dcreager Jan 7, 2025

sharkdp Jan 7, 2025

github-actions bot commented Dec 11, 2024 •

edited

Loading

AlexWaygood left a comment •

edited

Loading

carljm left a comment

sharkdp Jan 7, 2025

MichaReiser Jan 7, 2025

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

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

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

Conversation

sharkdp commented Dec 11, 2024 • edited Loading

Summary

Test Plan

MichaReiser Dec 11, 2024

Choose a reason for hiding this comment

sharkdp Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

dcreager Jan 7, 2025

Choose a reason for hiding this comment

sharkdp Jan 7, 2025

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

AlexWaygood left a comment • edited Loading

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

sharkdp Jan 7, 2025

Choose a reason for hiding this comment

MichaReiser Jan 7, 2025

Choose a reason for hiding this comment

sharkdp commented Dec 11, 2024 •

edited

Loading

sharkdp Jan 7, 2025 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

`ruff-ecosystem` results

AlexWaygood left a comment •

edited

Loading