Use hir::ItemLocalId as keys in TypeckTables. #43740

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

src/librustc/middle/dead.rs

@@ -119,6 +119,8 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {

    fn handle_field_pattern_match(&mut self, lhs: &hir::Pat, def: Def,
                                  pats: &[codemap::Spanned<hir::FieldPat>]) {
+


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

src/librustc/middle/reachable.rs

@@ -375,7 +375,7 @@ fn reachable_set<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, crate_num: CrateNum) ->
    });
    let mut reachable_context = ReachableContext {
        tcx,
-        tables: &ty::TypeckTables::empty(),
+        tables: &ty::TypeckTables::empty(DefId::invalid()),


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

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

src/librustc/ty/context.rs

        TypeckTables {
-            type_dependent_defs: NodeMap(),
+            local_id_root,


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

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

src/librustc/ty/context.rs

@@ -384,6 +390,30 @@ impl<'tcx> TypeckTables<'tcx> {
    pub fn upvar_capture(&self, upvar_id: ty::UpvarId) -> ty::UpvarCapture<'tcx> {
        self.upvar_capture_map[&upvar_id]
    }
+
+    /// Validate that a NodeId can safely be converted to an ItemLocalId for


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

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

src/librustc/hir/map/definitions.rs

@@ -466,7 +466,11 @@ impl Definitions {
    }

    pub fn find_node_for_hir_id(&self, hir_id: hir::HirId) -> ast::NodeId {


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

src/librustc/ty/context.rs

@@ -207,52 +207,55 @@ pub struct CommonTypes<'tcx> {

 #[derive(RustcEncodable, RustcDecodable)]
 pub struct TypeckTables<'tcx> {
+    /// The HirId::owner all ItemLocalIds in this table are relative to.
+    pub local_id_root: DefId,


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

pub struct LocalTableInContext<'a, V> {
    parent_def_id: Option<DefId>,
    data: &'a LocalItemMap<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

src/librustc/hir/def_id.rs

@@ -201,4 +201,11 @@ impl DefId {
    pub fn is_local(&self) -> bool {
        self.krate == LOCAL_CRATE
    }
+


unused function plz kill

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

src/librustc/middle/dead.rs

@@ -119,7 +119,9 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {

    fn handle_field_pattern_match(&mut self, lhs: &hir::Pat, def: Def,
                                  pats: &[codemap::Spanned<hir::FieldPat>]) {
-        let variant = match self.tables.node_id_to_type(lhs.id).sty {
+


nit: stray newlines in program

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

src/librustc/ty/context.rs

+}
+
+impl<'a, V> LocalTableInContextMut<'a, V> {
+


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

src/librustc_privacy/lib.rs

@@ -1606,21 +1656,26 @@ fn privacy_access_levels<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

    let krate = tcx.hir.krate();

+    let empty_tables = ty::TypeckTables::empty(None);
+


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

src/librustc/ty/context.rs

+/// 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

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 75022ed Compare August 10, 2017 14:30

michaelwoerister force-pushed the local-id-in-typecktables branch from 91207de to c078583 Compare 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 a69eaf6 Compare 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

Fix some merge fallout.

6fd7d85

michaelwoerister force-pushed the local-id-in-typecktables branch from b30f28a to 6fd7d85 Compare 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:master Aug 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hir::ItemLocalId as keys in TypeckTables. #43740

Use hir::ItemLocalId as keys in TypeckTables. #43740

michaelwoerister commented Aug 8, 2017

eddyb commented Aug 8, 2017

arielb1 Aug 9, 2017

arielb1 Aug 9, 2017

arielb1 Aug 9, 2017

arielb1 Aug 9, 2017

arielb1 Aug 9, 2017

arielb1 Aug 9, 2017

arielb1 commented Aug 9, 2017

michaelwoerister commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 11, 2017

michaelwoerister commented Aug 11, 2017

michaelwoerister commented Aug 11, 2017

arielb1 Aug 13, 2017

arielb1 Aug 13, 2017

arielb1 Aug 13, 2017

michaelwoerister Aug 14, 2017

arielb1 Aug 13, 2017

arielb1 commented Aug 13, 2017

michaelwoerister commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

kennytm Aug 14, 2017

michaelwoerister Aug 14, 2017

michaelwoerister commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

		@@ -119,6 +119,8 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {

		fn handle_field_pattern_match(&mut self, lhs: &hir::Pat, def: Def,
		pats: &[codemap::Spanned<hir::FieldPat>]) {

		@@ -466,7 +466,11 @@ impl Definitions {
		}

		pub fn find_node_for_hir_id(&self, hir_id: hir::HirId) -> ast::NodeId {

		@@ -1606,21 +1656,26 @@ fn privacy_access_levels<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

		let krate = tcx.hir.krate();

		let empty_tables = ty::TypeckTables::empty(None);

Use hir::ItemLocalId as keys in TypeckTables. #43740

Use hir::ItemLocalId as keys in TypeckTables. #43740

Conversation

michaelwoerister commented Aug 8, 2017

eddyb commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Aug 9, 2017

michaelwoerister commented Aug 10, 2017

bors commented Aug 10, 2017

bors commented Aug 11, 2017

michaelwoerister commented Aug 11, 2017

michaelwoerister commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Aug 13, 2017

michaelwoerister commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017

bors commented Aug 14, 2017