HirIdification: kill off NodeId stragglers by ljedrz · Pull Request #59068 · rust-lang/rust

ljedrz · 2019-03-10T12:58:35Z

The final stages of HirIdification (#57578).

This PR, along with #59042, should finalize the HirIdification process (at least the more straightforward bits).

replace NodeId with HirId in trait_impls
remove all NodeIds from borrowck
remove all NodeIds from typeck
remove all NodeIds from mir
remove trait_auto_impl (unused)

I would be cool to also remove NodeId from hir::def::Def, middle::privacy::AccessLevel and hir::ItemId, but I don't know if this is feasible.

I'll be happy to do more if I've missed anything.

rust-highfive · 2019-03-10T12:58:38Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

ljedrz · 2019-03-10T12:58:54Z

r? @Zoxc

Zoxc · 2019-03-10T22:31:56Z

@bors r+

bors · 2019-03-10T22:31:57Z

📌 Commit 584d61a has been approved by Zoxc

Zoxc · 2019-03-10T22:32:18Z

I would be cool to also remove NodeId from hir::def::Def, middle::privacy::AccessLevel and hir::ItemId, but I don't know if this is feasible.

Where do you get stuck on these?

ljedrz · 2019-03-11T08:17:29Z

With Def::Local and Def::Upvar (first component) everything is fine until rustc_resolve where they can (as far as I can tell) only be used with a NodeId. I haven't studied all the related code that much yet, but it looks as if they might be candidates for lowering; they are used in tandem with hir::Freevar which doesn't contain a HirId.

privacy::AccessLevel - issues with save_analysis.

ItemId - HashStable debug assertion for hir::Mod fails (probably error with id lowering).

I can give them another shot and send a WIP PR for further investigation.

Zoxc · 2019-03-11T08:24:48Z

@ljedrz For Def, I'd make it generic over the id used and have a type Def = def::Def<HirId> type alias for it.

Feel free to open PRs for the other ones.

ljedrz · 2019-03-12T12:25:42Z

I've tried the generic Def approach, but it quickly becomes quite messy (especially before the stable hash derive macro goes live); is an enum containing either NodeId or HirId out of the question (though I haven't attempted that yet - it might get messy too)?

Zoxc · 2019-03-12T12:32:19Z

I've tried the generic Def approach, but it quickly becomes quite messy

What do you mean by messy?

(especially before the stable hash derive macro goes live)

That will land soon at least.

ljedrz · 2019-03-12T12:48:41Z

What do you mean by messy?

There are other objects containing Def and they also need to become generic and often additionally have some of the bounds applicable to NodeId and HirId like Debug and Display.

eddyb · 2019-03-13T12:29:57Z

There are other objects containing Def and they also need to become generic

How so? I suspect that you can partition most of them into "used during/produced by rustc_resolve" (Def<NodeId>) and "contained in the HIR/elsewhere" (Def<HirId>).

As an aside, I should reiterate that the Def type should be named Res or something (since it's the result of name resolution, which is not exactly "definition" in same sense as DefId)

Also, things could be slightly nicer by having the HIR represent uses of local variables (including upvars) as a separate ExprKind variant (instead of Path), but that might not be a trivial refactoring right now.

ljedrz · 2019-03-13T12:33:40Z

Hmm, I guess I can give it another shot as soon as #56864 lands 👍.

mark-i-m · 2019-03-13T21:22:31Z

#56864 has already landed?

EDIT: ah, it landed 3 hours after your post 😛

…r=Zoxc HirIdification: kill off NodeId stragglers The final stages of HirIdification (rust-lang#57578). This PR, along with rust-lang#59042, should finalize the HirIdification process (at least the more straightforward bits). - replace `NodeId` with `HirId` in `trait_impls` - remove all `NodeId`s from `borrowck` - remove all `NodeId`s from `typeck` - remove all `NodeId`s from `mir` - remove `trait_auto_impl` (unused) I would be cool to also remove `NodeId` from `hir::def::Def`, `middle::privacy::AccessLevel` and `hir::ItemId`, but I don't know if this is feasible. I'll be happy to do more if I've missed anything.

bors · 2019-03-23T09:01:06Z

⌛ Testing commit 584d61a with merge c3182db4e8af03d6af9504e6a5389d1d4f4e861c...

bors · 2019-03-23T12:21:19Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: Zoxc
Pushing c3182db4e8af03d6af9504e6a5389d1d4f4e861c to master...

bors · 2019-03-23T12:21:22Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

Zoxc · 2019-03-23T16:33:42Z

@bors retry

bors · 2019-03-23T17:34:26Z

⌛ Testing commit 584d61a with merge 0633c55...

HirIdification: kill off NodeId stragglers The final stages of HirIdification (#57578). This PR, along with #59042, should finalize the HirIdification process (at least the more straightforward bits). - replace `NodeId` with `HirId` in `trait_impls` - remove all `NodeId`s from `borrowck` - remove all `NodeId`s from `typeck` - remove all `NodeId`s from `mir` - remove `trait_auto_impl` (unused) I would be cool to also remove `NodeId` from `hir::def::Def`, `middle::privacy::AccessLevel` and `hir::ItemId`, but I don't know if this is feasible. I'll be happy to do more if I've missed anything.

bors · 2019-03-23T20:52:35Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: Zoxc
Pushing 0633c55 to master...

Zoxc · 2019-04-02T19:58:50Z

@ljedrz Did you try dealing with Def or should I take a shot at it?

ljedrz · 2019-04-03T07:45:24Z

@Zoxc I've started, but I'm traveling this week, so I might not be able to resume until the next one; if you'd prefer this to be done more quickly you can use my branch that should have most of the HIR-and-above-side changes; what's left is splitting Def (or Res) into the HIR and resolver objects.

Zoxc · 2019-04-04T11:50:17Z

@ljedrz I made some progress in https://github.com/Zoxc/rust/tree/hiridify_def_id, but I run into issues lowering Defs with node ids that have yet to be lowered.

eddyb · 2019-04-04T12:59:43Z

@Zoxc Can't you allocate the NodeId -> HirId at the first use, and have the latter lowering of the actual node in question reuse the same HirId?

ljedrz added 5 commits March 10, 2019 13:07

hir: replace NodeId with HirId in trait_impls
7e1914f

HirIdification: remove all NodeIds from borrowck
401329e

HirIdification: remove all NodeIds from typeck
aa53741

HirIdification: remove all NodeIds from rustc_mir
9151eab

hir: remove trait_auto_impl
584d61a

rust-highfive assigned eddyb Mar 10, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2019

ljedrz changed the title ~~ill off NodeId stragglers~~HirIdification: kill off NodeId stragglersMar 10, 2019

rust-highfive assigned Zoxc and unassigned eddyb Mar 10, 2019

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 Mar 10, 2019

Zoxc mentioned this pull request Mar 14, 2019
HirIdification: rework Map #59042
Merged

Centril mentioned this pull request Mar 16, 2019
Rollup of 9 pull requests #59244
Closed

Mark-Simulacrum mentioned this pull request Mar 22, 2019
Rollup of 22 pull requests #59364
Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2019

bors merged commit 584d61a into rust-lang:masterMar 23, 2019

This was referenced Mar 23, 2019
When moving out of a for loop head, suggest borrowing it #59195
Merged
Rollup of 8 pull requests #59387
Closed

ljedrz deleted the kill_off_NodeId_stragglers branch March 24, 2019 05:24

Uh oh!

Uh oh!

Conversation

ljedrz commented Mar 10, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 10, 2019

Uh oh!

ljedrz commented Mar 10, 2019

Uh oh!

Zoxc commented Mar 10, 2019

Uh oh!

bors commented Mar 10, 2019

Uh oh!

Zoxc commented Mar 10, 2019

Uh oh!

ljedrz commented Mar 11, 2019

Uh oh!

Zoxc commented Mar 11, 2019

Uh oh!

ljedrz commented Mar 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zoxc commented Mar 12, 2019

Uh oh!

ljedrz commented Mar 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Mar 13, 2019

Uh oh!

ljedrz commented Mar 13, 2019

Uh oh!

mark-i-m commented Mar 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Mar 23, 2019

Uh oh!

bors commented Mar 23, 2019

Uh oh!

bors commented Mar 23, 2019

Uh oh!

Zoxc commented Mar 23, 2019

Uh oh!

bors commented Mar 23, 2019

Uh oh!

bors commented Mar 23, 2019

Uh oh!

Zoxc commented Apr 2, 2019

Uh oh!

ljedrz commented Apr 3, 2019

Uh oh!

Zoxc commented Apr 4, 2019

Uh oh!

eddyb commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ljedrz commented Mar 10, 2019•
edited
Loading

ljedrz commented Mar 12, 2019•
edited
Loading

ljedrz commented Mar 12, 2019•
edited
Loading

mark-i-m commented Mar 13, 2019•
edited
Loading