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

HirIdification: almost there #58915

Merged
merged 7 commits into from
Mar 8, 2019
Merged

HirIdification: almost there #58915

merged 7 commits into from
Mar 8, 2019

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Mar 4, 2019 •
edited
Loading

The next iteration of HirIdification (#57578).

Replaces a bunch of NodeId method calls (mostly as_local_node_id) with HirId ones.

Removes NodeId from:

  • PathSegment
  • PatKind
  • Destination (replaces it with HirId)

In addition this PR also removes Visitor::visit_def_mention, which doesn't seem to be doing anything.

@rust-highfive
Copy link
Collaborator

r? @cramertj

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2019
@ljedrz
Copy link
Contributor Author

ljedrz commented Mar 4, 2019

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned cramertj Mar 4, 2019
Zoxc
Zoxc reviewed Mar 4, 2019
View reviewed changes
src/librustc/hir/map/mod.rs
span_bug!(self.span(id), "body_owned_by: {} has no associated body",
self.node_to_string(id));
pub fn body_owned_by(&self, id: HirId) -> BodyId {
self.maybe_body_owned_by_by_hir_id(id).unwrap_or_else(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename maybe_body_owned_by_by_hir_id to maybe_body_owned_by_hir_id? =P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that one does sound silly ^^. I'm hoping to soon be able to just remove it in favor of just maybe_body_owned_by.

@Zoxc
Copy link
Contributor

Zoxc commented Mar 4, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2019

📌 Commit 296eaa520d1d1e1f2e582b05e2dfe06ee2cbe87f has been approved by Zoxc

@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 Mar 4, 2019
@bors
Copy link
Contributor

bors commented Mar 7, 2019

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 7, 2019
@ljedrz
Copy link
Contributor Author

ljedrz commented Mar 7, 2019

Rebased.

@ljedrz ljedrz mentioned this pull request Mar 7, 2019
Closed
3 tasks
@Zoxc
Copy link
Contributor

Zoxc commented Mar 7, 2019

@bors delegate+ r+

@bors
Copy link
Contributor

bors commented Mar 7, 2019

📌 Commit cd06038 has been approved by Zoxc

@bors
Copy link
Contributor

bors commented Mar 7, 2019

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

@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 Mar 7, 2019
@bors
Copy link
Contributor

bors commented Mar 7, 2019

✌️ @ljedrz can now approve this pull request

@felix91gr
Copy link
Contributor

I don't know where to ask this otherwise, so please forgive me if the question is out of band.

Question:

I'm seeing HirIdification all over rustc and clippy. Is HirId intended to replace all uses of NodeId eventually? I'm asking because I'm working on a Clippy lint that tries to evaluate some values in order to detect a null -> reference tranmute call. When implementing evaluation of local variables (the one case we're missing), I saw that I needed to evaluate a Def::Local value, which as you can see in the link still uses NodeId.

Therefore, should I wait until the full HirIdification, or will this usage of NodeId keep being at the syntax-level?

Thank you so much, and sorry for disrupting your work 🙇‍♂️

@Zoxc
Copy link
Contributor

Zoxc commented Mar 8, 2019

@felix91gr Eventually all NodeId uses after lowering to HIR will be replaced with HirId. NodeIds will still be used before that.

You can freely convert between NodeId and HirId though with tcx.hir().hir_to_node_id and tcx.hir().node_to_hir_id, so I don't see good reason to wait for anything here. You may just have to remove these calls and update your types later on.

@felix91gr
Copy link
Contributor

Eventually all NodeId uses after lowering to HIR will be replaced with HirId. NodeIds will still be used before that.

Ah, cool, that makes perfect sense :) thanks!

You can freely convert between NodeId and HirId though.

Ohhhhh! That's great, I didn't know that! Thank you 😊

@ljedrz
Copy link
Contributor Author

ljedrz commented Mar 8, 2019

@bors r=Zoxc

@bors
Copy link
Contributor

bors commented Mar 8, 2019

📌 Commit d7120e4 has been approved by Zoxc

@ljedrz ljedrz changed the title HirIdification: replace NodeId method calls HirIdification: almost there Mar 8, 2019
@ljedrz
Copy link
Contributor Author

ljedrz commented Mar 8, 2019

Added a clippy update that supersedes #58966. Bumping priority to 1.

@bors r=Zoxc p=1

@bors
Copy link
Contributor

bors commented Mar 8, 2019

📌 Commit 24fad4c has been approved by Zoxc

@ljedrz ljedrz mentioned this pull request Mar 8, 2019
Closed
@bors
Copy link
Contributor

bors commented Mar 8, 2019

⌛ Testing commit 24fad4c with merge 2a65cbe...

bors added a commit that referenced this pull request Mar 8, 2019
@bors
Auto merge of #58915 - ljedrz:deprecate_nodeid_methods, r=Zoxc
2a65cbe 
HirIdification: almost there

The next iteration of HirIdification (#57578).

Replaces a bunch of `NodeId` method calls (mostly `as_local_node_id`) with `HirId` ones.

Removes `NodeId` from:
- [x] `PathSegment`
- [x] `PatKind`
- [x] `Destination` (replaces it with `HirId`)

In addition this PR also removes `Visitor::visit_def_mention`, which doesn't seem to be doing anything.
@bors
Copy link
Contributor

bors commented Mar 8, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2019
@bors bors merged commit 24fad4c into rust-lang:master Mar 8, 2019
@bors bors mentioned this pull request Mar 8, 2019
Merged
@ljedrz ljedrz deleted the deprecate_nodeid_methods branch March 8, 2019 17:06
@bors bors mentioned this pull request Mar 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zoxc Zoxc Zoxc left review comments

Assignees

@Zoxc Zoxc

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ljedrz @rust-highfive @Zoxc @bors @felix91gr @cramertj