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

Remove all uses of NodeId in ResolverOutputs #72402

Merged
merged 9 commits into from
May 24, 2020

Conversation

marmeladema
Copy link
Contributor

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

@ecstatic-morse ecstatic-morse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ljedrz

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

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
Comment on lines +123 to +128
pub extern_crate_map: FxHashMap<DefId, CrateNum>,
pub trait_map: FxHashMap<hir::HirId, Vec<hir::TraitCandidate<hir::HirId>>>,
pub maybe_unused_trait_imports: FxHashSet<LocalDefId>,
pub maybe_unused_extern_crates: Vec<(DefId, Span)>,
pub export_map: ExportMap<hir::HirId>,
pub glob_map: FxHashMap<LocalDefId, FxHashSet<Symbol>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@petrochenkov petrochenkov self-assigned this May 21, 2020
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
@marmeladema
Copy link
Contributor Author

Rebased and fixed comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 21, 2020

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

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2020
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 21, 2020

📌 Commit f31e076 has been approved by ecstatic-morse

@petrochenkov petrochenkov removed their assignment May 21, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request May 23, 2020
…, r=ecstatic-morse

Remove all uses of `NodeId` in `ResolverOutputs`

cc rust-lang#50928

r? @ecstatic-morse
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 23, 2020
…, r=ecstatic-morse

Remove all uses of `NodeId` in `ResolverOutputs`

cc rust-lang#50928

r? @ecstatic-morse
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 23, 2020
…, r=ecstatic-morse

Remove all uses of `NodeId` in `ResolverOutputs`

cc rust-lang#50928

r? @ecstatic-morse
bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2020
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
@bors bors merged commit 95c4583 into rust-lang:master May 24, 2020
@marmeladema marmeladema deleted the resolver-outputs-def-id branch May 24, 2020 22:13
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 29, 2020
…, 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
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.

7 participants