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.

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

Pre-compute LocalDefId <-> HirId mappings and remove NodeId <-> HirId conversion APIs #73291

Merged
merged 5 commits into from
Jun 21, 2020

Conversation

marmeladema
Copy link
Contributor

cc #50928

I don't know who is exactly the best person to review this.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2020
@marmeladema
Copy link
Contributor Author

Next step is probably to move

    node_id_to_def_id: FxHashMap<ast::NodeId, LocalDefId>,
    def_id_to_node_id: IndexVec<LocalDefId, ast::NodeId>,

mappings from hir::Definitions to Resolver such that any references to NodeId are gone with it.

@petrochenkov
Copy link
Contributor

What is the end goal here, removing any mentions of NodeId from struct Definitions?

Definitions is a pretty heterogeneous structure.
Perhaps it can be split into two structures instead, one for fields used before lowering to HIR, and one for fields used after lowering to HIR?

struct Definitions {
    // Both before and after (or filled before, used after)
    table: DefPathTable,
    parent_modules_of_macro_defs: FxHashMap<ExpnId, DefId>,
    expansions_that_defined: FxHashMap<LocalDefId, ExpnId>,

    // Before
    def_id_to_span: IndexVec<LocalDefId, Span>,
    node_id_to_def_id: FxHashMap<ast::NodeId, LocalDefId>,
    def_id_to_node_id: IndexVec<LocalDefId, ast::NodeId>,
    next_disambiguator: FxHashMap<(LocalDefId, DefPathData), u32>,
    invocation_parents: FxHashMap<ExpnId, LocalDefId>,
    placeholder_field_indices: FxHashMap<ast::NodeId, usize>,

    // ???
    node_id_to_hir_id: IndexVec<ast::NodeId, Option<hir::HirId>>,
    hir_id_to_node_id: FxHashMap<hir::HirId, ast::NodeId>,
}

Having the "before" fields in the ResolverOutputs version of Definitions doesn't make any sense, but they are currently placed there.

@petrochenkov
Copy link
Contributor

The "after" fields could actually go into ResolverOutputs directly, without any additional structure.
So Definitions will exist solely during early compilation as a part of Resolver.

@petrochenkov petrochenkov 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 Jun 12, 2020
@marmeladema
Copy link
Contributor Author

My idea was to move everything NodeId related (ie: the "before" fields) into the Resolver such that they are dropped with it but I would keep the rest of the fields in Definitions.

So to summarize, using this PR as base of discussion, Definitions would only contains:

  • def_id_to_hir_id
  • hir_id_to_def_id
  • table
  • parent_modules_of_macro_defs
  • expansions_that_defined
    which is the list of fields that should be available post HIR lowering.

TL;DR: "before" fields in Resolver, "after" fields kept in Definitions.

@petrochenkov petrochenkov 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 Jun 13, 2020
src/librustc_hir/hir.rs Outdated Show resolved Hide resolved
src/librustc_middle/hir/map/collector.rs Outdated Show resolved Hide resolved
src/librustc_hir/definitions.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

@marmeladema

TL;DR: "before" fields in Resolver, "after" fields kept in Definitions.

Sounds like a plan!

@petrochenkov petrochenkov 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 Jun 13, 2020
@marmeladema marmeladema force-pushed the hir-id-ification-fix branch 2 times, most recently from bf059a3 to 864872b Compare June 14, 2020 20:39
@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 14, 2020

ID memo:

  • LocalDefId - the best variant, if you can use it for something do it. Cannot address entities in other crates and fine-grained entities like individual expressions in the current crate. Can address any items.
  • DefId - the second choice. Can address entities in other crates, but cannot address fine-grained entities like individual expressions.
  • HirId - Can address fine-grained entities like individual expressions in the current crate. Fine-grained entities from other crates can never be addressed in rustc.
  • NodeId - The last choice. Can address fine-grained entities like individual expressions when HirIds are not yet created.

cc @mark-i-m this can probably be added to the dev guide

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 19, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 19, 2020
@bors
Copy link
Contributor

bors commented Jun 20, 2020

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

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 20, 2020
@petrochenkov
Copy link
Contributor

#73357 has landed.

@marmeladema marmeladema force-pushed the hir-id-ification-fix branch from 864872b to b4fc8bd Compare June 20, 2020 10:05
@marmeladema
Copy link
Contributor Author

@petrochenkov rebased and fixed the conflicts 👍

@marmeladema marmeladema requested a review from petrochenkov June 20, 2020 10:35
@marmeladema marmeladema force-pushed the hir-id-ification-fix branch from b4fc8bd to 58d8374 Compare June 20, 2020 10:41
@rust-highfive

This comment has been minimized.

@marmeladema marmeladema force-pushed the hir-id-ification-fix branch from 58d8374 to 13104ef Compare June 20, 2020 12:02
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2020

📌 Commit 13104ef has been approved by petrochenkov

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 20, 2020
…=petrochenkov

Pre-compute `LocalDefId` <-> `HirId` mappings and remove `NodeId` <-> `HirId` conversion APIs

cc rust-lang#50928

I don't know who is exactly the best person to review this.

r? @petrochenkov
This was referenced Jun 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#72456 (Try to suggest dereferences on trait selection failed)
 - rust-lang#72788 (Projection bound validation)
 - rust-lang#72790 (core/time: Add Duration methods for zero)
 - rust-lang#73227 (Allow multiple `asm!` options groups and report an error on duplicate options)
 - rust-lang#73287 (lint: normalize projections using opaque types)
 - rust-lang#73291 (Pre-compute `LocalDefId` <-> `HirId` mappings and remove `NodeId` <-> `HirId` conversion APIs)
 - rust-lang#73378 (Remove use of specialization from librustc_arena)
 - rust-lang#73411 (Update bootstrap to rustc 1.45.0-beta.2 (1dc0f6d 2020-06-15))
 - rust-lang#73443 (ci: allow gating GHA on everything but macOS)

Failed merges:

r? @ghost
@bors bors merged commit 0a8fd43 into rust-lang:master Jun 21, 2020
@marmeladema marmeladema deleted the hir-id-ification-fix branch June 21, 2020 17:15
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 23, 2020
… r=petrochenkov

Move remaining `NodeId` APIs from `Definitions` to `Resolver`

Implements rust-lang#73291 (comment)

TL;DR: it moves all fields that are only needed during name resolution passes into the `Resolver` and keep the rest in `Definitions`. This effectively enforces that all references to `NodeId`s are gone once HIR lowering is completed.

After this, the only remaining work for rust-lang#50928 should be to adjust the dev guide.

r? @petrochenkov
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 23, 2020
… r=petrochenkov

Move remaining `NodeId` APIs from `Definitions` to `Resolver`

Implements rust-lang#73291 (comment)

TL;DR: it moves all fields that are only needed during name resolution passes into the `Resolver` and keep the rest in `Definitions`. This effectively enforces that all references to `NodeId`s are gone once HIR lowering is completed.

After this, the only remaining work for rust-lang#50928 should be to adjust the dev guide.

r? @petrochenkov
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants