Cached inference of all definitions in an unpacking by dhruvmanila · Pull Request #13979 · astral-sh/ruff

dhruvmanila · 2024-10-29T11:12:42Z

Summary

This PR adds a new salsa query and an ingredient to resolve all the variables involved in an unpacking assignment like (a, b) = (1, 2) at once. Previously, we'd recursively try to match the correct type for each definition individually which will result in creating duplicate diagnostics.

This PR still doesn't solve the duplicate diagnostics issue because that requires a different solution like using salsa accumulator or de-duplicating the diagnostics manually.

Related: #13773

Test Plan

Make sure that all unpack assignment test cases pass, there are no panics in the corpus tests.

Todo

Look at the performance regression

codspeed-hq · 2024-10-29T11:18:02Z

CodSpeed Performance Report

Merging #13979 will not alter performance

_{Comparing dhruv/unpack (c039525) with main (012f385)}

Summary

✅ 32 untouched benchmarks

crates/red_knot_python_semantic/src/types/infer.rs

## Summary This PR creates a new `TypeCheckDiagnosticsBuilder` for the `TypeCheckDiagnostics` struct. The main motivation behind this is to separate the helpers required to build the diagnostics from the type inference builder itself. This allows us to use such helpers outside of the inference builder like for example in the unpacking logic in #13979. ## Test Plan `cargo insta test`

github-actions · 2024-10-30T19:11:38Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

dhruvmanila · 2024-11-01T11:29:04Z

I'm not exactly sure what's causing the performance regression. It might very well be the fact that this PR creates additional ingredients and goes through a new salsa query which adds the overhead. This is because the tomllib contains multiple assignment statements where unpacking is required:

1red_knot_python_semantic::types::unpack::Unpack                             55           55           55

MichaReiser · 2024-11-01T11:55:21Z

Is it only that there are 55 new unpackings, or are there other ingredients with a different count?

dhruvmanila · 2024-11-01T12:57:58Z

Is it only that there are 55 new unpackings, or are there other ingredients with a different count?

There are other ingredients with different count:

main

1red_knot_python_semantic::semantic_index::definition::Definition         6_643        6_643        6_643
1red_knot_python_semantic::semantic_index::expression::Expression           553          553          553
1red_knot_python_semantic::semantic_index::symbol::ScopeId                1_978        1_978        1_978
1ruff_db::files::File                                                        59           59           59
1ruff_db::source::SourceText                                                 15           15           15
1                                                                         total     max_live         live

dhruv/unpack

1red_knot_python_semantic::semantic_index::definition::Definition         6_620        6_620        6_620
1red_knot_python_semantic::semantic_index::expression::Expression           549          549          549
1red_knot_python_semantic::semantic_index::symbol::ScopeId                1_974        1_974        1_974
1red_knot_python_semantic::types::unpack::Unpack                             55           55           55
1ruff_db::files::File                                                        59           59           59
1ruff_db::source::SourceText                                                 15           15           15
1                                                                         total     max_live         live

dhruvmanila · 2024-11-01T13:03:09Z

Oh ok, I got it. I think the Unpack ingredient is not well isolated for each statement.

dhruvmanila · 2024-11-01T13:23:41Z

Actually, I think it's because I'm creating the ingredient multiple times. I think I need to create it once in the semantic index and store it.

dhruvmanila · 2024-11-01T15:00:36Z

(I've put this in draft to fix the regression.)

crates/red_knot_python_semantic/src/types/unpacker.rs

crates/red_knot_python_semantic/src/unpack.rs

carljm

This is great, thank you!! And thanks for starting the process of slimming down infer.rs a little bit :)

carljm · 2024-11-04T19:57:20Z

crates/red_knot_python_semantic/src/semantic_index/builder.rs

+    /// The [`Unpack`] ingredient for the current definition that belongs to an unpacking
+    /// assignment. This is used to correctly map multiple definitions to the *same* unpacking.
+    /// For example, in `a, b = 1, 2`, both `a` and `b` creates separate definitions but they both
+    /// belong to the same unpacking.
+    current_unpack: Option<Unpack<'db>>,


This is a small nit, but it seems like maybe this could be part of CurrentAssignment rather than its own separately-maintained state? But maybe I've missed some subtlety why that can't work.

Yeah, I initially started with moving this information via CurrentAssignment -> DefinitionNodeRef -> DefinitionKind but that'll involve adding lifetimes and moving them around because Unpack uses a different lifetime while CurrentAssignment and DefinitionNodeRef uses the AST lifetime. But, now that I look at it again I think it might work.

Opened #14101

carljm · 2024-11-04T20:02:55Z

crates/red_knot_python_semantic/src/types/infer.rs

+                let unpacked = infer_unpack_types(self.db, unpack);
+                self.diagnostics.extend(unpacked.diagnostics());


I guess the quick-fix approach to the duplicate diagnostics would be to track on self which unpacks we've queried already, and if we've seen it already, don't merge the diagnostics.

But I think it makes sense to see if accumulators can work, since that's a more general approach that doesn't require us to manually track when we might call a sub-query more than once.

Yeah, I suggest not to spend more time on deduplicating diagnostics because I plan to look into this soon anyway

* main: (39 commits) Also remove trailing comma while fixing C409 and C419 (astral-sh#14097) Re-enable clippy `useless-format` (astral-sh#14095) Derive message formats macro support to string (astral-sh#14093) Avoid cloning `Name` when looking up function and class types (astral-sh#14092) Replace `format!` without parameters with `.to_string()` (astral-sh#14090) [red-knot] Do not panic when encountering string annotations (astral-sh#14091) [red-knot] Add MRO resolution for classes (astral-sh#14027) [red-knot] Remove `Type::None` (astral-sh#14024) Cached inference of all definitions in an unpacking (astral-sh#13979) Update dependency uuid to v11 (astral-sh#14084) Update Rust crate notify to v7 (astral-sh#14083) Update cloudflare/wrangler-action action to v3.11.0 (astral-sh#14080) Update dependency mdformat-mkdocs to v3.1.1 (astral-sh#14081) Update pre-commit dependencies (astral-sh#14082) Update dependency ruff to v0.7.2 (astral-sh#14077) Update NPM Development dependencies (astral-sh#14078) Update Rust crate thiserror to v1.0.67 (astral-sh#14076) Update Rust crate syn to v2.0.87 (astral-sh#14075) Update Rust crate serde to v1.0.214 (astral-sh#14074) Update Rust crate pep440_rs to v0.7.2 (astral-sh#14073) ...

## Summary Related to #13979 (comment), this PR removes the `current_unpack` state field from `SemanticIndexBuilder` and passes the `Unpack` ingredient via the `CurrentAssignment` -> `DefinitionNodeRef` conversion to finally store it on `DefintionNodeKind`. This involves updating the lifetime of `AnyParameterRef` (parameter to `declare_parameter`) to use the `'db` lifetime. Currently, all AST nodes stored on various enums are marked with `'a` lifetime but they're always utilized using the `'db` lifetime. This also removes the dedicated `'a` lifetime parameter on `add_definition` which is currently being used in `DefinitionNodeRef`. As mentioned, all AST nodes live through the `'db` lifetime so we can remove the `'a` lifetime parameter from that method and use the `'db` lifetime instead.

dhruvmanila added the ty Multi-file analysis & type inference label Oct 29, 2024

dhruvmanila force-pushed the dhruv/diagnostics-builder branch 2 times, most recently from 5cbd74e to 257ce5a Compare October 30, 2024 08:10

dhruvmanila force-pushed the dhruv/unpack branch 2 times, most recently from 71c4911 to c03f987 Compare October 30, 2024 10:05

dhruvmanila force-pushed the dhruv/diagnostics-builder branch from 257ce5a to bd0d782 Compare October 30, 2024 10:05

dhruvmanila mentioned this pull request Oct 30, 2024

Separate type check diagnostics builder #13978

Merged

dhruvmanila commented Oct 30, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Show resolved Hide resolved

dhruvmanila force-pushed the dhruv/diagnostics-builder branch from 8d3a2ff to b50d38c Compare October 30, 2024 18:45

Base automatically changed from dhruv/diagnostics-builder to main October 30, 2024 18:50

dhruvmanila force-pushed the dhruv/unpack branch from c03f987 to 77aae83 Compare October 30, 2024 18:56

dhruvmanila added 2 commits November 1, 2024 10:31

Cached inference of all definitions involved in unpacking

28c0dfe

Restructure assignment definition for unpacking targets

f9c2f15

dhruvmanila force-pushed the dhruv/unpack branch from 77aae83 to f9c2f15 Compare November 1, 2024 09:57

dhruvmanila marked this pull request as ready for review November 1, 2024 11:29

dhruvmanila requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners November 1, 2024 11:29

dhruvmanila marked this pull request as draft November 1, 2024 13:24

dhruvmanila force-pushed the dhruv/unpack branch from 66818b0 to 99470b2 Compare November 4, 2024 06:35

dhruvmanila force-pushed the dhruv/unpack branch 2 times, most recently from 1b40c22 to cc123b1 Compare November 4, 2024 07:20

Restructure unpack ingredient to avoid duplications

4a557ac

dhruvmanila force-pushed the dhruv/unpack branch from cc123b1 to 4a557ac Compare November 4, 2024 07:21

dhruvmanila marked this pull request as ready for review November 4, 2024 07:21

MichaReiser approved these changes Nov 4, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/unpacker.rs Outdated Show resolved Hide resolved

crates/red_knot_python_semantic/src/types/unpacker.rs Show resolved Hide resolved

crates/red_knot_python_semantic/src/unpack.rs Show resolved Hide resolved

Add documentation

c039525

dhruvmanila merged commit e302c2d into main Nov 4, 2024

dhruvmanila deleted the dhruv/unpack branch November 4, 2024 11:41

carljm reviewed Nov 4, 2024

View reviewed changes

dhruvmanila mentioned this pull request Nov 5, 2024

Remove unpack field from SemanticIndexBuilder #14101

Merged

dhruvmanila linked an issue Dec 17, 2024 that may be closed by this pull request

[red-knot] Follow-up from unpacked tuple assignment work #13773

Closed

6 tasks

		let unpacked = infer_unpack_types(self.db, unpack);
		self.diagnostics.extend(unpacked.diagnostics());

Conversation

dhruvmanila commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Todo

Uh oh!

codspeed-hq bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #13979 will not alter performance

Summary

Uh oh!

Uh oh!

github-actions bot commented Oct 30, 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!

dhruvmanila commented Nov 1, 2024

Uh oh!

MichaReiser commented Nov 1, 2024

Uh oh!

dhruvmanila commented Nov 1, 2024

Uh oh!

dhruvmanila commented Nov 1, 2024

Uh oh!

dhruvmanila commented Nov 1, 2024

Uh oh!

dhruvmanila commented Nov 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

carljm Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhruvmanila commented Oct 29, 2024 •

edited

Loading

codspeed-hq bot commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024 •

edited

Loading

`ruff-ecosystem` results