operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that #50929

zackmdavis · 2018-05-21T03:51:02Z

See #50928 for motivation.

Questions—

Is HirId-ification initiative #50928 actually a good idea as a plan of record, or is there some reason to keep NodeIds?
Are the uses of find_node_for_hir_id in this initial submission OK (see the FIXME comments)?
Can we bikeshed a better method names struct_span_lint_hir &c.? (Coined in analogy to the struct_span_lint_node and NodeId, but it feels kind of semantically clunky.)

michaelwoerister · 2018-05-21T12:39:37Z

src/librustc/middle/expr_use_visitor.rs

-                    delegate.decl_without_init(id, span);
+                local.pat.each_binding(|_, hir_id, span, _| {
+                    // FIXME: converting HirId → NodeId is said to be relatively expensive
+                    let node_id = self.mc.tcx.hir.definitions().find_node_for_hir_id(hir_id);


You can use self.mc.tcx.hir.hir_to_node_id() which is "just" a hash table lookup. find_node_for_hir_id() should actually be removed.

michaelwoerister · 2018-05-21T12:44:45Z

src/librustc/middle/liveness.rs

@@ -1265,22 +1259,23 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
        }
    }

-    fn access_var(&mut self, id: NodeId, nid: NodeId, succ: LiveNode, acc: u32, span: Span)
+    fn access_var(&mut self, hir_id: HirId, nid: NodeId, succ: LiveNode, acc: u32, span: Span)


You could change nid to HirId too, I think.

This doesn't feel like an organic part of this somewhat-narrowly-focused cleanup PR to me (value called as nid is ultimately coming out of a Def::Local(NodeId) variant, in contrast to hir::Pat already having a hir_id field)?

michaelwoerister · 2018-05-21T12:47:15Z

src/librustc/ty/context.rs

+        -> DiagnosticBuilder<'tcx>
+    {
+        // FIXME: converting HirId → NodeId is said to be relatively expensive
+        let node_id = self.hir.definitions().find_node_for_hir_id(hir_id);


Same as above: self.hir.hir_to_node_id() will make it fast.

michaelwoerister · 2018-05-21T12:54:55Z

Thanks for the PR, @zackmdavis!

Is #50928 actually a good idea as a plan of record, or is there some reason to keep NodeIds?

Mostly yes, I think. I know that @eddyb wants to get rid of NodeId, and I do too. It's good to see some progress here. Does anyone in @rust-lang/compiler think that we should not further replace NodeId with HirId?

Are the uses of find_node_for_hir_id in this initial submission OK (see the FIXME comments)?

There's a better alternative (which has been introduced later). See my comments in the code.

Can we bikeshed a better method names struct_span_lint_hir &c.? (Coined in analogy to the struct_span_lint_node and NodeId, but it feels kind of semantically clunky.)

I think the names are fine.

zackmdavis · 2018-05-27T03:04:42Z

@michaelwoerister (rebased, with a new commit using hir_to_node_id (everywhere))

michaelwoerister · 2018-05-28T08:05:46Z

Thanks, @zackmdavis!

@bors r+

bors · 2018-05-28T08:05:48Z

📌 Commit 6dd885e has been approved by michaelwoerister

bors · 2018-05-28T12:45:16Z

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

Changing the `each_binding` utility method to take the `HirId` of a binding pattern rather than its `NodeId` seems like a modest first step in support of the `HirId`ification initiative rust-lang#50928. (The inspiration for choosing this in particular came from the present author's previous work on diagnostics issued during liveness analysis, which is the most greatly affected module in this change.)

Michael Woerister pointed out that `hir_to_node_id` (introduced in August 2017's 28ddd7a) supersedes the functionality of `find_node_for_hir_id` (just a hash lookup compared to that linear search).

zackmdavis · 2018-05-28T16:21:24Z

@michaelwoerister (rebased to fix conflict ↑)

michaelwoerister · 2018-05-28T17:28:22Z

Thanks!

@bors r+

bors · 2018-05-28T17:28:23Z

📌 Commit 1b7488d has been approved by michaelwoerister

bors · 2018-05-28T19:16:33Z

⌛ Testing commit 1b7488d with merge 5bf68db...

@michaelwoerister

…lwoerister operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that See #50928 for motivation. Questions— * Is #50928 actually a good idea as a plan of record, or is there some reason to keep `NodeId`s? * Are the uses of `find_node_for_hir_id` in this initial submission OK (see the FIXME comments)? * Can we bikeshed a better method names `struct_span_lint_hir` _&c._? (Coined in analogy to the `struct_span_lint_node` and `NodeId`, but it feels kind of semantically clunky.) r? @michaelwoerister

bors · 2018-05-28T21:30:12Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 5bf68db to master...

@michaelwoerister

HirId-ification, continued Another incremental step towards the vision of #50928 (previously: #50929). r? @michaelwoerister

rust-highfive assigned michaelwoerister May 21, 2018

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

michaelwoerister reviewed May 21, 2018

View reviewed changes

michaelwoerister 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 May 23, 2018

zackmdavis force-pushed the hiridification_initiative branch 2 times, most recently from 9ee1803 to 6dd885e Compare May 27, 2018 01:28

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 May 28, 2018

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 May 28, 2018

zackmdavis added 3 commits May 28, 2018 09:20

in which NodeMap and friends are macrotized!

078486b

scrap find_node_for_hir_id in favor of hir_to_node_id

1b7488d

Michael Woerister pointed out that `hir_to_node_id` (introduced in August 2017's 28ddd7a) supersedes the functionality of `find_node_for_hir_id` (just a hash lookup compared to that linear search).

zackmdavis force-pushed the hiridification_initiative branch from 6dd885e to 1b7488d Compare May 28, 2018 16:20

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 May 28, 2018

bors merged commit 1b7488d into rust-lang:master May 28, 2018

zackmdavis deleted the hiridification_initiative branch May 28, 2018 21:30

bors mentioned this pull request May 28, 2018

merge unused-extern-crate and unnecessary-extern-crate lints #51015

Merged

4 tasks

zackmdavis mentioned this pull request Jun 3, 2018

HirId-ification, continued #51321

Merged

bors added a commit that referenced this pull request Jul 2, 2018

Auto merge of #51321 - zackmdavis:hiridification_generations, r=eddyb

b58b721

HirId-ification, continued Another incremental step towards the vision of #50928 (previously: #50929). r? @michaelwoerister

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that #50929

operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that #50929

zackmdavis commented May 21, 2018

michaelwoerister May 21, 2018

michaelwoerister May 21, 2018

zackmdavis May 27, 2018

michaelwoerister May 21, 2018

michaelwoerister commented May 21, 2018

zackmdavis commented May 27, 2018

michaelwoerister commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

zackmdavis commented May 28, 2018

michaelwoerister commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

operate on HirId instead of NodeId in hir::Pat::each_binding, and consequences of that #50929

operate on HirId instead of NodeId in hir::Pat::each_binding, and consequences of that #50929

Conversation

zackmdavis commented May 21, 2018

michaelwoerister May 21, 2018

Choose a reason for hiding this comment

michaelwoerister May 21, 2018

Choose a reason for hiding this comment

zackmdavis May 27, 2018

Choose a reason for hiding this comment

michaelwoerister May 21, 2018

Choose a reason for hiding this comment

michaelwoerister commented May 21, 2018

zackmdavis commented May 27, 2018

michaelwoerister commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

zackmdavis commented May 28, 2018

michaelwoerister commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that #50929

operate on `HirId` instead of `NodeId` in `hir::Pat::each_binding`, and consequences of that #50929