Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

eddyb · 2017-08-30T01:02:43Z

This is an alternative to @michaelwoerister's #43887, changing CodeExtent instead of ReScope.

The benefit here is that the same Regions are used same-crate and cross-crate, while preserving the incremental recompilation properties of the stable hir::ItemLocalId.

Only places which needed to get back to the ast::NodeId from CodeExtent was its span method, used in error reporting - passing the &RegionMaps down allowed using hir_to_node_id.
rustc::cfg and dataflow also had to be converted to hir::ItemLocalId because of their interactions with CodeExtent, especially in borrowck, and from that we have 3 more hir_to_node_id calls: cfg::graphviz node labels, borrowck move reporting, and the unconditional_recursion lint.

Out of all of those, only the lint actually makes a decision (on whether code will compile) based on the result of the conversion, the others only use it to know how to print information to the user.
So I think we're safe to say that the bulk of the code working with a CodeExtent is fine with local IDs.

r? @nikomatsakis

eddyb · 2017-08-30T12:28:44Z

Crater report shows no regressions, so I'm pretty confident this works.

bors · 2017-08-30T14:00:14Z

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

nikomatsakis

OK, I can't claim to have read these in depth, but everything looked straightforward to me. Basically what I would expect.

nikomatsakis · 2017-08-30T15:37:02Z

src/librustc/middle/region.rs

+
+    /// If there are any `yield` nested within a scope, this map
+    /// stores the `Span` of the first one.
+    yield_in_scope: FxHashMap<CodeExtent, Span>,


Why did you have to add this?

er, never mind, I found the answer later.

nikomatsakis · 2017-08-30T16:25:45Z

@bors r+

bors · 2017-08-30T16:25:46Z

📌 Commit 46ba34a has been approved by nikomatsakis

eddyb · 2017-08-30T20:24:17Z

@bors r=nikomatsakis

bors · 2017-08-30T20:24:18Z

📌 Commit ae82c78 has been approved by nikomatsakis

bors · 2017-08-30T23:02:30Z

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

eddyb · 2017-08-31T07:53:37Z

@bors r=nikomatsakis

bors · 2017-08-31T07:53:38Z

📌 Commit 7c18f29 has been approved by nikomatsakis

bors · 2017-08-31T11:49:49Z

⌛ Testing commit 7c18f29d1aece4414ada3c74e74f6ee5343e3f84 with merge a5d023839e716258e1e4e7caf193e7799f57420a...

bors · 2017-08-31T12:54:19Z

💔 Test failed - status-appveyor

nikomatsakis · 2017-08-31T13:56:01Z

Hmm, the failure looks spurious to me:

testing https://github.com/Aaronepower/tokei
Initialized empty Git repository in /c/projects/rust/build/ct/tokei/.git/
fatal: Could not parse object '5e11c4852fe4aa086b0e4fe5885822fbe57ba928'.
From https://github.com/Aaronepower/tokei
 * branch            master     -> FETCH_HEAD
fatal: Could not parse object '5e11c4852fe4aa086b0e4fe5885822fbe57ba928'.
From https://github.com/Aaronepower/tokei
 * branch            master     -> FETCH_HEAD
fatal: Could not parse object '5e11c4852fe4aa086b0e4fe5885822fbe57ba928'.
From https://github.com/Aaronepower/tokei
 * branch            master     -> FETCH_HEAD
HEAD is now at 5e11c48 Add Agda Support (#91)
 Downloading lazy_static v0.2.2
error: unable to get packages from source

nikomatsakis · 2017-08-31T13:56:21Z

@bors retry

failure to fetch Aaronepower/tokei repository (cc @rust-lang/infra -- not sure how to categorize these)

bors · 2017-08-31T15:46:35Z

⌛ Testing commit 7c18f29d1aece4414ada3c74e74f6ee5343e3f84 with merge 8547ab847c9d633d86b8bd1d647f42bb226e90ed...

bors · 2017-08-31T17:00:22Z

💔 Test failed - status-travis

eddyb · 2017-08-31T17:41:29Z

@bors retry

macOS timeout

bors · 2017-08-31T17:56:24Z

⌛ Testing commit 7c18f29d1aece4414ada3c74e74f6ee5343e3f84 with merge 3a544be3fba295f7b27e64831430c44da770ead5...

bors · 2017-08-31T19:10:14Z

💔 Test failed - status-travis

eddyb · 2017-08-31T19:13:34Z

Is nothing landing lately?

kennytm · 2017-08-31T19:25:34Z

@bors retry #40474 (still check i686-apple-Darwin, but different error)

[00:28:20] [ 44%] Building CXX object lib/Analysis/CMakeFiles/LLVMAnalysis.dir/AliasAnalysis.cpp.o

[00:38:30] error: failed to execute compile

[00:38:30] caused by: error reading compile response from server

[00:38:30] caused by: Failed to read response header

[00:38:30] caused by: failed to fill whole buffer

[00:38:30] error: failed to execute compile

[00:38:30] caused by: error reading compile response from server

[00:38:30] caused by: Failed to read response header

[00:38:30] caused by: failed to fill whole buffer

[00:38:30] make[3]: *** [lib/LTO/CMakeFiles/LLVMLTO.dir/Caching.cpp.o] Error 2

[00:38:30] make[3]: *** [lib/Analysis/CMakeFiles/LLVMAnalysis.dir/AliasAnalysis.cpp.o] Error 2



No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

eddyb · 2017-09-01T08:18:56Z

@bors r=nikomatsakis

bors · 2017-09-01T08:18:57Z

📌 Commit e4996ec has been approved by nikomatsakis

@michaelwoerister

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. This is an alternative to @michaelwoerister's #43887, changing `CodeExtent` instead of `ReScope`. The benefit here is that the same `Region`s are used same-crate and cross-crate, while preserving the incremental recompilation properties of the stable `hir::ItemLocalId`. Only places which needed to get back to the `ast::NodeId` from `CodeExtent` was its `span` method, used in error reporting - passing the `&RegionMaps` down allowed using `hir_to_node_id`. `rustc::cfg` and `dataflow` also had to be converted to `hir::ItemLocalId` because of their interactions with `CodeExtent`, especially in `borrowck`, and from that we have 3 more `hir_to_node_id` calls: `cfg::graphviz` node labels, `borrowck` move reporting, and the `unconditional_recursion` lint. Out of all of those, *only* the lint actually makes a decision (on whether code will compile) based on the result of the conversion, the others only use it to know how to print information to the user. So I think we're safe to say that the bulk of the code working with a `CodeExtent` is fine with local IDs. r? @nikomatsakis

bors · 2017-09-01T10:40:26Z

⌛ Testing commit e4996ec with merge a59a6d8...

bors · 2017-09-01T13:18:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a59a6d8 to master...

michaelwoerister · 2017-09-04T09:17:22Z

Nice, thanks a lot @eddyb!

@eddyb

Store fewer NodeIds in crate metadata. The first commit just replaces `NodeId` with `DefId` in `resolve_lifetime::Region`, so we don't run into problems when stable-hashing `TypeParameterDef` values from other crates. ~~The second commit is more interesting. It adds the `ReScopeAnon` variant to `ty::RegionKind`, which is semantically equivalent to `ReScope`. The only difference is that it does not contain a `CodeExtent` field. All `ReScope` occurrences are then replaced with `ReScopeAnon` upon metadata export. This way we don't end up with `NodeIds` from other crates in various things imported from metadata. `ReScopeAnon` can still be tested for equality.~~ This is fixed in a better way by @eddyb in #44171. r? @eddyb cc @rust-lang/compiler

rust-highfive assigned nikomatsakis Aug 30, 2017

eddyb force-pushed the scope branch 2 times, most recently from dfd14c4 to 46ba34a Compare August 30, 2017 07:41

nikomatsakis approved these changes Aug 30, 2017

View reviewed changes

eddyb force-pushed the scope branch from 46ba34a to ae82c78 Compare August 30, 2017 20:24

eddyb force-pushed the scope branch from ae82c78 to 7c18f29 Compare August 31, 2017 07:53

eddyb mentioned this pull request Aug 31, 2017

Store fewer NodeIds in crate metadata. #43887

Closed

kennytm mentioned this pull request Aug 31, 2017

Spurious failure with stalled macOS builds (specifically the check i686-apple-darwin job) #44221

Closed

eddyb added 3 commits September 1, 2017 11:17

rustc: use hir::ItemLocalId instead of ast::NodeId in CFG.

28ddd7a

rustc: take TyCtxt and RegionMaps in CodeMap::span.

ea6aca7

rustc: use hir::ItemLocalId instead of ast::NodeId in CodeExtent.

e4996ec

eddyb force-pushed the scope branch from 7c18f29 to e4996ec Compare September 1, 2017 08:18

bors merged commit e4996ec into rust-lang:master Sep 1, 2017

bors mentioned this pull request Sep 1, 2017

Fix end region emission order #44129

Merged

eddyb deleted the scope branch September 1, 2017 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

eddyb commented Aug 30, 2017 •

edited

Loading

eddyb commented Aug 30, 2017

bors commented Aug 30, 2017

nikomatsakis left a comment

nikomatsakis Aug 30, 2017

nikomatsakis Aug 30, 2017

nikomatsakis commented Aug 30, 2017

bors commented Aug 30, 2017

eddyb commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

eddyb commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

eddyb commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

eddyb commented Aug 31, 2017

kennytm commented Aug 31, 2017

eddyb commented Sep 1, 2017

bors commented Sep 1, 2017

bors commented Sep 1, 2017

bors commented Sep 1, 2017

michaelwoerister commented Sep 4, 2017

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

Conversation

eddyb commented Aug 30, 2017 • edited Loading

eddyb commented Aug 30, 2017

bors commented Aug 30, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Aug 30, 2017

Choose a reason for hiding this comment

nikomatsakis Aug 30, 2017

Choose a reason for hiding this comment

nikomatsakis commented Aug 30, 2017

bors commented Aug 30, 2017

eddyb commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

eddyb commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

nikomatsakis commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

eddyb commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

eddyb commented Aug 31, 2017

kennytm commented Aug 31, 2017

eddyb commented Sep 1, 2017

bors commented Sep 1, 2017

bors commented Sep 1, 2017

bors commented Sep 1, 2017

michaelwoerister commented Sep 4, 2017

eddyb commented Aug 30, 2017 •

edited

Loading