Use hir::ItemLocalId as keys in TypeckTables. by michaelwoerister · Pull Request #43740 · rust-lang/rust

michaelwoerister · 2017-08-08T14:02:47Z

This PR makes TypeckTables use ItemLocalId instead of NodeId as key. This is needed for incremental compilation -- for stable hashing and for being able to persist and reload these tables. The PR implements the most important part of #40303.

Some notes on the implementation:

The PR adds the HirId to HIR nodes where needed (Expr, Local, Block, Pat) which obviates the need to store a NodeId -> HirId mapping in crate metadata. Thanks @eddyb for the suggestion! In the future the HirId should completely replace the NodeId in HIR nodes.
Before something is read or stored in one of the various TypeckTables subtables, the entry's key is validated via the new TypeckTables::validate_hir_id() method. This makes sure that we are not mixing information from different items in a single table.

That last part could be made a bit nicer by either (a) new-typing the table-key and making validate_hir_id() the only way to convert a HirId to the new-typed key, or (b) just encapsulate sub-table access a little better. This PR, however, contents itself with not making things significantly worse.

Also, there's quite a bit of switching around between NodeId, HirId, and DefIndex. These conversions are cheap except for HirId -> NodeId, so if the valued reviewer finds such an instance in a performance critical place, please let me know.

Ideally we convert more and more code from NodeId to HirId in the future so that there are no more NodeIds after HIR lowering anywhere. Then the amount of switching should be minimal again.

r? @eddyb, maybe?

eddyb · 2017-08-08T14:09:46Z

LGTM. r? @arielb1

arielb1 · 2017-08-09T15:46:22Z

arielb1 · 2017-08-09T15:49:18Z

I think this should be a FIXME? Shouldn't tables be None when outside of an item?

arielb1 · 2017-08-09T16:09:56Z

The thing with local_id_root being sometimes invalid makes me uncomfortable. Please insert an Optionsomewhere (I think making local_id_root an option would work well).

arielb1 · 2017-08-09T16:13:06Z

When is this function called? It seems to be called on some random HIR id accesses.

arielb1 · 2017-08-09T16:22:03Z

Can't you rather do some sort of merging of this function with validate_hir_id? Unmarked linear searches are dangerous.

arielb1 · 2017-08-09T16:29:50Z

This is in several places initialized with an invalid_def_id. Why can't this be an Option?

arielb1 · 2017-08-09T16:34:02Z

I don't like the way validate_def_id is spread around randomly in code. I think I would make the TypeckTables fields to be functions that return a

pubstructLocalTableInContext<'a,V>{parent_def_id:Option<DefId>,data:&'aLocalItemMap<V>}

Which does validation on the accessor methods. Or something.

michaelwoerister · 2017-08-10T10:07:18Z

OK, I'll change the various visitors to use an Option<&TypeckTables>. I don't like make the local_id_root optional, since every TypeckTables should really have one of these.

I'll also look into a way of handling HirId validation in a more central place.

bors · 2017-08-10T11:21:30Z

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

bors · 2017-08-11T04:57:22Z

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

…eId.

…n TypeckTables.

…les.

… binary one. Unfortunately, the NodeId->HirId array is not sorted. Since this search is only done right before calling bug!(), let's not waste time allocating a faster lookup.

…h access.

michaelwoerister · 2017-08-11T14:11:06Z

@arielb1 I think I addressed your comments.

michaelwoerister · 2017-08-11T14:55:22Z

(Note: I've added the last three commits. The others were not touched except for rebasing).

arielb1 · 2017-08-13T10:49:07Z

unused function plz kill

arielb1 · 2017-08-13T10:51:20Z

nit: stray newlines in program

arielb1 · 2017-08-13T10:54:04Z

nit: stray newline in program

I actually like newlines in places like this. Makes the impl line easier to read. But I'll remove it for consistency.

arielb1 · 2017-08-13T10:57:29Z

nit: stray newline in program (I think)

arielb1 · 2017-08-13T11:02:26Z

Nice! r=me modulo nits.

michaelwoerister · 2017-08-14T09:22:59Z

@bors r=arielb1

Amended the nit fixes to the last commit. Thanks for the reviews!

bors · 2017-08-14T09:23:00Z

📌 Commit 6fd7d85 has been approved by arielb1

bors · 2017-08-14T11:12:40Z

⌛ Testing commit 6fd7d85 with merge 52523ded9a2d39d7a76f41a9d373610cdeecf501...

bors · 2017-08-14T11:28:10Z

💔 Test failed - status-travis

kennytm · 2017-08-14T12:37:49Z

+/// stored/returned.
+fn validate_hir_id_for_typeck_tables(local_id_root: Option<DefId>,
+ hir_id: hir::HirId,
+ mut_access: bool) {


[00:10:17] error: unused variable: `local_id_root` [00:10:17] --> /checkout/src/librustc/ty/context.rs:226:38 [00:10:17] | [00:10:17] 226 | fn validate_hir_id_for_typeck_tables(local_id_root: Option<DefId>, [00:10:17] | ^^^^^^^^^^^^^ [00:10:17] | [00:10:17] [00:10:17] error: unused variable: `hir_id` [00:10:17] --> /checkout/src/librustc/ty/context.rs:227:38 [00:10:17] | [00:10:17] 227 | hir_id: hir::HirId, [00:10:17] | ^^^^^^ [00:10:17] [00:10:17] error: unused variable: `mut_access` [00:10:17] --> /checkout/src/librustc/ty/context.rs:228:38 [00:10:17] | [00:10:17] 228 | mut_access: bool) { [00:10:17] | ^^^^^^^^^^ [00:10:17] [00:10:18] error: aborting due to 3 previous errors [00:10:18] [00:10:18] error: Could not compile `rustc`.

Thanks, @kennytm! That must be because of the cfg(debug_assertions) switch...

michaelwoerister · 2017-08-14T12:57:55Z

@bors r=arielb1

bors · 2017-08-14T12:57:56Z

📌 Commit 3b92b97 has been approved by arielb1

bors · 2017-08-14T14:15:12Z

⌛ Testing commit 3b92b97 with merge 0d12553...

@eddyb

…rielb1 Use hir::ItemLocalId as keys in TypeckTables. This PR makes `TypeckTables` use `ItemLocalId` instead of `NodeId` as key. This is needed for incremental compilation -- for stable hashing and for being able to persist and reload these tables. The PR implements the most important part of #40303. Some notes on the implementation: * The PR adds the `HirId` to HIR nodes where needed (`Expr`, `Local`, `Block`, `Pat`) which obviates the need to store a `NodeId -> HirId` mapping in crate metadata. Thanks @eddyb for the suggestion! In the future the `HirId` should completely replace the `NodeId` in HIR nodes. * Before something is read or stored in one of the various `TypeckTables` subtables, the entry's key is validated via the new `TypeckTables::validate_hir_id()` method. This makes sure that we are not mixing information from different items in a single table. That last part could be made a bit nicer by either (a) new-typing the table-key and making `validate_hir_id()` the only way to convert a `HirId` to the new-typed key, or (b) just encapsulate sub-table access a little better. This PR, however, contents itself with not making things significantly worse. Also, there's quite a bit of switching around between `NodeId`, `HirId`, and `DefIndex`. These conversions are cheap except for `HirId -> NodeId`, so if the valued reviewer finds such an instance in a performance critical place, please let me know. Ideally we convert more and more code from `NodeId` to `HirId` in the future so that there are no more `NodeId`s after HIR lowering anywhere. Then the amount of switching should be minimal again. r? @eddyb, maybe?

bors · 2017-08-14T17:04:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 0d12553 to master...

rust-highfive assigned eddyb Aug 8, 2017

rust-highfive assigned arielb1 and unassigned eddyb Aug 8, 2017

arielb1 reviewed Aug 9, 2017
View reviewed changes

Comment threadsrc/librustc/middle/dead.rs Outdated
arielb1Aug 9, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?

arielb1 reviewed Aug 9, 2017
View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 10, 2017

michaelwoerister force-pushed the local-id-in-typecktables branch from 49abb36 to 75022edCompare August 10, 2017 14:30

michaelwoerister force-pushed the local-id-in-typecktables branch from 91207de to c078583Compare August 11, 2017 10:09

michaelwoerister added 13 commits August 11, 2017 12:11

Make TypeckTables::type_dependent_defs use ItemLocalId instead of Nod…
783ccc4
…eId.

Use ItemLocalId as key for node_types, node_substs, and adjustments i…
7f2423e
…n TypeckTables.

Use ItemLocalId as key for TypeckTables::pat_binding_modes.
e777189

Use ItemLocalId as key for closure_tys and closure_kinds in TypeckTab…
454533f
…les.

Make Definitions::find_node_for_hir_id() a linear search instead of a…
55e04d9
… binary one. Unfortunately, the NodeId->HirId array is not sorted. Since this search is only done right before calling bug!(), let's not waste time allocating a faster lookup.

Add missing TypeckTables-switch in save-analysis.
6cd44a9

Use ItemLocalId as key for TypeckTables::liberated_fn_sigs.
890f93f

Use ItemLocalId as key for TypeckTables::fru_field_types.
801dd07

Use ItemLocalId as key for TypeckTables::cast_kinds.
fbc7398

Add some ID conversion methods to HIR map and Definitions.
a8cf6cc

Use DefIndex instead of NodeId in UpvarId.
4dcc3a4

Improve documentation for TypeckTables::validate_hir_id().
bdfd78d

Encapsulate sub-table access in TypeckTables and validate keys on eac…
1f54df1
…h access.

Improve validation of TypeckTables keys.
a69eaf6

michaelwoerister force-pushed the local-id-in-typecktables branch from c078583 to a69eaf6Compare August 11, 2017 10:17

michaelwoerister added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2017

arielb1 reviewed Aug 13, 2017
View reviewed changes

Comment threadsrc/librustc/hir/def_id.rs Outdated
arielb1Aug 13, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused function plz kill

arielb1 reviewed Aug 13, 2017
View reviewed changes

Comment threadsrc/librustc/middle/dead.rs Outdated
arielb1Aug 13, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stray newlines in program

arielb1 reviewed Aug 13, 2017
View reviewed changes

Comment threadsrc/librustc_privacy/lib.rs Outdated
arielb1Aug 13, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stray newline in program (I think)

Fix some merge fallout.
6fd7d85

michaelwoerister force-pushed the local-id-in-typecktables branch from b30f28a to 6fd7d85Compare August 14, 2017 09:21

kennytm reviewed Aug 14, 2017
View reviewed changes

Fix unused variable warnings in builds disabled debug-assertions.
3b92b97

bors merged commit 3b92b97 into rust-lang:masterAug 14, 2017

This was referenced Aug 14, 2017
Generator support #43076
Merged
use field init shorthand EVERYWHERE #43710
Merged

zackmdavis mentioned this pull request May 21, 2018
HirId-ification initiative #50928
Closed

Uh oh!

Uh oh!

Conversation

michaelwoerister commented Aug 8, 2017

Uh oh!

eddyb commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Aug 9, 2017

Uh oh!

michaelwoerister commented Aug 10, 2017

Uh oh!

bors commented Aug 10, 2017

Uh oh!

bors commented Aug 11, 2017

Uh oh!

michaelwoerister commented Aug 11, 2017

Uh oh!

michaelwoerister commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Aug 13, 2017

Uh oh!

michaelwoerister commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

bors commented Aug 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants