Introduce HirId, a replacement for NodeId after lowering to HIR.

HirId has a more stable representation than NodeId, meaning that
modifications to one item don't influence (part of) the IDs within
other items. The other part is a DefIndex for which there already
is a way of stable hashing and persistence.

This commit introduces the HirId type and generates a HirId for
every NodeId during HIR lowering, but the resulting values are
not yet used anywhere, except in consistency checks.

src/librustc/hir/map/definitions.rs

@@ -14,8 +14,10 @@
 //! There are also some rather random cases (like const initializer
 //! expressions) that are mostly just leftovers.
 
+use hir;
 use hir::def_id::{CrateNum, DefId, DefIndex, LOCAL_CRATE};
 use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::indexed_vec::IndexVec;
 use rustc_data_structures::stable_hasher::StableHasher;
 use serialize::{Encodable, Decodable, Encoder, Decoder};
 use std::fmt::Write;
@@ -121,6 +123,7 @@ pub struct Definitions {
     table: DefPathTable,
     node_to_def_index: NodeMap<DefIndex>,
     def_index_to_node: Vec<ast::NodeId>,
+    pub(super) node_to_hir_id: IndexVec<ast::NodeId, hir::HirId>,
 }
 
 /// A unique identifier that we can use to lookup a definition
@@ -206,6 +209,23 @@ impl DefPath {
         s
     }
 
+    /// Returns a string representation of the DefPath without
+    /// the crate-prefix. This method is useful if you don't have
+    /// a TyCtxt available.
+    pub fn to_string_no_crate(&self) -> String {
+        let mut s = String::with_capacity(self.data.len() * 16);
+
+        for component in &self.data {
+            write!(s,
+                   "::{}[{}]",
+                   component.data.as_interned_str(),
+                   component.disambiguator)
+                .unwrap();
+        }
+
+        s
+    }
+
     pub fn deterministic_hash(&self, tcx: TyCtxt) -> u64 {
         debug!("deterministic_hash({:?})", self);
         let mut state = StableHasher::new();
@@ -275,6 +295,7 @@ impl Definitions {
             },
             node_to_def_index: NodeMap(),
             def_index_to_node: vec![],
+            node_to_hir_id: IndexVec::new(),
         }
     }
 
@@ -367,6 +388,15 @@ impl Definitions {
 
         index
     }
+
+    /// Initialize the ast::NodeId to HirId mapping once it has been generated during
+    /// AST to HIR lowering.
+    pub fn init_node_id_to_hir_id_mapping(&mut self,
+                                          mapping: IndexVec<ast::NodeId, hir::HirId>) {
+        assert!(self.node_to_hir_id.is_empty(),
+                "Trying initialize NodeId -> HirId mapping twice");
+        self.node_to_hir_id = mapping;
+    }
 }
 
 impl DefPathData {

src/librustc/hir/map/hir_id_validator.rs

@@ -0,0 +1,184 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use hir::def_id::{DefId, DefIndex, CRATE_DEF_INDEX};
+use hir::{self, intravisit, HirId, ItemLocalId};
+use syntax::ast::NodeId;
+use hir::itemlikevisit::ItemLikeVisitor;
+use rustc_data_structures::fx::FxHashMap;
+
+pub fn check_crate<'hir>(hir_map: &hir::map::Map<'hir>) {
+    let mut outer_visitor = OuterVisitor {
+        hir_map: hir_map,
+        errors: vec![],
+    };
+
+    hir_map.dep_graph.with_ignore(|| {
+        hir_map.krate().visit_all_item_likes(&mut outer_visitor);
+        if !outer_visitor.errors.is_empty() {
+            let message = outer_visitor
+                .errors
+                .iter()
+                .fold(String::new(), |s1, s2| s1 + "\n" + s2);
+            bug!("{}", message);
+        }
+    });
+}
+
+struct HirIdValidator<'a, 'hir: 'a> {
+    hir_map: &'a hir::map::Map<'hir>,
+    owner_def_index: Option<DefIndex>,
+    hir_ids_seen: FxHashMap<ItemLocalId, NodeId>,
+    errors: Vec<String>,
+}
+
+struct OuterVisitor<'a, 'hir: 'a> {
+    hir_map: &'a hir::map::Map<'hir>,
+    errors: Vec<String>,
+}
+
+impl<'a, 'hir: 'a> OuterVisitor<'a, 'hir> {
+    fn new_inner_visitor(&self,
+                         hir_map: &'a hir::map::Map<'hir>)
+                         -> HirIdValidator<'a, 'hir> {
+        HirIdValidator {
+            hir_map: hir_map,
+            owner_def_index: None,
+            hir_ids_seen: FxHashMap(),
+            errors: Vec::new(),
+        }
+    }
+}
+
+impl<'a, 'hir: 'a> ItemLikeVisitor<'hir> for OuterVisitor<'a, 'hir> {
+    fn visit_item(&mut self, i: &'hir hir::Item) {
+        let mut inner_visitor = self.new_inner_visitor(self.hir_map);
+        inner_visitor.check(i.id, |this| intravisit::walk_item(this, i));
+        self.errors.extend(inner_visitor.errors.drain(..));
+    }
+
+    fn visit_trait_item(&mut self, i: &'hir hir::TraitItem) {
+        let mut inner_visitor = self.new_inner_visitor(self.hir_map);
+        inner_visitor.check(i.id, |this| intravisit::walk_trait_item(this, i));
+        self.errors.extend(inner_visitor.errors.drain(..));
+    }
+
+    fn visit_impl_item(&mut self, i: &'hir hir::ImplItem) {
+        let mut inner_visitor = self.new_inner_visitor(self.hir_map);
+        inner_visitor.check(i.id, |this| intravisit::walk_impl_item(this, i));
+        self.errors.extend(inner_visitor.errors.drain(..));
+    }
+}
+
+impl<'a, 'hir: 'a> HirIdValidator<'a, 'hir> {
+
+    fn check<F: FnOnce(&mut HirIdValidator<'a, 'hir>)>(&mut self,
+                                                       node_id: NodeId,
+                                                       walk: F) {
+        assert!(self.owner_def_index.is_none());
+        let owner_def_index = self.hir_map.local_def_id(node_id).index;
+        self.owner_def_index = Some(owner_def_index);
+        walk(self);
+
+        if owner_def_index == CRATE_DEF_INDEX {
+            return
+        }
+
+        // There's always at least one entry for the owning item itself
+        let max = self.hir_ids_seen
+                      .keys()
+                      .map(|local_id| local_id.as_usize())
+                      .max()
+                      .unwrap();
+
+        if max != self.hir_ids_seen.len() - 1 {
+            // Collect the missing ItemLocalIds
+            let missing: Vec<_> = (0 .. max + 1)
+              .filter(|&i| !self.hir_ids_seen.contains_key(&ItemLocalId(i as u32)))
+              .collect();
+
+            // Try to map those to something more useful
+            let mut missing_items = vec![];
+
+            for local_id in missing {
+                let hir_id = HirId {
+                    owner: owner_def_index,
+                    local_id: ItemLocalId(local_id as u32),
+                };
+
+                // We are already in ICE mode here, so doing a linear search
+                // should be fine.
+                let (node_id, _) = self.hir_map
+                                       .definitions()
+                                       .node_to_hir_id
+                                       .iter()
+                                       .enumerate()
+                                       .find(|&(_, &entry)| hir_id == entry)
+                                       .unwrap();
+                let node_id = NodeId::new(node_id);
+                missing_items.push(format!("[local_id: {}, node:{}]",
+                                           local_id,
+                                           self.hir_map.node_to_string(node_id)));
+            }
+
+            self.errors.push(format!(
+                "ItemLocalIds not assigned densely in {}. \
+                Max ItemLocalId = {}, missing IDs = {:?}",
+                self.hir_map.def_path(DefId::local(owner_def_index)).to_string_no_crate(),
+                max,
+                missing_items));
+        }
+    }
+}
+
+impl<'a, 'hir: 'a> intravisit::Visitor<'hir> for HirIdValidator<'a, 'hir> {
+
+    fn nested_visit_map<'this>(&'this mut self)
+                               -> intravisit::NestedVisitorMap<'this, 'hir> {
+        intravisit::NestedVisitorMap::OnlyBodies(self.hir_map)
+    }
+
+    fn visit_id(&mut self, node_id: NodeId) {
+        let owner = self.owner_def_index.unwrap();
+        let stable_id = self.hir_map.definitions().node_to_hir_id[node_id];
+
+        if stable_id == hir::DUMMY_HIR_ID {
+            self.errors.push(format!("HirIdValidator: No HirId assigned for NodeId {}: {:?}",
+                                     node_id,
+                                     self.hir_map.node_to_string(node_id)));
+        }
+
+        if owner != stable_id.owner {
+            self.errors.push(format!(
+                "HirIdValidator: The recorded owner of {} is {} instead of {}",
+                self.hir_map.node_to_string(node_id),
+                self.hir_map.def_path(DefId::local(stable_id.owner)).to_string_no_crate(),
+                self.hir_map.def_path(DefId::local(owner)).to_string_no_crate()));
+        }
+
+        if let Some(prev) = self.hir_ids_seen.insert(stable_id.local_id, node_id) {
+            if prev != node_id {
+                self.errors.push(format!(
+                    "HirIdValidator: Same HirId {}/{} assigned for nodes {} and {}",
+                    self.hir_map.def_path(DefId::local(stable_id.owner)).to_string_no_crate(),
+                    stable_id.local_id.as_usize(),
+                    self.hir_map.node_to_string(prev),
+                    self.hir_map.node_to_string(node_id)));
+            }
+        }
+    }
+
+    fn visit_impl_item_ref(&mut self, _: &'hir hir::ImplItemRef) {
+        // Explicitly do nothing here. ImplItemRefs contain hir::Visibility
+        // values that actually belong to an ImplItem instead of the ItemImpl
+        // we are currently in. So for those it's correct that they have a
+        // different owner.
+    }
+}

src/librustc/hir/map/mod.rs

@@ -36,6 +36,7 @@ pub mod blocks;
 mod collector;
 mod def_collector;
 pub mod definitions;
+mod hir_id_validator;
 
 #[derive(Copy, Clone, Debug)]
 pub enum Node<'hir> {
@@ -964,13 +965,17 @@ pub fn map_crate<'hir>(forest: &'hir mut Forest,
               entries, vector_length, (entries as f64 / vector_length as f64) * 100.);
     }
 
-    Map {
+    let map = Map {
         forest: forest,
         dep_graph: forest.dep_graph.clone(),
         map: map,
         definitions: definitions,
         inlined_bodies: RefCell::new(DefIdMap()),
-    }
+    };
+
+    hir_id_validator::check_crate(&map);
+
+    map
 }
 
 /// Identical to the `PpAnn` implementation for `hir::Crate`,

src/librustc/hir/mod.rs

@@ -30,7 +30,7 @@ pub use self::Visibility::{Public, Inherited};
 pub use self::PathParameters::*;
 
 use hir::def::Def;
-use hir::def_id::DefId;
+use hir::def_id::{DefId, DefIndex, CRATE_DEF_INDEX};
 use util::nodemap::{NodeMap, FxHashSet};
 
 use syntax_pos::{Span, ExpnId, DUMMY_SP};
@@ -43,6 +43,8 @@ use syntax::symbol::{Symbol, keywords};
 use syntax::tokenstream::TokenStream;
 use syntax::util::ThinVec;
 
+use rustc_data_structures::indexed_vec;
+
 use std::collections::BTreeMap;
 use std::fmt;
 
@@ -73,6 +75,63 @@ pub mod pat_util;
 pub mod print;
 pub mod svh;
 
+/// A HirId uniquely identifies a node in the HIR of then current crate. It is
+/// composed of the `owner`, which is the DefIndex of the directly enclosing
+/// hir::Item, hir::TraitItem, or hir::ImplItem (i.e. the closest "item-like"),
+/// and the `local_id` which is unique within the given owner.
+///
+/// This two-level structure makes for more stable values: One can move an item
+/// around within the source code, or add or remove stuff before it, without
+/// the local_id part of the HirId changing, which is a very useful property
+/// incremental compilation where we have to persist things through changes to
+/// the code base.
+#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Debug,
+         RustcEncodable, RustcDecodable)]
+pub struct HirId {
+    pub owner: DefIndex,
+    pub local_id: ItemLocalId,
+}
+
+/// An `ItemLocalId` uniquely identifies something within a given "item-like",
+/// that is within a hir::Item, hir::TraitItem, or hir::ImplItem. There is no
+/// guarantee that the numerical value of a given `ItemLocalId` corresponds to
+/// the node's position within the owning item in any way, but there is a
+/// guarantee that the `LocalItemId`s within an owner occupy a dense range of
+/// integers starting at zero, so a mapping that maps all or most nodes within
+/// an "item-like" to something else can be implement by a `Vec` instead of a
+/// tree or hash map.
+#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Debug,
+         RustcEncodable, RustcDecodable)]
+pub struct ItemLocalId(pub u32);
+
+impl ItemLocalId {
+    pub fn as_usize(&self) -> usize {
+        self.0 as usize
+    }
+}
+
+impl indexed_vec::Idx for ItemLocalId {
+    fn new(idx: usize) -> Self {
+        debug_assert!((idx as u32) as usize == idx);
+        ItemLocalId(idx as u32)
+    }
+
+    fn index(self) -> usize {
+        self.0 as usize
+    }
+}
+
+/// The `HirId` corresponding to CRATE_NODE_ID and CRATE_DEF_INDEX
+pub const CRATE_HIR_ID: HirId = HirId {
+    owner: CRATE_DEF_INDEX,
+    local_id: ItemLocalId(0)
+};
+
+pub const DUMMY_HIR_ID: HirId = HirId {
+    owner: CRATE_DEF_INDEX,
+    local_id: ItemLocalId(!0)
+};
+
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Copy)]
 pub struct Lifetime {
     pub id: NodeId,

src/libsyntax/ext/placeholders.rs

@@ -178,17 +178,9 @@ impl<'a, 'b> Folder for PlaceholderExpander<'a, 'b> {
             block.stmts = block.stmts.move_flat_map(|mut stmt| {
                 remaining_stmts -= 1;
 
-                match stmt.node {
-                    // Avoid wasting a node id on a trailing expression statement,
-                    // which shares a HIR node with the expression itself.
-                    ast::StmtKind::Expr(ref expr) if remaining_stmts == 0 => stmt.id = expr.id,
-
-                    _ if self.monotonic => {
-                        assert_eq!(stmt.id, ast::DUMMY_NODE_ID);
-                        stmt.id = self.cx.resolver.next_node_id();
-                    }
-
-                    _ => {}
+                if self.monotonic {
+                    assert_eq!(stmt.id, ast::DUMMY_NODE_ID);
+                    stmt.id = self.cx.resolver.next_node_id();
                 }
 
                 Some(stmt)

src/test/compile-fail/region-bounds-on-objects-and-type-parameters.rs

@@ -18,7 +18,7 @@ trait SomeTrait { }
 
 // Bounds on object types:
 
-struct Foo<'a,'b,'c> { //~ ERROR parameter `'b` is never used
+struct Foo<'a,'b,'c> { //~ ERROR parameter `'c` is never used
     // All of these are ok, because we can derive exactly one bound:
     a: Box<IsStatic>,
     b: Box<Is<'static>>,