Lower AST node id only once by adwinwhite · Pull Request #130259 · rust-lang/rust

adwinwhite · 2024-09-12T04:36:02Z

I basically followed the given instructions except the inline part.

lower_jump_destination can't reuse local existing HirId due to unknown name resolution result so I created an additional mapping for labels.

r? @cjgillot

rustbot · 2024-09-12T04:36:10Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

adwinwhite · 2024-09-12T04:37:34Z

r? @cjgillot

rustbot · 2024-09-12T04:37:36Z

Could not assign reviewer from: cjgillot.
User(s) cjgillot are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

adwinwhite · 2024-09-12T13:43:34Z

The test's output changed because I rearranged the order of some lowering to avoid duplicate lowering.

cjgillot · 2024-09-14T01:31:02Z

compiler/rustc_ast_lowering/src/block.rs

+        self.arena.alloc(self.lower_block_noalloc(hir_id, b, targeted_by_break))
+    }
+
+    pub(super) fn lower_block_with_hir_id(


Should this be inlined into its single call site?

Good idea. I didn't notice there's only one call site.

cjgillot · 2024-09-14T01:32:19Z

compiler/rustc_ast_lowering/src/expr.rs

        let if_expr = self.expr(span, if_kind);
        let block = self.block_expr(self.arena.alloc(if_expr));
        let span = self.lower_span(span.with_hi(cond.span.hi()));
-        let opt_label = self.lower_label(opt_label);


It's the limitation that the recording of label hir id is coupled with lower_label method.
So I have to lower label outside otherwise I need to introduce node_id and hir_id parameters to lower_expr_while_in_loop_scope.

cjgillot · 2024-09-14T01:34:06Z

compiler/rustc_ast_lowering/src/item.rs

        let vis_span = self.lower_span(i.vis.span);
-        let hir_id = self.lower_node_id(i.id);
+        let hir_id =
+            hir::HirId { owner: self.current_hir_id_owner, local_id: hir::ItemLocalId::ZERO };


This can use HirId::make_owner.

cjgillot · 2024-09-14T01:34:24Z

compiler/rustc_ast_lowering/src/lib.rs

+    /// NodeIds of labelled nodes that are lowered inside the current HIR owner.
+    labelled_node_id_to_local_id: NodeMap<hir::ItemLocalId>,
+    /// NodeIds of identifier that are lowered inside the current HIR owner.
+    ident_to_local_id: NodeMap<hir::ItemLocalId>,


Should we have a single map for both cases?

cjgillot · 2024-09-14T01:36:02Z

compiler/rustc_ast_lowering/src/lib.rs

    #[instrument(level = "debug", skip(self), ret)]
    fn lower_node_id(&mut self, ast_node_id: NodeId) -> HirId {
        assert_ne!(ast_node_id, DUMMY_NODE_ID);



Should we keep a check, gated behind #[cfg(debug_assertions)], that we don't call this twice on the same NodeId?

Good idea. I didn't keep the check to avoid hashing cost. Thanks for reminding me of conditional compilation!

adwinwhite · 2024-10-08T02:02:39Z

@cjgillot May you have another look? There is no hurry of course.

compiler/rustc_ast_lowering/src/lib.rs

cjgillot · 2024-10-12T23:10:16Z

compiler/rustc_ast_lowering/src/pat.rs

+                // All identifiers resolves to this canonical identifier share its `HirId`.
+                let binding_id = if canonical_id == p.id {
+                    self.ident_and_label_to_local_id.insert(canonical_id, hir_id.local_id);
+                    hir_id
+                } else {
+                    hir::HirId {
+                        owner: self.current_hir_id_owner,
+                        local_id: self.ident_and_label_to_local_id[&canonical_id],
+                    }
+                };


Could you merge this with the previous match?

cjgillot · 2024-10-12T23:10:39Z

Sorry for the very slow review.

bors · 2024-10-24T21:05:21Z

☔ The latest upstream changes (presumably #131985) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot · 2024-10-26T14:23:48Z

r=me after rebase

adwinwhite · 2024-10-28T09:46:20Z

@bors r=cjgillot

bors · 2024-10-28T09:46:23Z

@adwinwhite: 🔑 Insufficient privileges: Not in reviewers

adwinwhite · 2024-10-28T09:57:31Z

@rustbot ready

cjgillot · 2024-10-28T14:11:29Z

@bors r+

bors · 2024-10-28T14:11:32Z

📌 Commit e3bf50e has been approved by cjgillot

It is now in the queue for this repository.

…jgillot Lower AST node id only once Fixes rust-lang#96346. I basically followed the given instructions except the inline part. `lower_jump_destination` can't reuse local existing `HirId` due to unknown name resolution result so I created an additional mapping for labels. r? `@cjgillot`

…jgillot Lower AST node id only once Fixes rust-lang#96346. I basically followed the given instructions except the inline part. `lower_jump_destination` can't reuse local existing `HirId` due to unknown name resolution result so I created an additional mapping for labels. r? ``@cjgillot``

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutS::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) r? `@ghost` `@rustbot` modify labels: rollup

…jgillot Lower AST node id only once Fixes rust-lang#96346. I basically followed the given instructions except the inline part. `lower_jump_destination` can't reuse local existing `HirId` due to unknown name resolution result so I created an additional mapping for labels. r? ````@cjgillot````

…jgillot Lower AST node id only once Fixes rust-lang#96346. I basically followed the given instructions except the inline part. `lower_jump_destination` can't reuse local existing `HirId` due to unknown name resolution result so I created an additional mapping for labels. r? `````@cjgillot`````

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutData::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) - rust-lang#132295 (fixed wast version was released, remove randomization exemption) r? `@ghost` `@rustbot` modify labels: rollup

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#130259 (Lower AST node id only once) - rust-lang#131441 (Add a new trait `proc_macro::ToTokens`) - rust-lang#132247 (stable_mir: Directly use types from rustc_abi) - rust-lang#132249 (compiler: Add rustc_abi dependence to the compiler) - rust-lang#132255 (Add `LayoutData::is_uninhabited` and use it) - rust-lang#132258 ([rustdoc] Unify variant struct fields margins with struct fields) - rust-lang#132260 (cg_llvm: Use a type-safe helper to cast `&str` and `&[u8]` to `*const c_char`) - rust-lang#132261 (refactor: cleaner check to return None) - rust-lang#132271 (Updating Fuchsia platform-support documentation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130259 - adwinwhite:lower-node-id-once, r=cjgillot Lower AST node id only once Fixes rust-lang#96346. I basically followed the given instructions except the inline part. `lower_jump_destination` can't reuse local existing `HirId` due to unknown name resolution result so I created an additional mapping for labels. r? ```@cjgillot```

Kobzol · 2024-10-30T19:20:47Z

Just FYI, this PR had a nice effect on the performance of the compiler :) #132277 (comment) Great work!

rustbot assigned TaKO8Ki Sep 12, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 12, 2024

rustbot assigned cjgillot and unassigned TaKO8Ki Sep 12, 2024

This comment has been minimized.

Sign in to view

adwinwhite force-pushed the lower-node-id-once branch from f7f31d7 to eaf91bb Compare September 12, 2024 06:32

adwinwhite marked this pull request as draft September 12, 2024 07:41

adwinwhite force-pushed the lower-node-id-once branch from eaf91bb to 7760acf Compare September 12, 2024 10:47

This comment has been minimized.

Sign in to view

adwinwhite force-pushed the lower-node-id-once branch from 7760acf to 2b28424 Compare September 12, 2024 12:20

adwinwhite marked this pull request as ready for review September 12, 2024 13:38

cjgillot reviewed Sep 14, 2024

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2024

adwinwhite requested a review from cjgillot September 15, 2024 04:56

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2024

cjgillot reviewed Oct 12, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

adwinwhite force-pushed the lower-node-id-once branch from 8ac9b58 to 4db575a Compare October 14, 2024 05:48

adwinwhite requested a review from cjgillot October 14, 2024 07:59

cjgillot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 27, 2024

adwinwhite added 3 commits October 28, 2024 14:12

Lower AST node id only once

cb08e08

Add duplicate lowering check

1a39247

more consistent debug_assertions

e3bf50e

adwinwhite force-pushed the lower-node-id-once branch from 4db575a to e3bf50e Compare October 28, 2024 08:11

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2024

GuillaumeGomez mentioned this pull request Oct 28, 2024

Rollup of 5 pull requests #132269

Closed

This was referenced Oct 28, 2024

Rollup of 9 pull requests #132273

Closed

Rollup of 9 pull requests #132277

Merged

matthiaskrgr mentioned this pull request Oct 29, 2024

Rollup of 10 pull requests #132307

Closed

workingjubilee mentioned this pull request Oct 29, 2024

Rollup of 12 pull requests #132308

Closed

bors merged commit a24b377 into rust-lang:master Oct 29, 2024

rustbot added this to the 1.84.0 milestone Oct 29, 2024

Uh oh!

Conversation

adwinwhite commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 12, 2024

Uh oh!

adwinwhite commented Sep 12, 2024

Uh oh!

rustbot commented Sep 12, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

adwinwhite commented Sep 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adwinwhite commented Oct 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Oct 12, 2024

Uh oh!

This comment has been minimized.

bors commented Oct 24, 2024

Uh oh!

cjgillot commented Oct 26, 2024

Uh oh!

adwinwhite commented Oct 28, 2024

Uh oh!

bors commented Oct 28, 2024

Uh oh!

adwinwhite commented Oct 28, 2024

Uh oh!

cjgillot commented Oct 28, 2024

Uh oh!

bors commented Oct 28, 2024

Uh oh!

Kobzol commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adwinwhite commented Sep 12, 2024 •

edited

Loading