Remove all uses of `NodeId` in `ResolverOutputs` by marmeladema · Pull Request #72402 · rust-lang/rust

marmeladema · 2020-05-20T23:12:02Z

cc #50928

r? @ecstatic-morse

ecstatic-morse

cc @ljedrz

Left a nit. I'll approve once it is fixed and CI is green. Thanks @marmeladema!

src/librustc_resolve/lib.rs

ecstatic-morse · 2020-05-20T23:42:06Z

src/librustc_middle/ty/mod.rs

It does seem like all of these could plausibly be DefId. Did you find something during your experiments that wasn't a definition? If not, we should open a cleanup issue once this gets merged.

I've only tried for trait_map but yes, for this field, there is no def id, only hir id

@petrochenkov I see you've assigned yourself. Any recommendations here? Is there a reason that trait_map and export_map cannot use DefIds?

Also because the global context uses HirId, using DefId here would mean another map clone/conversion in librustc_middle.

The zoo of DefIds, LocalDefIds and HirIds here is a bit surprising (all of these could be LocalDefIds (or HirIds)), but it looks like the choices are made to minimize conversions at use sites.

I guess the right thing to do here would be changing DefIds into LocalDefIds in GlobalCtxt which is the end destination of these tables.

I can definitely take a look at that in a follow up PR!

src/librustc_hir/definitions.rs

src/librustc_resolve/lib.rs

…tead of `NodeId`

…of `NodeId`

…r `Copy` types

marmeladema · 2020-05-21T16:37:01Z

Rebased and fixed comments

ecstatic-morse · 2020-05-21T16:48:11Z

Thanks @marmeladema! It's possible that someone with more knowledge of AST -> HIR lowering (@petrochenkov?) could find an elegant solution to #72402 (comment). However, because this may unlock #50928 (comment), I'm happy to approve it in its present state.

@bors r+

bors · 2020-05-21T16:49:27Z

📌 Commit f31e076 has been approved by ecstatic-morse

@ecstatic-morse

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

@ecstatic-morse

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

@ecstatic-morse

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#72402 (Remove all uses of `NodeId` in `ResolverOutputs`) - rust-lang#72527 (bootstrap: propagate test-args to miri and clippy test suites) - rust-lang#72530 (Clean up E0602 explanation) - rust-lang#72532 (Use `dyn` trait syntax in more comments and docs) - rust-lang#72535 (Use sort_unstable_by in its own docs) Failed merges: r? @ghost

@ecstatic-morse

…, r=petrochenkov Cleanup `Resolver::<clone|into>_outputs` methods Follow-up cleanup work of rust-lang#72402 First commit has been split out from rust-lang#72552 r? @ecstatic-morse

rust-highfive assigned ecstatic-morse May 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2020

ecstatic-morse suggested changes May 20, 2020

View reviewed changes

petrochenkov self-assigned this May 21, 2020

marmeladema requested a review from ecstatic-morse May 21, 2020 07:29

ljedrz reviewed May 21, 2020

View reviewed changes

src/librustc_hir/definitions.rs Outdated Show resolved Hide resolved

ljedrz reviewed May 21, 2020

View reviewed changes

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved

marmeladema added 9 commits May 21, 2020 17:07

Use HirId as key for ResolverOutputs::trait_map instead of NodeId

5728c53

Use HirId in value of ResolverOutputs::trait_map instead of NodeId

3c5dba7

Use HirId in ResolverOutputs::export_map instead of NodeId

52359f7

Use LocalDefId in ResolverOutputs::maybe_unused_trait_imports ins…

13c86f2

…tead of `NodeId`

Use DefId in ResolverOutputs::maybe_unused_extern_crates instead …

25f575b

…of `NodeId`

Use DefId in ResolverOutputs::glob_map instead of NodeId

21f65ae

Use DefId in ResolverOutputs::extern_crate_map instead of NodeId

8ff6ffd

Use collect() instead of manually inserting elements into maps

4c4cb46

Replace unecessary calls to .clone() by argument binding pattern fo…

f31e076

…r `Copy` types

marmeladema force-pushed the resolver-outputs-def-id branch from bc1f1f1 to f31e076 Compare May 21, 2020 16:36

This comment has been minimized.

Sign in to view

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 May 21, 2020

This comment has been minimized.

Sign in to view

petrochenkov removed their assignment May 21, 2020

RalfJung added a commit to RalfJung/rust that referenced this pull request May 23, 2020

Rollup merge of rust-lang#72402 - marmeladema:resolver-outputs-def-id…

88e2815

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

RalfJung mentioned this pull request May 23, 2020

Rollup of 6 pull requests #72490

Closed

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 23, 2020

Rollup merge of rust-lang#72402 - marmeladema:resolver-outputs-def-id…

da05f6d

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

Dylan-DPC-zz mentioned this pull request May 23, 2020

Rollup of 6 pull requests #72500

Closed

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 23, 2020

Rollup merge of rust-lang#72402 - marmeladema:resolver-outputs-def-id…

fa1f189

…, r=ecstatic-morse Remove all uses of `NodeId` in `ResolverOutputs` cc rust-lang#50928 r? @ecstatic-morse

Dylan-DPC-zz mentioned this pull request May 23, 2020

Rollup of 6 pull requests #72501

Closed

marmeladema mentioned this pull request May 23, 2020

HirId-ification initiative #50928

Closed

RalfJung mentioned this pull request May 24, 2020

Rollup of 5 pull requests #72539

Merged

bors merged commit 95c4583 into rust-lang:master May 24, 2020

marmeladema deleted the resolver-outputs-def-id branch May 24, 2020 22:13

marmeladema mentioned this pull request May 25, 2020

Introduce LocalDefId to HirId lookup table #72552

Closed

SimonSapin mentioned this pull request May 26, 2020

Regression: spurious "error: expected ," in serde::Serialize on macro_rules-variable path enum discriminant #72608

Closed

marmeladema mentioned this pull request May 26, 2020

Cleanup Resolver::<clone|into>_outputs methods #72636

Merged

Uh oh!

Conversation

marmeladema commented May 20, 2020

Uh oh!

ecstatic-morse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ecstatic-morse May 20, 2020

Choose a reason for hiding this comment

Uh oh!

marmeladema May 20, 2020

Choose a reason for hiding this comment

Uh oh!

ecstatic-morse May 21, 2020

Choose a reason for hiding this comment

Uh oh!

marmeladema May 21, 2020

Choose a reason for hiding this comment

Uh oh!

petrochenkov May 21, 2020

Choose a reason for hiding this comment

Uh oh!

marmeladema May 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marmeladema commented May 21, 2020

Uh oh!

ecstatic-morse commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented May 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ecstatic-morse commented May 21, 2020 •

edited

Loading