Auto merge of #43740 - michaelwoerister:local-id-in-typecktables, r=arielb1

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?

Author: bors

src/librustc/hir/lowering.rs

@@ -434,18 +434,22 @@ impl Definitions {
         DefPath::make(LOCAL_CRATE, index, |p| self.def_key(p))
     }
 
+    #[inline]
     pub fn opt_def_index(&self, node: ast::NodeId) -> Option<DefIndex> {
         self.node_to_def_index.get(&node).cloned()
     }
 
+    #[inline]
     pub fn opt_local_def_id(&self, node: ast::NodeId) -> Option<DefId> {
         self.opt_def_index(node).map(DefId::local)
     }
 
+    #[inline]
     pub fn local_def_id(&self, node: ast::NodeId) -> DefId {
         self.opt_local_def_id(node).unwrap()
     }
 
+    #[inline]
     pub fn as_local_node_id(&self, def_id: DefId) -> Option<ast::NodeId> {
         if def_id.krate == LOCAL_CRATE {
             let space_index = def_id.index.address_space().index();
@@ -461,10 +465,27 @@ impl Definitions {
         }
     }
 
+    #[inline]
     pub fn node_to_hir_id(&self, node_id: ast::NodeId) -> hir::HirId {
         self.node_to_hir_id[node_id]
     }
 
+    pub fn find_node_for_hir_id(&self, hir_id: hir::HirId) -> ast::NodeId {
+        self.node_to_hir_id
+            .iter()
+            .position(|x| *x == hir_id)
+            .map(|idx| ast::NodeId::new(idx))
+            .unwrap()
+    }
+
+    #[inline]
+    pub fn def_index_to_hir_id(&self, def_index: DefIndex) -> hir::HirId {
+        let space_index = def_index.address_space().index();
+        let array_index = def_index.as_array_index();
+        let node_id = self.def_index_to_node[space_index][array_index];
+        self.node_to_hir_id[node_id]
+    }
+
     /// Add a definition with a parent definition.
     pub fn create_root_def(&mut self,
                            crate_name: &str,

src/librustc/hir/map/definitions.rs

@@ -357,6 +357,7 @@ impl<'hir> Map<'hir> {
         }
     }
 
+    #[inline]
     pub fn definitions(&self) -> &Definitions {
         &self.definitions
     }
@@ -377,21 +378,39 @@ impl<'hir> Map<'hir> {
         self.definitions.def_path(def_id.index)
     }
 
+    #[inline]
     pub fn local_def_id(&self, node: NodeId) -> DefId {
         self.opt_local_def_id(node).unwrap_or_else(|| {
             bug!("local_def_id: no entry for `{}`, which has a map of `{:?}`",
                  node, self.find_entry(node))
         })
     }
 
+    #[inline]
     pub fn opt_local_def_id(&self, node: NodeId) -> Option<DefId> {
         self.definitions.opt_local_def_id(node)
     }
 
+    #[inline]
     pub fn as_local_node_id(&self, def_id: DefId) -> Option<NodeId> {
         self.definitions.as_local_node_id(def_id)
     }
 
+    #[inline]
+    pub fn node_to_hir_id(&self, node_id: NodeId) -> HirId {
+        self.definitions.node_to_hir_id(node_id)
+    }
+
+    #[inline]
+    pub fn def_index_to_hir_id(&self, def_index: DefIndex) -> HirId {
+        self.definitions.def_index_to_hir_id(def_index)
+    }
+
+    #[inline]
+    pub fn def_index_to_node_id(&self, def_index: DefIndex) -> NodeId {
+        self.definitions.as_local_node_id(DefId::local(def_index)).unwrap()
+    }
+
     fn entry_count(&self) -> usize {
         self.map.len()
     }

src/librustc/hir/map/mod.rs

@@ -129,9 +129,11 @@ pub const CRATE_HIR_ID: HirId = HirId {
 
 pub const DUMMY_HIR_ID: HirId = HirId {
     owner: CRATE_DEF_INDEX,
-    local_id: ItemLocalId(!0)
+    local_id: DUMMY_ITEM_LOCAL_ID,
 };
 
+pub const DUMMY_ITEM_LOCAL_ID: ItemLocalId = ItemLocalId(!0);
+
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Copy)]
 pub struct Lifetime {
     pub id: NodeId,
@@ -547,6 +549,7 @@ pub struct Block {
     /// without a semicolon, if any
     pub expr: Option<P<Expr>>,
     pub id: NodeId,
+    pub hir_id: HirId,
     /// Distinguishes between `unsafe { ... }` and `{ ... }`
     pub rules: BlockCheckMode,
     pub span: Span,
@@ -560,6 +563,7 @@ pub struct Block {
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash)]
 pub struct Pat {
     pub id: NodeId,
+    pub hir_id: HirId,
     pub node: PatKind,
     pub span: Span,
 }
@@ -897,6 +901,7 @@ pub struct Local {
     /// Initializer expression to set the value, if any
     pub init: Option<P<Expr>>,
     pub id: NodeId,
+    pub hir_id: HirId,
     pub span: Span,
     pub attrs: ThinVec<Attribute>,
     pub source: LocalSource,
@@ -986,6 +991,7 @@ pub struct Expr {
     pub span: Span,
     pub node: Expr_,
     pub attrs: ThinVec<Attribute>,
+    pub hir_id: HirId,
 }
 
 impl fmt::Debug for Expr {
@@ -1423,6 +1429,7 @@ pub struct InlineAsm {
 pub struct Arg {
     pub pat: P<Pat>,
     pub id: NodeId,
+    pub hir_id: HirId,
 }
 
 /// Represents the header (not the body) of a function declaration

src/librustc/hir/mod.rs

@@ -14,7 +14,7 @@ use hir::map::DefPathHash;
 use ich::{self, CachingCodemapView};
 use session::config::DebugInfoLevel::NoDebugInfo;
 use ty;
-use util::nodemap::NodeMap;
+use util::nodemap::{NodeMap, ItemLocalMap};
 
 use std::hash as std_hash;
 use std::collections::{HashMap, HashSet, BTreeMap};
@@ -358,6 +358,18 @@ pub fn hash_stable_nodemap<'a, 'tcx, 'gcx, V, W>(
     });
 }
 
+pub fn hash_stable_itemlocalmap<'a, 'tcx, 'gcx, V, W>(
+    hcx: &mut StableHashingContext<'a, 'gcx, 'tcx>,
+    hasher: &mut StableHasher<W>,
+    map: &ItemLocalMap<V>)
+    where V: HashStable<StableHashingContext<'a, 'gcx, 'tcx>>,
+          W: StableHasherResult,
+{
+    hash_stable_hashmap(hcx, hasher, map, |_, local_id| {
+        *local_id
+    });
+}
+
 
 pub fn hash_stable_btreemap<'a, 'tcx, 'gcx, K, V, SK, F, W>(
     hcx: &mut StableHashingContext<'a, 'gcx, 'tcx>,

src/librustc/ich/hcx.rs

@@ -359,6 +359,7 @@ impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for hir::B
             ref stmts,
             ref expr,
             id,
+            hir_id: _,
             rules,
             span,
             targeted_by_break,
@@ -423,6 +424,7 @@ impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for hir::P
 
         let hir::Pat {
             id,
+            hir_id: _,
             ref node,
             ref span
         } = *self;
@@ -504,6 +506,7 @@ impl_stable_hash_for!(struct hir::Local {
     ty,
     init,
     id,
+    hir_id,
     span,
     attrs,
     source
@@ -551,6 +554,7 @@ impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for hir::E
         hcx.while_hashing_hir_bodies(true, |hcx| {
             let hir::Expr {
                 id,
+                hir_id: _,
                 ref span,
                 ref node,
                 ref attrs
@@ -1021,7 +1025,8 @@ impl_stable_hash_for!(enum hir::Stmt_ {
 
 impl_stable_hash_for!(struct hir::Arg {
     pat,
-    id
+    id,
+    hir_id
 });
 
 impl_stable_hash_for!(struct hir::Body {

src/librustc/ich/impls_hir.rs

@@ -11,7 +11,7 @@
 //! This module contains `HashStable` implementations for various data types
 //! from rustc::ty in no particular order.
 
-use ich::{self, StableHashingContext, NodeIdHashingMode};
+use ich::StableHashingContext;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher,
                                            StableHasherResult};
 use std::hash as std_hash;
@@ -611,64 +611,6 @@ impl_stable_hash_for!(struct ty::ExistentialProjection<'tcx> {
     ty
 });
 
-
-impl<'a, 'gcx, 'tcx> HashStable<StableHashingContext<'a, 'gcx, 'tcx>>
-for ty::TypeckTables<'gcx> {
-    fn hash_stable<W: StableHasherResult>(&self,
-                                          hcx: &mut StableHashingContext<'a, 'gcx, 'tcx>,
-                                          hasher: &mut StableHasher<W>) {
-        let ty::TypeckTables {
-            ref type_dependent_defs,
-            ref node_types,
-            ref node_substs,
-            ref adjustments,
-            ref pat_binding_modes,
-            ref upvar_capture_map,
-            ref closure_tys,
-            ref closure_kinds,
-            ref liberated_fn_sigs,
-            ref fru_field_types,
-
-            ref cast_kinds,
-
-            ref used_trait_imports,
-            tainted_by_errors,
-            ref free_region_map,
-        } = *self;
-
-        hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
-            ich::hash_stable_nodemap(hcx, hasher, type_dependent_defs);
-            ich::hash_stable_nodemap(hcx, hasher, node_types);
-            ich::hash_stable_nodemap(hcx, hasher, node_substs);
-            ich::hash_stable_nodemap(hcx, hasher, adjustments);
-            ich::hash_stable_nodemap(hcx, hasher, pat_binding_modes);
-            ich::hash_stable_hashmap(hcx, hasher, upvar_capture_map, |hcx, up_var_id| {
-                let ty::UpvarId {
-                    var_id,
-                    closure_expr_id
-                } = *up_var_id;
-
-                let var_def_id = hcx.tcx().hir.local_def_id(var_id);
-                let closure_def_id = hcx.tcx().hir.local_def_id(closure_expr_id);
-                (hcx.def_path_hash(var_def_id), hcx.def_path_hash(closure_def_id))
-            });
-
-            ich::hash_stable_nodemap(hcx, hasher, closure_tys);
-            ich::hash_stable_nodemap(hcx, hasher, closure_kinds);
-            ich::hash_stable_nodemap(hcx, hasher, liberated_fn_sigs);
-            ich::hash_stable_nodemap(hcx, hasher, fru_field_types);
-            ich::hash_stable_nodemap(hcx, hasher, cast_kinds);
-
-            ich::hash_stable_hashset(hcx, hasher, used_trait_imports, |hcx, def_id| {
-                hcx.def_path_hash(*def_id)
-            });
-
-            tainted_by_errors.hash_stable(hcx, hasher);
-            free_region_map.hash_stable(hcx, hasher);
-        })
-    }
-}
-
 impl_stable_hash_for!(enum ty::fast_reject::SimplifiedType {
     BoolSimplifiedType,
     CharSimplifiedType,

src/librustc/ich/impls_ty.rs

@@ -14,7 +14,7 @@ pub use self::fingerprint::Fingerprint;
 pub use self::caching_codemap_view::CachingCodemapView;
 pub use self::hcx::{StableHashingContext, NodeIdHashingMode, hash_stable_hashmap,
                     hash_stable_hashset, hash_stable_nodemap,
-                    hash_stable_btreemap};
+                    hash_stable_btreemap, hash_stable_itemlocalmap};
 mod fingerprint;
 mod caching_codemap_view;
 mod hcx;

src/librustc/ich/mod.rs

@@ -913,7 +913,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
             }
             infer::UpvarRegion(ref upvar_id, _) => {
                 format!(" for capture of `{}` by closure",
-                        self.tcx.local_var_name_str(upvar_id.var_id).to_string())
+                        self.tcx.local_var_name_str_def_index(upvar_id.var_id))
             }
         };
 

src/librustc/infer/error_reporting/mod.rs

@@ -8,13 +8,11 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use hir::{self, Local, Pat, Body};
+use hir::{self, Local, Pat, Body, HirId};
 use hir::intravisit::{self, Visitor, NestedVisitorMap};
 use infer::InferCtxt;
 use infer::type_variable::TypeVariableOrigin;
 use ty::{self, Ty, TyInfer, TyVar};
-
-use syntax::ast::NodeId;
 use syntax_pos::Span;
 
 struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
@@ -26,7 +24,7 @@ struct FindLocalByTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
 }
 
 impl<'a, 'gcx, 'tcx> FindLocalByTypeVisitor<'a, 'gcx, 'tcx> {
-    fn node_matches_type(&mut self, node_id: NodeId) -> bool {
+    fn node_matches_type(&mut self, node_id: HirId) -> bool {
         let ty_opt = self.infcx.in_progress_tables.and_then(|tables| {
             tables.borrow().node_id_to_type_opt(node_id)
         });
@@ -56,15 +54,15 @@ impl<'a, 'gcx, 'tcx> Visitor<'gcx> for FindLocalByTypeVisitor<'a, 'gcx, 'tcx> {
     }
 
     fn visit_local(&mut self, local: &'gcx Local) {
-        if self.found_local_pattern.is_none() && self.node_matches_type(local.id) {
+        if self.found_local_pattern.is_none() && self.node_matches_type(local.hir_id) {
             self.found_local_pattern = Some(&*local.pat);
         }
         intravisit::walk_local(self, local);
     }
 
     fn visit_body(&mut self, body: &'gcx Body) {
         for argument in &body.arguments {
-            if self.found_arg_pattern.is_none() && self.node_matches_type(argument.id) {
+            if self.found_arg_pattern.is_none() && self.node_matches_type(argument.hir_id) {
                 self.found_arg_pattern = Some(&*argument.pat);
             }
         }

src/librustc/infer/error_reporting/need_type_info.rs

@@ -45,8 +45,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                 err.span_note(span,
                               &format!("...so that closure can access `{}`",
                                        self.tcx
-                                           .local_var_name_str(upvar_id.var_id)
-                                           .to_string()));
+                                           .local_var_name_str_def_index(upvar_id.var_id)));
             }
             infer::InfStackClosure(span) => {
                 err.span_note(span, "...so that closure does not outlive its stack frame");
@@ -176,18 +175,19 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                                E0313,
                                                "lifetime of borrowed pointer outlives lifetime \
                                                 of captured variable `{}`...",
-                                               self.tcx.local_var_name_str(upvar_id.var_id));
+                                               self.tcx
+                                                   .local_var_name_str_def_index(upvar_id.var_id));
                 self.tcx.note_and_explain_region(&mut err,
                                                  "...the borrowed pointer is valid for ",
                                                  sub,
                                                  "...");
                 self.tcx
-                    .note_and_explain_region(&mut err,
-                                             &format!("...but `{}` is only valid for ",
-                                                      self.tcx
-                                                          .local_var_name_str(upvar_id.var_id)),
-                                             sup,
-                                             "");
+                    .note_and_explain_region(
+                      &mut err,
+                      &format!("...but `{}` is only valid for ",
+                               self.tcx.local_var_name_str_def_index(upvar_id.var_id)),
+                      sup,
+                      "");
                 err
             }
             infer::InfStackClosure(span) => {