Auto merge of #59042 - ljedrz:HirIdification_rework_map, r=Zoxc

HirIdification: rework Map

The next iteration of HirIdification (#57578).

- remove `NodeId` from `Entry`
- change `Map::map` to an `FxHashMap<HirId, Entry>`
- base the `NodeId` `Map` methods on `HirId` ones (reverses the current state)
- HirIdify `librustdoc` a little bit (some `NodeId` `Map` methods were converted to work on `HirId`s)

The second change might have performance implications, so I'd do a perf run to be sure it's fine; it simplifies the codebase and shouldn't have an impact as long as the `Map` searches are cached (which is now possible thanks to using `HirId`s).

r? @Zoxc

src/librustc/hir/map/collector.rs

@@ -8,8 +8,8 @@ use crate::ich::Fingerprint;
 use crate::middle::cstore::CrateStore;
 use crate::session::CrateDisambiguator;
 use crate::session::Session;
-use std::iter::repeat;
+use crate::util::nodemap::FxHashMap;
-use syntax::ast::{NodeId, CRATE_NODE_ID};
+use syntax::ast::NodeId;
 use syntax::source_map::SourceMap;
 use syntax_pos::Span;
 
@@ -25,7 +25,7 @@ pub(super) struct NodeCollector<'a, 'hir> {
     source_map: &'a SourceMap,
 
     /// The node map
-    map: Vec<Option<Entry<'hir>>>,
+    map: FxHashMap<HirId, Entry<'hir>>,
     /// The parent of this node
     parent_node: hir::HirId,
 
@@ -145,7 +145,8 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
         let mut collector = NodeCollector {
             krate,
             source_map: sess.source_map(),
-            map: repeat(None).take(sess.current_node_id_count()).collect(),
+            map: FxHashMap::with_capacity_and_hasher(sess.current_node_id_count(),
+                Default::default()),
             parent_node: hir::CRATE_HIR_ID,
             current_signature_dep_index: root_mod_sig_dep_index,
             current_full_dep_index: root_mod_full_dep_index,
@@ -157,9 +158,8 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
             hcx,
             hir_body_nodes,
         };
-        collector.insert_entry(CRATE_NODE_ID, Entry {
+        collector.insert_entry(hir::CRATE_HIR_ID, Entry {
-            parent: CRATE_NODE_ID,
+            parent: hir::CRATE_HIR_ID,
-            parent_hir: hir::CRATE_HIR_ID,
             dep_node: root_mod_sig_dep_index,
             node: Node::Crate,
         });
@@ -171,7 +171,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
                                                   crate_disambiguator: CrateDisambiguator,
                                                   cstore: &dyn CrateStore,
                                                   commandline_args_hash: u64)
-                                                  -> (Vec<Option<Entry<'hir>>>, Svh)
+                                                  -> (FxHashMap<HirId, Entry<'hir>>, Svh)
     {
         self.hir_body_nodes.sort_unstable_by_key(|bn| bn.0);
 
@@ -222,15 +222,14 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
         (self.map, svh)
     }
 
-    fn insert_entry(&mut self, id: NodeId, entry: Entry<'hir>) {
+    fn insert_entry(&mut self, id: HirId, entry: Entry<'hir>) {
         debug!("hir_map: {:?} => {:?}", id, entry);
-        self.map[id.as_usize()] = Some(entry);
+        self.map.insert(id, entry);
     }
 
     fn insert(&mut self, span: Span, hir_id: HirId, node: Node<'hir>) {
         let entry = Entry {
-            parent: self.hir_to_node_id[&self.parent_node],
+            parent: self.parent_node,
-            parent_hir: self.parent_node,
             dep_node: if self.currently_in_body {
                 self.current_full_dep_index
             } else {
@@ -239,12 +238,11 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
             node,
         };
 
-        let node_id = self.hir_to_node_id[&hir_id];
-
         // Make sure that the DepNode of some node coincides with the HirId
         // owner of that node.
         if cfg!(debug_assertions) {
-           assert_eq!(self.definitions.node_to_hir_id(node_id), hir_id);
+            let node_id = self.hir_to_node_id[&hir_id];
+            assert_eq!(self.definitions.node_to_hir_id(node_id), hir_id);
 
             if hir_id.owner != self.current_dep_node_owner {
                 let node_str = match self.definitions.opt_def_index(node_id) {
@@ -277,7 +275,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
             }
         }
 
-        self.insert_entry(node_id, entry);
+        self.insert_entry(hir_id, entry);
     }
 
     fn with_parent<F: FnOnce(&mut Self)>(

src/librustc/hir/map/mod.rs

@@ -11,7 +11,7 @@ use crate::middle::cstore::CrateStoreDyn;
 
 use rustc_target::spec::abi::Abi;
 use rustc_data_structures::svh::Svh;
-use syntax::ast::{self, Name, NodeId, CRATE_NODE_ID};
+use syntax::ast::{self, Name, NodeId};
 use syntax::source_map::Spanned;
 use syntax::ext::base::MacroKind;
 use syntax_pos::{Span, DUMMY_SP};
@@ -38,14 +38,13 @@ pub const REGULAR_SPACE: DefIndexAddressSpace = DefIndexAddressSpace::High;
 /// Represents an entry and its parent `NodeId`.
 #[derive(Copy, Clone, Debug)]
 pub struct Entry<'hir> {
-    parent: NodeId,
+    parent: HirId,
-    parent_hir: HirId,
     dep_node: DepNodeIndex,
     node: Node<'hir>,
 }
 
 impl<'hir> Entry<'hir> {
-    fn parent_node(self) -> Option<NodeId> {
+    fn parent_node(self) -> Option<HirId> {
         match self.node {
             Node::Crate | Node::MacroDef(_) => None,
             _ => Some(self.parent),
@@ -183,7 +182,7 @@ pub struct Map<'hir> {
     ///
     /// Also, indexing is pretty quick when you've got a vector and
     /// plain old integers.
-    map: Vec<Option<Entry<'hir>>>,
+    map: FxHashMap<HirId, Entry<'hir>>,
 
     definitions: &'hir Definitions,
 
@@ -200,17 +199,17 @@ impl<'hir> Map<'hir> {
     /// read recorded). If the function just returns a DefId or
     /// NodeId, no actual content was returned, so no read is needed.
     pub fn read(&self, id: NodeId) {
-        if let Some(entry) = self.map[id.as_usize()] {
+        let hir_id = self.node_to_hir_id(id);
-            self.dep_graph.read_index(entry.dep_node);
+        self.read_by_hir_id(hir_id);
-        } else {
-            bug!("called `HirMap::read()` with invalid `NodeId`: {:?}", id)
-        }
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn read_by_hir_id(&self, hir_id: HirId) {
-        let node_id = self.hir_to_node_id(hir_id);
+        if let Some(entry) = self.map.get(&hir_id) {
-        self.read(node_id);
+            self.dep_graph.read_index(entry.dep_node);
+        } else {
+            bug!("called `HirMap::read()` with invalid `HirId`: {:?}", hir_id)
+        }
     }
 
     #[inline]
@@ -242,18 +241,18 @@ impl<'hir> Map<'hir> {
     #[inline]
     pub fn local_def_id(&self, node: NodeId) -> DefId {
         self.opt_local_def_id(node).unwrap_or_else(|| {
+            let hir_id = self.node_to_hir_id(node);
             bug!("local_def_id: no entry for `{}`, which has a map of `{:?}`",
-                 node, self.find_entry(node))
+                 node, self.find_entry(hir_id))
         })
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     #[inline]
     pub fn local_def_id_from_hir_id(&self, hir_id: HirId) -> DefId {
-        let node_id = self.hir_to_node_id(hir_id);
+        self.opt_local_def_id_from_hir_id(hir_id).unwrap_or_else(|| {
-        self.opt_local_def_id(node_id).unwrap_or_else(|| {
             bug!("local_def_id_from_hir_id: no entry for `{:?}`, which has a map of `{:?}`",
-                 hir_id, self.find_entry(node_id))
+                 hir_id, self.find_entry(hir_id))
         })
     }
 
@@ -423,8 +422,8 @@ impl<'hir> Map<'hir> {
         self.map.len()
     }
 
-    fn find_entry(&self, id: NodeId) -> Option<Entry<'hir>> {
+    fn find_entry(&self, id: HirId) -> Option<Entry<'hir>> {
-        self.map.get(id.as_usize()).cloned().unwrap_or(None)
+        self.map.get(&id).cloned()
     }
 
     pub fn krate(&self) -> &'hir Crate {
@@ -456,27 +455,26 @@ impl<'hir> Map<'hir> {
     }
 
     pub fn fn_decl(&self, node_id: ast::NodeId) -> Option<FnDecl> {
-        if let Some(entry) = self.find_entry(node_id) {
+        let hir_id = self.node_to_hir_id(node_id);
-            entry.fn_decl().cloned()
+        self.fn_decl_by_hir_id(hir_id)
-        } else {
-            bug!("no entry for node_id `{}`", node_id)
-        }
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn fn_decl_by_hir_id(&self, hir_id: HirId) -> Option<FnDecl> {
-        let node_id = self.hir_to_node_id(hir_id);
+        if let Some(entry) = self.find_entry(hir_id) {
-        self.fn_decl(node_id)
+            entry.fn_decl().cloned()
+        } else {
+            bug!("no entry for hir_id `{}`", hir_id)
+        }
     }
 
     /// Returns the `NodeId` that corresponds to the definition of
     /// which this is the body of, i.e., a `fn`, `const` or `static`
     /// item (possibly associated), a closure, or a `hir::AnonConst`.
     pub fn body_owner(&self, BodyId { hir_id }: BodyId) -> NodeId {
-        let node_id = self.hir_to_node_id(hir_id);
+        let parent = self.get_parent_node_by_hir_id(hir_id);
-        let parent = self.get_parent_node(node_id);
+        assert!(self.map.get(&parent).map_or(false, |e| e.is_body_owner(hir_id)));
-        assert!(self.map[parent.as_usize()].map_or(false, |e| e.is_body_owner(hir_id)));
+        self.hir_to_node_id(parent)
-        parent
     }
 
     pub fn body_owner_def_id(&self, id: BodyId) -> DefId {
@@ -486,25 +484,25 @@ impl<'hir> Map<'hir> {
     /// Given a `NodeId`, returns the `BodyId` associated with it,
     /// if the node is a body owner, otherwise returns `None`.
     pub fn maybe_body_owned_by(&self, id: NodeId) -> Option<BodyId> {
-        if let Some(entry) = self.find_entry(id) {
+        let hir_id = self.node_to_hir_id(id);
+        self.maybe_body_owned_by_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn maybe_body_owned_by_by_hir_id(&self, hir_id: HirId) -> Option<BodyId> {
+        if let Some(entry) = self.find_entry(hir_id) {
             if self.dep_graph.is_fully_enabled() {
-                let hir_id_owner = self.node_to_hir_id(id).owner;
+                let hir_id_owner = hir_id.owner;
                 let def_path_hash = self.definitions.def_path_hash(hir_id_owner);
                 self.dep_graph.read(def_path_hash.to_dep_node(DepKind::HirBody));
             }
 
             entry.associated_body()
         } else {
-            bug!("no entry for id `{}`", id)
+            bug!("no entry for id `{}`", hir_id)
         }
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn maybe_body_owned_by_by_hir_id(&self, id: HirId) -> Option<BodyId> {
-        let node_id = self.hir_to_node_id(id);
-        self.maybe_body_owned_by(node_id)
-    }
-
     /// Given a body owner's id, returns the `BodyId` associated with it.
     pub fn body_owned_by(&self, id: HirId) -> BodyId {
         self.maybe_body_owned_by_by_hir_id(id).unwrap_or_else(|| {
@@ -514,7 +512,13 @@ impl<'hir> Map<'hir> {
     }
 
     pub fn body_owner_kind(&self, id: NodeId) -> BodyOwnerKind {
-        match self.get(id) {
+        let hir_id = self.node_to_hir_id(id);
+        self.body_owner_kind_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn body_owner_kind_by_hir_id(&self, id: HirId) -> BodyOwnerKind {
+        match self.get_by_hir_id(id) {
             Node::Item(&Item { node: ItemKind::Const(..), .. }) |
             Node::TraitItem(&TraitItem { node: TraitItemKind::Const(..), .. }) |
             Node::ImplItem(&ImplItem { node: ImplItemKind::Const(..), .. }) |
@@ -537,12 +541,6 @@ impl<'hir> Map<'hir> {
         }
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn body_owner_kind_by_hir_id(&self, id: HirId) -> BodyOwnerKind {
-        let node_id = self.hir_to_node_id(id);
-        self.body_owner_kind(node_id)
-    }
-
     pub fn ty_param_owner(&self, id: HirId) -> HirId {
         match self.get_by_hir_id(id) {
             Node::Item(&Item { node: ItemKind::Trait(..), .. }) |
@@ -579,11 +577,11 @@ impl<'hir> Map<'hir> {
         &self.forest.krate.attrs
     }
 
-    pub fn get_module(&self, module: DefId) -> (&'hir Mod, Span, HirId) {
+    pub fn get_module(&self, module: DefId) -> (&'hir Mod, Span, HirId)
-        let node_id = self.as_local_node_id(module).unwrap();
+    {
-        let hir_id = self.node_to_hir_id(node_id);
+        let hir_id = self.as_local_hir_id(module).unwrap();
-        self.read(node_id);
+        self.read_by_hir_id(hir_id);
-        match self.find_entry(node_id).unwrap().node {
+        match self.find_entry(hir_id).unwrap().node {
             Node::Item(&Item {
                 span,
                 node: ItemKind::Mod(ref m),
@@ -623,14 +621,15 @@ impl<'hir> Map<'hir> {
     /// Retrieve the Node corresponding to `id`, panicking if it cannot
     /// be found.
     pub fn get(&self, id: NodeId) -> Node<'hir> {
-        // read recorded by `find`
+        let hir_id = self.node_to_hir_id(id);
-        self.find(id).unwrap_or_else(|| bug!("couldn't find node id {} in the AST map", id))
+        self.get_by_hir_id(hir_id)
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn get_by_hir_id(&self, id: HirId) -> Node<'hir> {
-        let node_id = self.hir_to_node_id(id);
+        // read recorded by `find`
-        self.get(node_id)
+        self.find_by_hir_id(id).unwrap_or_else(||
+            bug!("couldn't find hir id {} in the HIR map", id))
     }
 
     pub fn get_if_local(&self, id: DefId) -> Option<Node<'hir>> {
@@ -666,7 +665,13 @@ impl<'hir> Map<'hir> {
 
     /// Retrieves the `Node` corresponding to `id`, returning `None` if cannot be found.
     pub fn find(&self, id: NodeId) -> Option<Node<'hir>> {
-        let result = self.find_entry(id).and_then(|entry| {
+        let hir_id = self.node_to_hir_id(id);
+        self.find_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn find_by_hir_id(&self, hir_id: HirId) -> Option<Node<'hir>> {
+        let result = self.find_entry(hir_id).and_then(|entry| {
             if let Node::Crate = entry.node {
                 None
             } else {
@@ -674,17 +679,11 @@ impl<'hir> Map<'hir> {
             }
         });
         if result.is_some() {
-            self.read(id);
+            self.read_by_hir_id(hir_id);
         }
         result
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn find_by_hir_id(&self, hir_id: HirId) -> Option<Node<'hir>> {
-        let node_id = self.hir_to_node_id(hir_id);
-        self.find(node_id)
-    }
-
     /// Similar to `get_parent`; returns the parent node-id, or own `id` if there is
     /// no parent. Note that the parent may be `CRATE_NODE_ID`, which is not itself
     /// present in the map -- so passing the return value of get_parent_node to
@@ -696,20 +695,22 @@ impl<'hir> Map<'hir> {
     /// from a node to the root of the ast (unless you get the same ID back here
     /// that can happen if the ID is not in the map itself or is just weird).
     pub fn get_parent_node(&self, id: NodeId) -> NodeId {
+        let hir_id = self.node_to_hir_id(id);
+        let parent_hir_id = self.get_parent_node_by_hir_id(hir_id);
+        self.hir_to_node_id(parent_hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn get_parent_node_by_hir_id(&self, hir_id: HirId) -> HirId {
         if self.dep_graph.is_fully_enabled() {
-            let hir_id_owner = self.node_to_hir_id(id).owner;
+            let hir_id_owner = hir_id.owner;
             let def_path_hash = self.definitions.def_path_hash(hir_id_owner);
             self.dep_graph.read(def_path_hash.to_dep_node(DepKind::HirBody));
         }
 
-        self.find_entry(id).and_then(|x| x.parent_node()).unwrap_or(id)
+        self.find_entry(hir_id)
-    }
+            .and_then(|x| x.parent_node())
-
+            .unwrap_or(hir_id)
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn get_parent_node_by_hir_id(&self, id: HirId) -> HirId {
-        let node_id = self.hir_to_node_id(id);
-        let parent_node_id = self.get_parent_node(node_id);
-        self.node_to_hir_id(parent_node_id)
     }
 
     /// Check if the node is an argument. An argument is a local variable whose
@@ -739,17 +740,17 @@ impl<'hir> Map<'hir> {
     /// is not an error, since items in the crate module have the crate root as
     /// parent.
     fn walk_parent_nodes<F, F2>(&self,
-                                start_id: NodeId,
+                                start_id: HirId,
                                 found: F,
                                 bail_early: F2)
-        -> Result<NodeId, NodeId>
+        -> Result<HirId, HirId>
         where F: Fn(&Node<'hir>) -> bool, F2: Fn(&Node<'hir>) -> bool
     {
         let mut id = start_id;
         loop {
-            let parent_node = self.get_parent_node(id);
+            let parent_node = self.get_parent_node_by_hir_id(id);
-            if parent_node == CRATE_NODE_ID {
+            if parent_node == CRATE_HIR_ID {
-                return Ok(CRATE_NODE_ID);
+                return Ok(CRATE_HIR_ID);
             }
             if parent_node == id {
                 return Err(id);
@@ -816,10 +817,7 @@ impl<'hir> Map<'hir> {
             }
         };
 
-        let node_id = self.hir_to_node_id(id);
+        self.walk_parent_nodes(id, match_fn, match_non_returning_block).ok()
-        self.walk_parent_nodes(node_id, match_fn, match_non_returning_block)
-            .ok()
-            .map(|return_node_id| self.node_to_hir_id(return_node_id))
     }
 
     /// Retrieves the `NodeId` for `id`'s parent item, or `id` itself if no
@@ -827,7 +825,14 @@ impl<'hir> Map<'hir> {
     /// in the HIR which is recorded by the map and is an item, either an item
     /// in a module, trait, or impl.
     pub fn get_parent(&self, id: NodeId) -> NodeId {
-        match self.walk_parent_nodes(id, |node| match *node {
+        let hir_id = self.node_to_hir_id(id);
+        let parent_hir_id = self.get_parent_item(hir_id);
+        self.hir_to_node_id(parent_hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn get_parent_item(&self, hir_id: HirId) -> HirId {
+        match self.walk_parent_nodes(hir_id, |node| match *node {
             Node::Item(_) |
             Node::ForeignItem(_) |
             Node::TraitItem(_) |
@@ -839,29 +844,22 @@ impl<'hir> Map<'hir> {
         }
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn get_parent_item(&self, id: HirId) -> HirId {
-        let node_id = self.hir_to_node_id(id);
-        let parent_node_id = self.get_parent(node_id);
-        self.node_to_hir_id(parent_node_id)
-    }
-
     /// Returns the `DefId` of `id`'s nearest module parent, or `id` itself if no
     /// module parent is in this map.
     pub fn get_module_parent(&self, id: NodeId) -> DefId {
-        self.local_def_id(self.get_module_parent_node(id))
+        let hir_id = self.node_to_hir_id(id);
+        self.get_module_parent_by_hir_id(hir_id)
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn get_module_parent_by_hir_id(&self, id: HirId) -> DefId {
-        let node_id = self.hir_to_node_id(id);
+        self.local_def_id_from_hir_id(self.get_module_parent_node(id))
-        self.get_module_parent(node_id)
     }
 
-    /// Returns the `NodeId` of `id`'s nearest module parent, or `id` itself if no
+    /// Returns the `HirId` of `id`'s nearest module parent, or `id` itself if no
     /// module parent is in this map.
-    pub fn get_module_parent_node(&self, id: NodeId) -> NodeId {
+    pub fn get_module_parent_node(&self, hir_id: HirId) -> HirId {
-        match self.walk_parent_nodes(id, |node| match *node {
+        match self.walk_parent_nodes(hir_id, |node| match *node {
             Node::Item(&Item { node: ItemKind::Mod(_), .. }) => true,
             _ => false,
         }, |_| false) {
@@ -874,8 +872,8 @@ impl<'hir> Map<'hir> {
     /// FIXME: it is not clear to me that all items qualify as scopes -- statics
     /// and associated types probably shouldn't, for example. Behavior in this
     /// regard should be expected to be highly unstable.
-    pub fn get_enclosing_scope(&self, id: NodeId) -> Option<NodeId> {
+    pub fn get_enclosing_scope(&self, hir_id: HirId) -> Option<HirId> {
-        self.walk_parent_nodes(id, |node| match *node {
+        self.walk_parent_nodes(hir_id, |node| match *node {
             Node::Item(_) |
             Node::ForeignItem(_) |
             Node::TraitItem(_) |
@@ -886,39 +884,37 @@ impl<'hir> Map<'hir> {
     }
 
     pub fn get_parent_did(&self, id: NodeId) -> DefId {
-        self.local_def_id(self.get_parent(id))
+        let hir_id = self.node_to_hir_id(id);
+        self.get_parent_did_by_hir_id(hir_id)
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn get_parent_did_by_hir_id(&self, id: HirId) -> DefId {
-        let node_id = self.hir_to_node_id(id);
+        self.local_def_id_from_hir_id(self.get_parent_item(id))
-        self.get_parent_did(node_id)
     }
 
     pub fn get_foreign_abi(&self, id: NodeId) -> Abi {
-        let parent = self.get_parent(id);
+        let hir_id = self.node_to_hir_id(id);
+        self.get_foreign_abi_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn get_foreign_abi_by_hir_id(&self, hir_id: HirId) -> Abi {
+        let parent = self.get_parent_item(hir_id);
         if let Some(entry) = self.find_entry(parent) {
             if let Entry {
                 node: Node::Item(Item { node: ItemKind::ForeignMod(ref nm), .. }), .. } = entry
             {
-                self.read(id); // reveals some of the content of a node
+                self.read_by_hir_id(hir_id); // reveals some of the content of a node
                 return nm.abi;
             }
         }
-        bug!("expected foreign mod or inlined parent, found {}", self.node_to_string(parent))
+        bug!("expected foreign mod or inlined parent, found {}", self.hir_to_string(parent))
-    }
-
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn get_foreign_abi_by_hir_id(&self, id: HirId) -> Abi {
-        let node_id = self.hir_to_node_id(id);
-        self.get_foreign_abi(node_id)
     }
 
     pub fn expect_item(&self, id: NodeId) -> &'hir Item {
-        match self.find(id) { // read recorded by `find`
+        let hir_id = self.node_to_hir_id(id);
-            Some(Node::Item(item)) => item,
+        self.expect_item_by_hir_id(hir_id)
-            _ => bug!("expected item, found {}", self.node_to_string(id))
-        }
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
@@ -973,21 +969,27 @@ impl<'hir> Map<'hir> {
     }
 
     pub fn expect_expr(&self, id: NodeId) -> &'hir Expr {
-        match self.find(id) { // read recorded by find
+        let hir_id = self.node_to_hir_id(id);
-            Some(Node::Expr(expr)) => expr,
+        self.expect_expr_by_hir_id(hir_id)
-            _ => bug!("expected expr, found {}", self.node_to_string(id))
-        }
     }
 
     // FIXME(@ljedrz): replace the NodeId variant
     pub fn expect_expr_by_hir_id(&self, id: HirId) -> &'hir Expr {
-        let node_id = self.hir_to_node_id(id);
+        match self.find_by_hir_id(id) { // read recorded by find
-        self.expect_expr(node_id)
+            Some(Node::Expr(expr)) => expr,
+            _ => bug!("expected expr, found {}", self.hir_to_string(id))
+        }
     }
 
     /// Returns the name associated with the given NodeId's AST.
     pub fn name(&self, id: NodeId) -> Name {
-        match self.get(id) {
+        let hir_id = self.node_to_hir_id(id);
+        self.name_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn name_by_hir_id(&self, id: HirId) -> Name {
+        match self.get_by_hir_id(id) {
             Node::Item(i) => i.ident.name,
             Node::ForeignItem(fi) => fi.ident.name,
             Node::ImplItem(ii) => ii.ident.name,
@@ -997,21 +999,21 @@ impl<'hir> Map<'hir> {
             Node::Lifetime(lt) => lt.name.ident().name,
             Node::GenericParam(param) => param.name.ident().name,
             Node::Binding(&Pat { node: PatKind::Binding(_, _, l, _), .. }) => l.name,
-            Node::Ctor(..) => self.name(self.get_parent(id)),
+            Node::Ctor(..) => self.name_by_hir_id(self.get_parent_item(id)),
-            _ => bug!("no name for {}", self.node_to_string(id))
+            _ => bug!("no name for {}", self.hir_to_string(id))
         }
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn name_by_hir_id(&self, id: HirId) -> Name {
-        let node_id = self.hir_to_node_id(id);
-        self.name(node_id)
-    }
-
     /// Given a node ID, get a list of attributes associated with the AST
     /// corresponding to the Node ID
     pub fn attrs(&self, id: NodeId) -> &'hir [ast::Attribute] {
-        self.read(id); // reveals attributes on the node
+        let hir_id = self.node_to_hir_id(id);
+        self.attrs_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn attrs_by_hir_id(&self, id: HirId) -> &'hir [ast::Attribute] {
+        self.read_by_hir_id(id); // reveals attributes on the node
         let attrs = match self.find_entry(id).map(|entry| entry.node) {
             Some(Node::Local(l)) => Some(&l.attrs[..]),
             Some(Node::Item(i)) => Some(&i.attrs[..]),
@@ -1025,19 +1027,13 @@ impl<'hir> Map<'hir> {
             Some(Node::GenericParam(param)) => Some(&param.attrs[..]),
             // Unit/tuple structs/variants take the attributes straight from
             // the struct/variant definition.
-            Some(Node::Ctor(..)) => return self.attrs(self.get_parent(id)),
+            Some(Node::Ctor(..)) => return self.attrs_by_hir_id(self.get_parent_item(id)),
             Some(Node::Crate) => Some(&self.forest.krate.attrs[..]),
             _ => None
         };
         attrs.unwrap_or(&[])
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn attrs_by_hir_id(&self, id: HirId) -> &'hir [ast::Attribute] {
-        let node_id = self.hir_to_node_id(id);
-        self.attrs(node_id)
-    }
-
     /// Returns an iterator that yields the node id's with paths that
     /// match `parts`.  (Requires `parts` is non-empty.)
     ///
@@ -1051,13 +1047,19 @@ impl<'hir> Map<'hir> {
             map: self,
             item_name: parts.last().unwrap(),
             in_which: &parts[..parts.len() - 1],
-            idx: CRATE_NODE_ID,
+            idx: ast::CRATE_NODE_ID,
         }
     }
 
     pub fn span(&self, id: NodeId) -> Span {
-        self.read(id); // reveals span from node
+        let hir_id = self.node_to_hir_id(id);
-        match self.find_entry(id).map(|entry| entry.node) {
+        self.span_by_hir_id(hir_id)
+    }
+
+    // FIXME(@ljedrz): replace the NodeId variant
+    pub fn span_by_hir_id(&self, hir_id: HirId) -> Span {
+        self.read_by_hir_id(hir_id); // reveals span from node
+        match self.find_entry(hir_id).map(|entry| entry.node) {
             Some(Node::Item(item)) => item.span,
             Some(Node::ForeignItem(foreign_item)) => foreign_item.span,
             Some(Node::TraitItem(trait_method)) => trait_method.span,
@@ -1073,7 +1075,9 @@ impl<'hir> Map<'hir> {
             Some(Node::Binding(pat)) => pat.span,
             Some(Node::Pat(pat)) => pat.span,
             Some(Node::Block(block)) => block.span,
-            Some(Node::Ctor(..)) => match self.find(self.get_parent_node(id)) {
+            Some(Node::Ctor(..)) => match self.find_by_hir_id(
+                self.get_parent_node_by_hir_id(hir_id))
+            {
                 Some(Node::Item(item)) => item.span,
                 Some(Node::Variant(variant)) => variant.span,
                 _ => unreachable!(),
@@ -1087,16 +1091,10 @@ impl<'hir> Map<'hir> {
             Some(Node::Local(local)) => local.span,
             Some(Node::MacroDef(macro_def)) => macro_def.span,
             Some(Node::Crate) => self.forest.krate.span,
-            None => bug!("hir::map::Map::span: id not in map: {:?}", id),
+            None => bug!("hir::map::Map::span: id not in map: {:?}", hir_id),
         }
     }
 
-    // FIXME(@ljedrz): replace the NodeId variant
-    pub fn span_by_hir_id(&self, id: HirId) -> Span {
-        let node_id = self.hir_to_node_id(id);
-        self.span(node_id)
-    }
-
     pub fn span_if_local(&self, id: DefId) -> Option<Span> {
         self.as_local_node_id(id).map(|id| self.span(id))
     }
@@ -1201,7 +1199,8 @@ impl<'a, 'hir> Iterator for NodesMatchingSuffix<'a, 'hir> {
                 return None;
             }
             self.idx = NodeId::from_u32(self.idx.as_u32() + 1);
-            let name = match self.map.find_entry(idx).map(|entry| entry.node) {
+            let hir_idx = self.map.node_to_hir_id(idx);
+            let name = match self.map.find_entry(hir_idx).map(|entry| entry.node) {
                 Some(Node::Item(n)) => n.name(),
                 Some(Node::ForeignItem(n)) => n.name(),
                 Some(Node::TraitItem(n)) => n.name(),
@@ -1259,18 +1258,6 @@ pub fn map_crate<'hir>(sess: &crate::session::Session,
         )
     };
 
-    if log_enabled!(::log::Level::Debug) {
-        // This only makes sense for ordered stores; note the
-        // enumerate to count the number of entries.
-        let (entries_less_1, _) = map.iter().filter_map(|x| *x).enumerate().last()
-            .expect("AST map was empty after folding?");
-
-        let entries = entries_less_1 + 1;
-        let vector_length = map.len();
-        debug!("The AST map has {} entries with a maximum of {}: occupancy {:.1}%",
-              entries, vector_length, (entries as f64 / vector_length as f64) * 100.);
-    }
-
     let map = Map {
         forest,
         dep_graph: forest.dep_graph.clone(),

src/librustc_privacy/lib.rs

@@ -1827,6 +1827,7 @@ fn check_mod_privacy<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
         empty_tables: &empty_tables,
     };
     let (module, span, hir_id) = tcx.hir().get_module(module_def_id);
+
     intravisit::walk_mod(&mut visitor, module, hir_id);
 
     // Check privacy of explicitly written types and traits as well as

src/librustdoc/core.rs

@@ -166,15 +166,6 @@ impl<'tcx> DocContext<'tcx> {
 
     /// Like the function of the same name on the HIR map, but skips calling it on fake DefIds.
     /// (This avoids a slice-index-out-of-bounds panic.)
-    pub fn as_local_node_id(&self, def_id: DefId) -> Option<ast::NodeId> {
-        if self.all_fake_def_ids.borrow().contains(&def_id) {
-            None
-        } else {
-            self.tcx.hir().as_local_node_id(def_id)
-        }
-    }
-
-    // FIXME(@ljedrz): remove the NodeId variant
     pub fn as_local_hir_id(&self, def_id: DefId) -> Option<HirId> {
         if self.all_fake_def_ids.borrow().contains(&def_id) {
             None

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -38,7 +38,7 @@ pub fn collect_intra_doc_links(krate: Crate, cx: &DocContext<'_>) -> Crate {
 
 struct LinkCollector<'a, 'tcx> {
     cx: &'a DocContext<'tcx>,
-    mod_ids: Vec<ast::NodeId>,
+    mod_ids: Vec<hir::HirId>,
 }
 
 impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
@@ -55,7 +55,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                path_str: &str,
                ns: Namespace,
                current_item: &Option<String>,
-               parent_id: Option<ast::NodeId>)
+               parent_id: Option<hir::HirId>)
         -> Result<(Def, Option<String>), ()>
     {
         let cx = self.cx;
@@ -64,8 +64,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         // path.
         if let Some(id) = parent_id.or(self.mod_ids.last().cloned()) {
             // FIXME: `with_scope` requires the `NodeId` of a module.
+            let node_id = cx.tcx.hir().hir_to_node_id(id);
             let result = cx.enter_resolver(|resolver| {
-                resolver.with_scope(id, |resolver| {
+                resolver.with_scope(node_id, |resolver| {
                     resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns == ValueNS)
                 })
             });
@@ -127,7 +128,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
             }
 
             // FIXME: `with_scope` requires the `NodeId` of a module.
-            let ty = cx.enter_resolver(|resolver| resolver.with_scope(id, |resolver| {
+            let node_id = cx.tcx.hir().hir_to_node_id(id);
+            let ty = cx.enter_resolver(|resolver| resolver.with_scope(node_id, |resolver| {
                     resolver.resolve_str_path_error(DUMMY_SP, &path, false)
             }))?;
             match ty.def {
@@ -216,11 +218,11 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
         };
 
         // FIXME: get the resolver to work with non-local resolve scopes.
-        let parent_node = self.cx.as_local_node_id(item.def_id).and_then(|node_id| {
+        let parent_node = self.cx.as_local_hir_id(item.def_id).and_then(|hir_id| {
             // FIXME: this fails hard for impls in non-module scope, but is necessary for the
             // current `resolve()` implementation.
-            match self.cx.tcx.hir().get_module_parent_node(node_id) {
+            match self.cx.tcx.hir().get_module_parent_node(hir_id) {
-                id if id != node_id => Some(id),
+                id if id != hir_id => Some(id),
                 _ => None,
             }
         });
@@ -239,9 +241,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                     }
                 } else {
                     match parent_node.or(self.mod_ids.last().cloned()) {
-                        Some(parent) if parent != ast::CRATE_NODE_ID => {
+                        Some(parent) if parent != hir::CRATE_HIR_ID => {
                             // FIXME: can we pull the parent module's name from elsewhere?
-                            Some(self.cx.tcx.hir().name(parent).to_string())
+                            Some(self.cx.tcx.hir().name_by_hir_id(parent).to_string())
                         }
                         _ => None,
                     }
@@ -258,7 +260,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
         };
 
         if item.is_mod() && item.attrs.inner_docs {
-            self.mod_ids.push(self.cx.tcx.hir().hir_to_node_id(item_hir_id.unwrap()));
+            self.mod_ids.push(item_hir_id.unwrap());
         }
 
         let cx = self.cx;
@@ -392,7 +394,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
         }
 
         if item.is_mod() && !item.attrs.inner_docs {
-            self.mod_ids.push(self.cx.tcx.hir().hir_to_node_id(item_hir_id.unwrap()));
+            self.mod_ids.push(item_hir_id.unwrap());
         }
 
         if item.is_mod() {

src/librustdoc/visit_ast.rs

@@ -31,7 +31,7 @@ pub struct RustdocVisitor<'a, 'tcx> {
     pub module: Module,
     pub attrs: hir::HirVec<ast::Attribute>,
     pub cx: &'a core::DocContext<'tcx>,
-    view_item_stack: FxHashSet<ast::NodeId>,
+    view_item_stack: FxHashSet<hir::HirId>,
     inlining: bool,
     /// Are the current module and all of its parents public?
     inside_public_path: bool,
@@ -44,7 +44,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
     ) -> RustdocVisitor<'a, 'tcx> {
         // If the root is re-exported, terminate all recursion.
         let mut stack = FxHashSet::default();
-        stack.insert(ast::CRATE_NODE_ID);
+        stack.insert(hir::CRATE_HIR_ID);
         RustdocVisitor {
             module: Module::new(None),
             attrs: hir::HirVec::new(),
@@ -271,13 +271,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                           om: &mut Module,
                           please_inline: bool) -> bool {
 
-        fn inherits_doc_hidden(cx: &core::DocContext<'_>, mut node: ast::NodeId) -> bool {
+        fn inherits_doc_hidden(cx: &core::DocContext<'_>, mut node: hir::HirId) -> bool {
             while let Some(id) = cx.tcx.hir().get_enclosing_scope(node) {
                 node = id;
-                if cx.tcx.hir().attrs(node).lists("doc").has_word("hidden") {
+                if cx.tcx.hir().attrs_by_hir_id(node).lists("doc").has_word("hidden") {
                     return true;
                 }
-                if node == ast::CRATE_NODE_ID {
+                if node == hir::CRATE_HIR_ID {
                     break;
                 }
             }
@@ -326,21 +326,21 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             return false
         }
 
-        let def_node_id = match tcx.hir().as_local_node_id(def_did) {
+        let def_hir_id = match tcx.hir().as_local_hir_id(def_did) {
             Some(n) => n, None => return false
         };
 
         let is_private = !self.cx.renderinfo.borrow().access_levels.is_public(def_did);
-        let is_hidden = inherits_doc_hidden(self.cx, def_node_id);
+        let is_hidden = inherits_doc_hidden(self.cx, def_hir_id);
 
         // Only inline if requested or if the item would otherwise be stripped.
         if (!please_inline && !is_private && !is_hidden) || is_no_inline {
             return false
         }
 
-        if !self.view_item_stack.insert(def_node_id) { return false }
+        if !self.view_item_stack.insert(def_hir_id) { return false }
 
-        let ret = match tcx.hir().get(def_node_id) {
+        let ret = match tcx.hir().get_by_hir_id(def_hir_id) {
             Node::Item(&hir::Item { node: hir::ItemKind::Mod(ref m), .. }) if glob => {
                 let prev = mem::replace(&mut self.inlining, true);
                 for i in &m.item_ids {
@@ -373,7 +373,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             }
             _ => false,
         };
-        self.view_item_stack.remove(&def_node_id);
+        self.view_item_stack.remove(&def_hir_id);
         ret
     }