replace graph rewriting with detecting inlined ids

We now detect inlined id's earlier (in the HIR map) and rewrite a read
of them to be a read of the metadata for the associated item.

Author: nikomatsakis

src/librustc/dep_graph/README.md

@@ -134,6 +134,10 @@ to read from it. Similarly, reading from the `tcache` map for item `X`
 (which is a `DepTrackingMap`, described below) automatically invokes
 `dep_graph.read(ItemSignature(X))`.
 
+**Note:** adding `Hir` nodes requires a bit of caution due to the
+"inlining" that old trans and constant evaluation still use. See the
+section on inlining below.
+
 To make this strategy work, a certain amount of indirection is
 required. For example, modules in the HIR do not have direct pointers
 to the items that they contain. Rather, they contain node-ids -- one
@@ -387,3 +391,24 @@ RUST_DEP_GRAPH_FILTER='Hir&foo -> TypeckItemBody & bar'
 This will dump out all the nodes that lead from `Hir(foo)` to
 `TypeckItemBody(bar)`, from which you can (hopefully) see the source
 of the erroneous edge.
+
+### Inlining of HIR nodes
+
+For the time being, at least, we still sometimes "inline" HIR nodes
+from other crates into the current HIR map. This creates a weird
+scenario where the same logical item (let's call it `X`) has two
+def-ids: the original def-id `X` and a new, inlined one `X'`. `X'` is
+in the current crate, but it's not like other HIR nodes: in
+particular, when we restart compilation, it will not be available to
+hash. Therefore, we do not want `Hir(X')` nodes appearing in our
+graph.  Instead, we want a "read" of `Hir(X')` to be represented as a
+read of `MetaData(X)`, since the metadata for `X` is where the inlined
+representation originated in the first place.
+
+To achieve this, the HIR map will detect if the def-id originates in
+an inlined node and add a dependency to a suitable `MetaData` node
+instead. If you are reading a HIR node and are not sure if it may be
+inlined or not, you can use `tcx.map.read(node_id)` and it will detect
+whether the node is inlined or not and do the right thing.  You can
+also use `tcx.map.is_inlined_def_id()` and
+`tcx.map.is_inlined_node_id()` to test.

src/librustc/dep_graph/visit.rs

@@ -41,6 +41,7 @@ pub fn visit_all_items_in_krate<'a, 'tcx, V, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
             let task_id = (self.dep_node_fn)(item_def_id);
             let _task = self.tcx.dep_graph.in_task(task_id.clone());
             debug!("Started task {:?}", task_id);
+            assert!(!self.tcx.map.is_inlined_def_id(item_def_id));
             self.tcx.dep_graph.read(DepNode::Hir(item_def_id));
             self.visitor.visit_item(i);
             debug!("Ended task {:?}", task_id);

src/librustc/hir/map/mod.rs

@@ -226,60 +226,99 @@ impl<'ast> Map<'ast> {
     /// otherwise have had access to those contents, and hence needs a
     /// read recorded). If the function just returns a DefId or
     /// NodeId, no actual content was returned, so no read is needed.
-    fn read(&self, id: NodeId) {
+    pub fn read(&self, id: NodeId) {
         self.dep_graph.read(self.dep_node(id));
     }
 
     fn dep_node(&self, id0: NodeId) -> DepNode<DefId> {
         let map = self.map.borrow();
         let mut id = id0;
-        loop {
-            match map[id as usize] {
-                EntryItem(_, item) => {
-                    let def_id = self.local_def_id(item.id);
-                    // NB                          ^~~~~~~
-                    //
-                    // You would expect that `item.id == id`, but this
-                    // is not always the case. In particular, for a
-                    // ViewPath item like `use self::{mem, foo}`, we
-                    // map the ids for `mem` and `foo` to the
-                    // enclosing view path item. This seems mega super
-                    // ultra wrong, but then who am I to judge?
-                    // -nmatsakis
-                    return DepNode::Hir(def_id);
-                }
+        if !self.is_inlined_node_id(id) {
+            loop {
+                match map[id as usize] {
+                    EntryItem(_, item) => {
+                        let def_id = self.local_def_id(item.id);
+                        // NB                          ^~~~~~~
+                        //
+                        // You would expect that `item.id == id`, but this
+                        // is not always the case. In particular, for a
+                        // ViewPath item like `use self::{mem, foo}`, we
+                        // map the ids for `mem` and `foo` to the
+                        // enclosing view path item. This seems mega super
+                        // ultra wrong, but then who am I to judge?
+                        // -nmatsakis
+                        assert!(!self.is_inlined_def_id(def_id));
+                        return DepNode::Hir(def_id);
+                    }
 
-                EntryForeignItem(p, _) |
-                EntryTraitItem(p, _) |
-                EntryImplItem(p, _) |
-                EntryVariant(p, _) |
-                EntryExpr(p, _) |
-                EntryStmt(p, _) |
-                EntryLocal(p, _) |
-                EntryPat(p, _) |
-                EntryBlock(p, _) |
-                EntryStructCtor(p, _) |
-                EntryLifetime(p, _) |
-                EntryTyParam(p, _) =>
-                    id = p,
-
-                RootCrate |
-                RootInlinedParent(_) =>
-                    // FIXME(#32015) clarify story about cross-crate dep tracking
-                    return DepNode::Krate,
-
-                NotPresent =>
-                    // Some nodes, notably struct fields, are not
-                    // present in the map for whatever reason, but
-                    // they *do* have def-ids. So if we encounter an
-                    // empty hole, check for that case.
-                    return self.opt_local_def_id(id)
-                               .map(|def_id| DepNode::Hir(def_id))
-                               .unwrap_or_else(|| {
-                                   bug!("Walking parents from `{}` \
-                                         led to `NotPresent` at `{}`",
-                                        id0, id)
-                               }),
+                    EntryForeignItem(p, _) |
+                    EntryTraitItem(p, _) |
+                    EntryImplItem(p, _) |
+                    EntryVariant(p, _) |
+                    EntryExpr(p, _) |
+                    EntryStmt(p, _) |
+                    EntryLocal(p, _) |
+                    EntryPat(p, _) |
+                    EntryBlock(p, _) |
+                    EntryStructCtor(p, _) |
+                    EntryLifetime(p, _) |
+                    EntryTyParam(p, _) =>
+                        id = p,
+
+                    RootCrate =>
+                        return DepNode::Krate,
+
+                    RootInlinedParent(_) =>
+                        bug!("node {} has inlined ancestor but is not inlined", id0),
+
+                    NotPresent =>
+                        // Some nodes, notably struct fields, are not
+                        // present in the map for whatever reason, but
+                        // they *do* have def-ids. So if we encounter an
+                        // empty hole, check for that case.
+                        return self.opt_local_def_id(id)
+                                   .map(|def_id| DepNode::Hir(def_id))
+                                   .unwrap_or_else(|| {
+                                       bug!("Walking parents from `{}` \
+                                             led to `NotPresent` at `{}`",
+                                            id0, id)
+                                   }),
+                }
+            }
+        } else {
+            // reading from an inlined def-id is really a read out of
+            // the metadata from which we loaded the item.
+            loop {
+                match map[id as usize] {
+                    EntryItem(p, _) |
+                    EntryForeignItem(p, _) |
+                    EntryTraitItem(p, _) |
+                    EntryImplItem(p, _) |
+                    EntryVariant(p, _) |
+                    EntryExpr(p, _) |
+                    EntryStmt(p, _) |
+                    EntryLocal(p, _) |
+                    EntryPat(p, _) |
+                    EntryBlock(p, _) |
+                    EntryStructCtor(p, _) |
+                    EntryLifetime(p, _) |
+                    EntryTyParam(p, _) =>
+                        id = p,
+
+                    RootInlinedParent(parent) => match *parent {
+                        InlinedItem::Item(def_id, _) |
+                        InlinedItem::TraitItem(def_id, _) |
+                        InlinedItem::ImplItem(def_id, _) |
+                        InlinedItem::Foreign(def_id, _) =>
+                            return DepNode::MetaData(def_id)
+                    },
+
+                    RootCrate =>
+                        bug!("node {} has crate ancestor but is inlined", id0),
+
+                    NotPresent =>
+                        bug!("node {} is inlined but not present in map", id0),
+                }
             }
         }
     }
@@ -876,7 +915,8 @@ pub fn map_decoded_item<'ast, F: FoldOps>(map: &Map<'ast>,
                                           -> &'ast InlinedItem {
     let mut fld = IdAndSpanUpdater::new(fold_ops);
     let ii = match ii {
-        II::Item(i) => II::Item(i.map(|i| fld.fold_item(i))),
+        II::Item(d, i) => II::Item(fld.fold_ops.new_def_id(d),
+                                   i.map(|i| fld.fold_item(i))),
         II::TraitItem(d, ti) => {
             II::TraitItem(fld.fold_ops.new_def_id(d),
                           ti.map(|ti| fld.fold_trait_item(ti)))
@@ -885,7 +925,8 @@ pub fn map_decoded_item<'ast, F: FoldOps>(map: &Map<'ast>,
             II::ImplItem(fld.fold_ops.new_def_id(d),
                          ii.map(|ii| fld.fold_impl_item(ii)))
         }
-        II::Foreign(i) => II::Foreign(i.map(|i| fld.fold_foreign_item(i)))
+        II::Foreign(d, i) => II::Foreign(fld.fold_ops.new_def_id(d),
+                                         i.map(|i| fld.fold_foreign_item(i)))
     };
 
     let ii = map.forest.inlined_items.alloc(ii);

src/librustc/middle/cstore.rs

@@ -94,19 +94,19 @@ pub enum DefLike {
 /// that we trans.
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
 pub enum InlinedItem {
-    Item(P<hir::Item>),
+    Item(DefId /* def-id in source crate */, P<hir::Item>),
     TraitItem(DefId /* impl id */, P<hir::TraitItem>),
     ImplItem(DefId /* impl id */, P<hir::ImplItem>),
-    Foreign(P<hir::ForeignItem>),
+    Foreign(DefId /* extern item */, P<hir::ForeignItem>),
 }
 
 /// A borrowed version of `hir::InlinedItem`.
 #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
 pub enum InlinedItemRef<'a> {
-    Item(&'a hir::Item),
+    Item(DefId, &'a hir::Item),
     TraitItem(DefId, &'a hir::TraitItem),
     ImplItem(DefId, &'a hir::ImplItem),
-    Foreign(&'a hir::ForeignItem)
+    Foreign(DefId, &'a hir::ForeignItem)
 }
 
 /// Item definitions in the currently-compiled crate would have the CrateNum
@@ -283,8 +283,8 @@ impl InlinedItem {
         where V: Visitor<'ast>
     {
         match *self {
-            InlinedItem::Item(ref i) => visitor.visit_item(&i),
-            InlinedItem::Foreign(ref i) => visitor.visit_foreign_item(&i),
+            InlinedItem::Item(_, ref i) => visitor.visit_item(&i),
+            InlinedItem::Foreign(_, ref i) => visitor.visit_foreign_item(&i),
             InlinedItem::TraitItem(_, ref ti) => visitor.visit_trait_item(ti),
             InlinedItem::ImplItem(_, ref ii) => visitor.visit_impl_item(ii),
         }

src/librustc_const_eval/eval.rs

@@ -142,7 +142,7 @@ pub fn lookup_const_by_id<'a, 'tcx: 'a>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
         }
         let mut used_substs = false;
         let expr_ty = match tcx.sess.cstore.maybe_get_item_ast(tcx, def_id) {
-            Some((&InlinedItem::Item(ref item), _)) => match item.node {
+            Some((&InlinedItem::Item(_, ref item), _)) => match item.node {
                 hir::ItemConst(ref ty, ref const_expr) => {
                     Some((&**const_expr, tcx.ast_ty_to_prim_ty(ty)))
                 },
@@ -198,7 +198,7 @@ fn inline_const_fn_from_external_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     }
 
     let fn_id = match tcx.sess.cstore.maybe_get_item_ast(tcx, def_id) {
-        Some((&InlinedItem::Item(ref item), _)) => Some(item.id),
+        Some((&InlinedItem::Item(_, ref item), _)) => Some(item.id),
         Some((&InlinedItem::ImplItem(_, ref item), _)) => Some(item.id),
         _ => None
     };

src/librustc_incremental/persist/hash.rs

@@ -70,6 +70,11 @@ impl<'a, 'tcx> HashContext<'a, 'tcx> {
                 def_id,
                 self.tcx.item_path_str(def_id));
 
+        assert!(!self.tcx.map.is_inlined_def_id(def_id),
+                "cannot hash HIR for inlined def-id {:?} => {:?}",
+                def_id,
+                self.tcx.item_path_str(def_id));
+
         // FIXME(#32753) -- should we use a distinct hash here
         self.tcx.calculate_item_hash(def_id)
     }

src/librustc_incremental/persist/save.rs

@@ -106,7 +106,7 @@ pub fn encode_dep_graph<'a, 'tcx>(hcx: &mut HashContext<'a, 'tcx>,
                                   encoder: &mut Encoder)
                                   -> io::Result<()>
 {
-    let (nodes, edges) = post_process_graph(hcx, query);
+    let (nodes, edges) = (query.nodes(), query.edges());
 
     // Create hashes for inputs.
     let hashes =
@@ -142,57 +142,6 @@ pub fn encode_dep_graph<'a, 'tcx>(hcx: &mut HashContext<'a, 'tcx>,
     Ok(())
 }
 
-pub fn post_process_graph<'a, 'tcx>(hcx: &mut HashContext<'a, 'tcx>,
-                                    query: &DepGraphQuery<DefId>)
-                                    -> (Vec<DepNode<DefId>>, Vec<(DepNode<DefId>, DepNode<DefId>)>)
-{
-    let tcx = hcx.tcx;
-    let mut cache = FnvHashMap();
-
-    let mut uninline_def_id = |def_id: DefId| -> Option<DefId> {
-        if tcx.map.is_inlined_def_id(def_id) {
-            Some(
-                cache.entry(def_id)
-                     .or_insert_with(|| {
-                         let def_path = tcx.def_path(def_id);
-                         debug!("post_process_graph: uninlining def-id {:?} to yield {:?}",
-                                def_id, def_path);
-                         let retraced_def_id = tcx.retrace_path(&def_path).unwrap();
-                         debug!("post_process_graph: retraced to {:?}", retraced_def_id);
-                         retraced_def_id
-                     })
-                     .clone())
-        } else {
-            None
-        }
-    };
-
-    let mut uninline_metadata = |node: &DepNode<DefId>| -> DepNode<DefId> {
-        match *node {
-            DepNode::Hir(def_id) => {
-                match uninline_def_id(def_id) {
-                    Some(uninlined_def_id) => DepNode::MetaData(uninlined_def_id),
-                    None => DepNode::Hir(def_id)
-                }
-            }
-            _ => node.clone()
-        }
-    };
-
-    let nodes = query.nodes()
-                     .into_iter()
-                     .map(|node| uninline_metadata(node))
-                     .collect();
-
-    let edges = query.edges()
-                     .into_iter()
-                     .map(|(from, to)| (uninline_metadata(from), uninline_metadata(to)))
-                     .collect();
-
-    (nodes, edges)
-}
-
-
 pub fn encode_metadata_hashes<'a, 'tcx>(hcx: &mut HashContext<'a, 'tcx>,
                                         builder: &mut DefIdDirectoryBuilder,
                                         query: &DepGraphQuery<DefId>,