Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Forbid lowering the same NodeId multiple times #96346

Closed
cjgillot opened this issue Apr 23, 2022 · 10 comments · Fixed by #130259
Closed

Forbid lowering the same NodeId multiple times #96346

cjgillot opened this issue Apr 23, 2022 · 10 comments · Fixed by #130259
Assignees
@adwinwhite
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

cjgillot commented Apr 23, 2022 •
edited by rustbot
Loading

During AST->HIR lowering, the method lower_node_id is tasked to transform NodeIds that identify AST nodes into HirIds that identify HIR nodes. This method maintains a node_id_to_local_id: FxHashMap<NodeId, hir::ItemLocalId> to remember this mapping.

However, this mapping is not entirely useful, and mostly exists (1) to make the developer's life easier, (2) to lower resolutions to local bindings Res::Local in lower_res. The mapping node_id_to_local_id should be removed, and multiple calls to local_node_id with the same NodeId should be forbidden. For usage (2), Res::Local only appears for ident patterns (PatKind::Ident) which are lowered by lower_pat_ident. Hence, lower_res should use a dedicated hash-map filled by lower_pat_ident.

Furthermore, next_id calls lower_node_id with a node_id: NodeId which is entirely local to that function, and will never be known to any other code. Manipulations of node_id_to_local_id there are entirely useless.

Instructions:

  • inline lower_node_id into next_id and skip manipulations of node_id_to_local_id;
  • replace the Entry::Occupied branch in lower_node_id by a panic!, and fix all the ICEs;
  • create another mapping node_id_to_local_id_for_res (name to bikeshed) which is only filled by lower_pat_ident and read by lower_res; this mapping should be saved by with_hir_id_owner like node_id_to_local_id is;
  • remove node_id_to_local_id, or eventually keep it only to debug-assert that we don't call lower_node_id twice on the same NodeId.
@cjgillot cjgillot added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Apr 23, 2022
@kckeiks
Copy link
Contributor

kckeiks commented Apr 23, 2022

@rustbot claim

@raiyansayeed
Copy link
Contributor

raiyansayeed commented Jun 7, 2022 •
edited
Loading

Hey @kckeiks , are you still working on this task by chance?

@kckeiks
Copy link
Contributor

kckeiks commented Jun 7, 2022 •
edited
Loading

@raiyansayeed I was but I have a couple of other issues that I'm working on so you can take this one if you want.

@spastorino
Copy link
Member

@raiyansayeed feel free to ping me too on Zulip if you need help or something.

@raiyansayeed
Copy link
Contributor

@rustbot claim

@rustbot rustbot assigned raiyansayeed and unassigned kckeiks Jun 9, 2022
@trevyn
Copy link
Contributor

trevyn commented Sep 6, 2022

@raiyansayeed are you still working on this?

@Noratrieb
Copy link
Member

No activity, feel free to claim it again. @rustbot release-assignment

@azadnn
Copy link

azadnn commented Jan 26, 2023

@rustbot claim

@azadnn azadnn removed their assignment Feb 1, 2023
@azadnn
Copy link

azadnn commented Feb 1, 2023

https://rust-lang.zulipchat.com/#narrow/stream/315146-t-compiler.2Fetc.2Fincremental-hir/topic/reduce.2Feliminate.20duplicate.20invocation.20of.20.60lower_node_id.60/near/324846414

@Enselic Enselic added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 22, 2023
@adwinwhite
Copy link
Contributor

@rustbot claim

@adwinwhite adwinwhite mentioned this issue Sep 12, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 28, 2024
@GuillaumeGomez
Rollup merge of rust-lang#130259 - adwinwhite:lower-node-id-once, r=c…
93dd589 
…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`
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 28, 2024
@workingjubilee
Rollup merge of rust-lang#130259 - adwinwhite:lower-node-id-once, r=c…
55edadc 
…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``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 29, 2024
@matthiaskrgr
Rollup merge of rust-lang#130259 - adwinwhite:lower-node-id-once, r=c…
8edbd04 
…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````
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 29, 2024
@workingjubilee
Rollup merge of rust-lang#130259 - adwinwhite:lower-node-id-once, r=c…
8223991 
…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`````
@bors bors closed this as completed in a24b377 Oct 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 29, 2024
@rust-timer
Unrolled build for rust-lang#130259
815a7c7 
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```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adwinwhite adwinwhite

Labels
E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Lower AST node id only once adwinwhite/rust
9 participants
@spastorino @Enselic @trevyn @cjgillot @kckeiks @raiyansayeed @Noratrieb @adwinwhite @azadnn