refactor: Lower type-refs before type inference #19462

Veykril · 2025-03-27T08:32:07Z

This PR refactors how we deal with items in hir-def lowering.

It now lowers all of them through an "ExpressionStore" (kind of a misnomer as this point) as their so called *Signatures.
We now uniformly lower type AST into TypeRefs before type inference.
Likewise, this moves macro expansion out of type inference, resulting in a single place where we do non-defmap macro expansion.
Finally, this PR removes a lot of information from ItemTree, making the DefMap a lot less likely to be recomputed and have it only depend on actual early name resolution related information (not 100% true, we still have ADT fields in there but thats a follow up removal).

Veykril · 2025-04-04T11:40:05Z

CI is green for the first time 🎉 I did should_panic a couple of low bearing diagnostics tests though, figured I'll look into those more generally as a followup as the diagnostics part in hir is a bit of a mess to follow (that or maybe Chayim is already planning on cleaning that up with their related work there).

Gonna leave the history as is until reviews, then I'll squash. Might cleanup a couple things after, same for documentation, I'll do that as a follow up.

crates/hir-def/src/hir/type_ref.rs

crates/hir-def/src/lib.rs

crates/hir-def/src/nameres/tests/incremental.rs

davidbarsky · 2025-04-05T14:05:57Z

crates/hir-def/src/db.rs

+    #[salsa::tracked]
+    fn enum_variants(&self, id: EnumId) -> Arc<EnumVariants> {
+        self.enum_variants_with_diagnostics(id).0
+    }


I assume you changed this in the macro, but what does this do?

(Should we just define this this as a tracked method on EnumId instead?)

It's just shorthand syntax. Instead of having to create a a function and pointing to it wtih invoke_actual this allows writing that inline. Its basically just salsa::tracked but written as part of the trait here

Ah, gotcha. We can probably replace a lot of usages of invoke_actual with this, I think!

ChayimFriedman2

I reviewed the changes (although I didn't look at the newest changes deeply), looks good! Commented a few nits.

crates/hir-def/src/hir/type_ref.rs

ChayimFriedman2 · 2025-04-01T10:52:46Z

crates/hir-def/src/item_tree.rs

@@ -515,10 +340,8 @@ pub enum AttrOwner {
    TopLevel,

    Variant(FileItemTreeId<Variant>),
+    // while not relevant to early name resolution, fields can contain visibility


Why is this a reason to include them in the item tree? Can't we resolve visibilities from signatures?

I was asking myself that earlier when I stumbled upon this again. Pretty sure this can go as well. I assume I put that comment there due to a misunderstanding. Will turn that into a FIXME

crates/hir-def/src/item_tree/pretty.rs

ChayimFriedman2 · 2025-04-03T16:53:21Z

crates/hir-def/src/db.rs

-    #[salsa::invoke_actual(TraitAliasData::trait_alias_query)]
-    fn trait_alias_data(&self, e: TraitAliasId) -> Arc<TraitAliasData>;
+    #[salsa::tracked]
+    fn variant_fields(&self, id: VariantId) -> Arc<VariantFields> {


I'm not saying you should do it in this PR, but maybe it is time to consider making those return references?

Absolutely. There is a lot of Arcs we should be able to drop now in one swoop. I kept using Arcs everywhere for now as to not do even more work in this PR.

crates/hir-def/src/lib.rs

crates/hir-ty/src/display.rs

crates/hir-ty/src/lib.rs

crates/hir-ty/src/method_resolution.rs

crates/hir/src/lib.rs

davidbarsky · 2025-04-08T16:36:35Z

crates/hir-def/src/item_tree/tests.rs

@@ -276,153 +266,6 @@ m!();
    );
 }

-#[test]
-fn mod_paths() {


i'm not sure i see these tests moved elsewhere—do we just not need these anymore, or...?

They test things that are removed from the item tree.

ChayimFriedman2

I reviewed the newer commits. Left a few nits.

crates/hir-def/src/nameres/collector.rs

crates/hir-def/src/signatures.rs

Veykril · 2025-04-09T08:14:21Z

Alright, will squash once CI goes green and then merge

This refactors how we deal with items in hir-def lowering. - It now lowers all of them through an "ExpressionStore" (kind of a misnomer as this point) as their so called *Signatures. - We now uniformly lower type AST into TypeRefs before type inference. - Likewise, this moves macro expansion out of type inference, resulting in a single place where we do non-defmap macro expansion. - Finally, this PR removes a lot of information from ItemTree, making the DefMap a lot less likely to be recomputed and have it only depend on actual early name resolution related information (not 100% true, we still have ADT fields in there but thats a follow up removal).

Veykril · 2025-04-09T09:27:18Z

This did reduce memory usage on self by ~160mb, but it also regressed with unknown types in inference. I imagine this PR regressed some support for const arguments in types.

That is, all queries that have a `with_diagnostic` variant. Them being tracked was (maybe) needed before rust-lang#19462, because back then diagnostics could refer `AstId`s (for macro types), but now they are no longer needed.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2025

Veykril force-pushed the push-ypvprvvwkyll branch 11 times, most recently from 4d8191c to cd3b8db Compare April 3, 2025 08:04

Veykril force-pushed the push-ypvprvvwkyll branch 3 times, most recently from 9fa36ff to 125d8c6 Compare April 4, 2025 11:23

Veykril marked this pull request as ready for review April 4, 2025 11:40

Veykril commented Apr 4, 2025

View reviewed changes

crates/hir-def/src/hir/type_ref.rs Show resolved Hide resolved

crates/hir-def/src/lib.rs Show resolved Hide resolved

crates/hir-def/src/nameres/tests/incremental.rs Show resolved Hide resolved

Veykril force-pushed the push-ypvprvvwkyll branch 3 times, most recently from e09d309 to f29fc03 Compare April 5, 2025 08:34

davidbarsky reviewed Apr 5, 2025

View reviewed changes

ChayimFriedman2 approved these changes Apr 5, 2025

View reviewed changes

Veykril force-pushed the push-ypvprvvwkyll branch 2 times, most recently from 9777fbb to b2ed56f Compare April 6, 2025 09:10

davidbarsky reviewed Apr 8, 2025

View reviewed changes

ChayimFriedman2 approved these changes Apr 9, 2025

View reviewed changes

crates/hir-def/src/nameres/collector.rs Outdated Show resolved Hide resolved

crates/hir-def/src/nameres/collector.rs Outdated Show resolved Hide resolved

crates/hir-def/src/signatures.rs Outdated Show resolved Hide resolved

Veykril force-pushed the push-ypvprvvwkyll branch from 3990cc6 to e3633c3 Compare April 9, 2025 08:12

Veykril force-pushed the push-ypvprvvwkyll branch from e3633c3 to 3ee9346 Compare April 9, 2025 08:25

Veykril force-pushed the push-ypvprvvwkyll branch from 3ee9346 to 95dce5a Compare April 9, 2025 08:42

Veykril force-pushed the push-ypvprvvwkyll branch from 95dce5a to 1fd9520 Compare April 9, 2025 08:43

Veykril enabled auto-merge April 9, 2025 08:43

Veykril added this pull request to the merge queue Apr 9, 2025

Merged via the queue into rust-lang:master with commit dc363f7 Apr 9, 2025
14 checks passed

Veykril deleted the push-ypvprvvwkyll branch April 9, 2025 09:06

This was referenced Apr 9, 2025

Move ADT fields out of the item tree #19549

Open

Lower const parameters #7434

Open

davidbarsky mentioned this pull request Apr 10, 2025

internal: add prime-caches subcommand #19565

Merged

ChayimFriedman2 mentioned this pull request Apr 18, 2025

internal: Make hir-ty non-diagnostic queries transparent #19616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Lower type-refs before type inference #19462

refactor: Lower type-refs before type inference #19462

Veykril commented Mar 27, 2025 •

edited

Loading

Veykril commented Apr 4, 2025 •

edited

Loading

davidbarsky Apr 5, 2025

Veykril Apr 5, 2025

davidbarsky Apr 5, 2025

ChayimFriedman2 left a comment

ChayimFriedman2 Apr 1, 2025

Veykril Apr 5, 2025

ChayimFriedman2 Apr 3, 2025

Veykril Apr 5, 2025

davidbarsky Apr 8, 2025

ChayimFriedman2 Apr 8, 2025

ChayimFriedman2 left a comment

Veykril commented Apr 9, 2025

Veykril commented Apr 9, 2025

refactor: Lower type-refs before type inference #19462

refactor: Lower type-refs before type inference #19462

Conversation

Veykril commented Mar 27, 2025 • edited Loading

Veykril commented Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChayimFriedman2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChayimFriedman2 left a comment

Choose a reason for hiding this comment

Veykril commented Apr 9, 2025

Veykril commented Apr 9, 2025

Veykril commented Mar 27, 2025 •

edited

Loading

Veykril commented Apr 4, 2025 •

edited

Loading