[red-knot] Improve symbol-lookup tracing by sharkdp · Pull Request #14907 · astral-sh/ruff

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


 /// 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 ty Multi-file analysis & type inference label Dec 11, 2024

sharkdp requested review from AlexWaygood, MichaReiser and carljm 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

Comments

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

[red-knot] Improve symbol-lookup tracing#14907
sharkdp merged 1 commit intomainfrom
david/tracing-symbol-lookups

sharkdp commented Dec 11, 2024 •

edited

Loading

Uh oh!

MichaReiser Dec 11, 2024

Uh oh!

sharkdp Jan 7, 2025 •

edited

Loading

Uh oh!

dcreager Jan 7, 2025

Uh oh!

sharkdp Jan 7, 2025

Uh oh!

github-actions bot commented Dec 11, 2024 •

edited

Loading

Uh oh!

AlexWaygood left a comment •

edited

Loading

Uh oh!

carljm left a comment

Uh oh!

sharkdp Jan 7, 2025

Uh oh!

MichaReiser Jan 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

Comments

Conversation

sharkdp commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

MichaReiser Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

sharkdp Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcreager Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

sharkdp Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

sharkdp Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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