Auto merge of #35960 - nikomatsakis:incr-comp-krate-edges, r=michaelwoerister

fix a few errant `Krate` edges

Exploring the effect of small changes on `syntex` reuse, I discovered the following sources of unnecessary edges from `Krate`

r? @michaelwoerister

src/librustc/dep_graph/README.md

@@ -341,6 +341,8 @@ path is found (as demonstrated above).
 
 ### Debugging the dependency graph
 
+#### Dumping the graph
+
 The compiler is also capable of dumping the dependency graph for your
 debugging pleasure. To do so, pass the `-Z dump-dep-graph` flag. The
 graph will be dumped to `dep_graph.{txt,dot}` in the current
@@ -392,6 +394,35 @@ 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.
 
+#### Tracking down incorrect edges
+
+Sometimes, after you dump the dependency graph, you will find some
+path that should not exist, but you will not be quite sure how it came
+to be. **When the compiler is built with debug assertions,** it can
+help you track that down. Simply set the `RUST_FORBID_DEP_GRAPH_EDGE`
+environment variable to a filter. Every edge created in the dep-graph
+will be tested against that filter -- if it matches, a `bug!` is
+reported, so you can easily see the backtrace (`RUST_BACKTRACE=1`).
+
+The syntax for these filters is the same as described in the previous
+section. However, note that this filter is applied to every **edge**
+and doesn't handle longer paths in the graph, unlike the previous
+section.
+
+Example:
+
+You find that there is a path from the `Hir` of `foo` to the type
+check of `bar` and you don't think there should be. You dump the
+dep-graph as described in the previous section and open `dep-graph.txt`
+to see something like:
+
+    Hir(foo) -> Collect(bar)
+    Collect(bar) -> TypeckItemBody(bar)
+    
+That first edge looks suspicious to you. So you set
+`RUST_FORBID_DEP_GRAPH_EDGE` to `Hir&foo -> Collect&bar`, re-run, and
+then observe the backtrace. Voila, bug fixed!
+
 ### Inlining of HIR nodes
 
 For the time being, at least, we still sometimes "inline" HIR nodes

src/librustc/dep_graph/debug.rs

@@ -66,4 +66,11 @@ pub fn new(test: &str) -> Result<EdgeFilter, Box<Error>> {
             })
         }
     }
+
+    pub fn test<D: Clone + Debug>(&self,
+                                  source: &DepNode<D>,
+                                  target: &DepNode<D>)
+                                  -> bool {
+        self.source.test(source) && self.target.test(target)
+    }
 }

src/librustc/dep_graph/dep_tracking_map.rs

@@ -80,6 +80,17 @@ pub fn contains_key(&self, k: &M::Key) -> bool {
     pub fn keys(&self) -> Vec<M::Key> {
         self.map.keys().cloned().collect()
     }
+
+    /// Append `elem` to the vector stored for `k`, creating a new vector if needed.
+    /// This is considered a write to `k`.
+    pub fn push<E: Clone>(&mut self, k: M::Key, elem: E)
+        where M: DepTrackingMapConfig<Value=Vec<E>>
+    {
+        self.write(&k);
+        self.map.entry(k)
+                .or_insert(Vec::new())
+                .push(elem);
+    }
 }
 
 impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {

src/librustc/dep_graph/graph.rs

@@ -46,7 +46,7 @@ pub fn new(enabled: bool) -> DepGraph {
             data: Rc::new(DepGraphData {
                 thread: DepGraphThreadData::new(enabled),
                 previous_work_products: RefCell::new(FnvHashMap()),
-                work_products: RefCell::new(FnvHashMap())
+                work_products: RefCell::new(FnvHashMap()),
             })
         }
     }

src/librustc/dep_graph/mod.rs

@@ -15,6 +15,7 @@
 mod graph;
 mod query;
 mod raii;
+mod shadow;
 mod thread;
 mod visit;
 

src/librustc/dep_graph/raii.rs

@@ -47,3 +47,4 @@ fn drop(&mut self) {
         self.data.enqueue(DepMessage::PopIgnore);
     }
 }
+

src/librustc/dep_graph/shadow.rs

+// Copyright 2012-2015 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.
+
+//! The "Shadow Graph" is maintained on the main thread and which
+//! tracks each message relating to the dep-graph and applies some
+//! sanity checks as they go by. If an error results, it means you get
+//! a nice stack-trace telling you precisely what caused the error.
+//!
+//! NOTE: This is a debugging facility which can potentially have non-trivial
+//! runtime impact. Therefore, it is largely compiled out if
+//! debug-assertions are not enabled.
+//!
+//! The basic sanity check, enabled if you have debug assertions
+//! enabled, is that there is always a task (or ignore) on the stack
+//! when you do read/write, and that the tasks are pushed/popped
+//! according to a proper stack discipline.
+//!
+//! Optionally, if you specify RUST_FORBID_DEP_GRAPH_EDGE, you can
+//! specify an edge filter to be applied to each edge as it is
+//! created.  See `./README.md` for details.
+
+use hir::def_id::DefId;
+use std::cell::{BorrowState, RefCell};
+use std::env;
+
+use super::DepNode;
+use super::thread::DepMessage;
+use super::debug::EdgeFilter;
+
+pub struct ShadowGraph {
+    // if you push None onto the stack, that corresponds to an Ignore
+    stack: RefCell<Vec<Option<DepNode<DefId>>>>,
+    forbidden_edge: Option<EdgeFilter>,
+}
+
+const ENABLED: bool = cfg!(debug_assertions);
+
+impl ShadowGraph {
+    pub fn new() -> Self {
+        let forbidden_edge = if !ENABLED {
+            None
+        } else {
+            match env::var("RUST_FORBID_DEP_GRAPH_EDGE") {
+                Ok(s) => {
+                    match EdgeFilter::new(&s) {
+                        Ok(f) => Some(f),
+                        Err(err) => bug!("RUST_FORBID_DEP_GRAPH_EDGE invalid: {}", err),
+                    }
+                }
+                Err(_) => None,
+            }
+        };
+
+        ShadowGraph {
+            stack: RefCell::new(vec![]),
+            forbidden_edge: forbidden_edge,
+        }
+    }
+
+    pub fn enqueue(&self, message: &DepMessage) {
+        if ENABLED {
+            match self.stack.borrow_state() {
+                BorrowState::Unused => {}
+                _ => {
+                    // When we apply edge filters, that invokes the
+                    // Debug trait on DefIds, which in turn reads from
+                    // various bits of state and creates reads! Ignore
+                    // those recursive reads.
+                    return;
+                }
+            }
+
+            let mut stack = self.stack.borrow_mut();
+            match *message {
+                DepMessage::Read(ref n) => self.check_edge(Some(Some(n)), top(&stack)),
+                DepMessage::Write(ref n) => self.check_edge(top(&stack), Some(Some(n))),
+                DepMessage::PushTask(ref n) => stack.push(Some(n.clone())),
+                DepMessage::PushIgnore => stack.push(None),
+                DepMessage::PopTask(ref n) => {
+                    match stack.pop() {
+                        Some(Some(m)) => {
+                            if *n != m {
+                                bug!("stack mismatch: found {:?} expected {:?}", m, n)
+                            }
+                        }
+                        Some(None) => bug!("stack mismatch: found Ignore expected {:?}", n),
+                        None => bug!("stack mismatch: found empty stack, expected {:?}", n),
+                    }
+                }
+                DepMessage::PopIgnore => {
+                    match stack.pop() {
+                        Some(Some(m)) => bug!("stack mismatch: found {:?} expected ignore", m),
+                        Some(None) => (),
+                        None => bug!("stack mismatch: found empty stack, expected ignore"),
+                    }
+                }
+                DepMessage::Query => (),
+            }
+        }
+    }
+
+    fn check_edge(&self,
+                  source: Option<Option<&DepNode<DefId>>>,
+                  target: Option<Option<&DepNode<DefId>>>) {
+        assert!(ENABLED);
+        match (source, target) {
+            // cannot happen, one side is always Some(Some(_))
+            (None, None) => unreachable!(),
+
+            // nothing on top of the stack
+            (None, Some(n)) | (Some(n), None) => bug!("read/write of {:?} but no current task", n),
+
+            // this corresponds to an Ignore being top of the stack
+            (Some(None), _) | (_, Some(None)) => (),
+
+            // a task is on top of the stack
+            (Some(Some(source)), Some(Some(target))) => {
+                if let Some(ref forbidden_edge) = self.forbidden_edge {
+                    if forbidden_edge.test(source, target) {
+                        bug!("forbidden edge {:?} -> {:?} created", source, target)
+                    }
+                }
+            }
+        }
+    }
+}
+
+// Do a little juggling: we get back a reference to an option at the
+// top of the stack, convert it to an optional reference.
+fn top<'s>(stack: &'s Vec<Option<DepNode<DefId>>>) -> Option<Option<&'s DepNode<DefId>>> {
+    stack.last()
+        .map(|n: &'s Option<DepNode<DefId>>| -> Option<&'s DepNode<DefId>> {
+            // (*)
+            // (*) type annotation just there to clarify what would
+            // otherwise be some *really* obscure code
+            n.as_ref()
+        })
+}

src/librustc/dep_graph/thread.rs

@@ -20,13 +20,13 @@
 
 use hir::def_id::DefId;
 use rustc_data_structures::veccell::VecCell;
-use std::cell::Cell;
 use std::sync::mpsc::{self, Sender, Receiver};
 use std::thread;
 
 use super::DepGraphQuery;
 use super::DepNode;
 use super::edges::DepGraphEdges;
+use super::shadow::ShadowGraph;
 
 #[derive(Debug)]
 pub enum DepMessage {
@@ -42,12 +42,16 @@ pub enum DepMessage {
 pub struct DepGraphThreadData {
     enabled: bool,
 
-    // Local counter that just tracks how many tasks are pushed onto the
-    // stack, so that we still get an error in the case where one is
-    // missing. If dep-graph construction is enabled, we'd get the same
-    // error when processing tasks later on, but that's annoying because
-    // it lacks precision about the source of the error.
-    tasks_pushed: Cell<usize>,
+    // The "shadow graph" is a debugging aid. We give it each message
+    // in real time as it arrives and it checks for various errors
+    // (for example, a read/write when there is no current task; it
+    // can also apply user-defined filters; see `shadow` module for
+    // details). This only occurs if debug-assertions are enabled.
+    //
+    // Note that in some cases the same errors will occur when the
+    // data is processed off the main thread, but that's annoying
+    // because it lacks precision about the source of the error.
+    shadow_graph: ShadowGraph,
 
     // current buffer, where we accumulate messages
     messages: VecCell<DepMessage>,
@@ -76,7 +80,7 @@ pub fn new(enabled: bool) -> DepGraphThreadData {
 
         DepGraphThreadData {
             enabled: enabled,
-            tasks_pushed: Cell::new(0),
+            shadow_graph: ShadowGraph::new(),
             messages: VecCell::with_capacity(INITIAL_CAPACITY),
             swap_in: rx2,
             swap_out: tx1,
@@ -118,21 +122,7 @@ pub fn query(&self) -> DepGraphQuery<DefId> {
     /// the buffer is full, this may swap.)
     #[inline]
     pub fn enqueue(&self, message: DepMessage) {
-        // Regardless of whether dep graph construction is enabled, we
-        // still want to check that we always have a valid task on the
-        // stack when a read/write/etc event occurs.
-        match message {
-            DepMessage::Read(_) | DepMessage::Write(_) =>
-                if self.tasks_pushed.get() == 0 {
-                    self.invalid_message("read/write but no current task")
-                },
-            DepMessage::PushTask(_) | DepMessage::PushIgnore =>
-                self.tasks_pushed.set(self.tasks_pushed.get() + 1),
-            DepMessage::PopTask(_) | DepMessage::PopIgnore =>
-                self.tasks_pushed.set(self.tasks_pushed.get() - 1),
-            DepMessage::Query =>
-                (),
-        }
+        self.shadow_graph.enqueue(&message);
 
         if self.enabled {
             self.enqueue_enabled(message);
@@ -147,11 +137,6 @@ fn enqueue_enabled(&self, message: DepMessage) {
             self.swap();
         }
     }
-
-    // Outline this too.
-    fn invalid_message(&self, string: &str) {
-        bug!("{}; see src/librustc/dep_graph/README.md for more information", string)
-    }
 }
 
 /// Definition of the depgraph thread.

src/librustc/hir/def_id.rs

@@ -58,19 +58,14 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "DefId {{ krate: {:?}, node: {:?}",
                self.krate, self.index)?;
 
-        // Unfortunately, there seems to be no way to attempt to print
-        // a path for a def-id, so I'll just make a best effort for now
-        // and otherwise fallback to just printing the crate/node pair
-        if self.is_local() { // (1)
-            // (1) side-step fact that not all external things have paths at
-            // the moment, such as type parameters
-            ty::tls::with_opt(|opt_tcx| {
-                if let Some(tcx) = opt_tcx {
-                    write!(f, " => {}", tcx.item_path_str(*self))?;
+        ty::tls::with_opt(|opt_tcx| {
+            if let Some(tcx) = opt_tcx {
+                if let Some(def_path) = tcx.opt_def_path(*self) {
+                    write!(f, " => {}", def_path.to_string(tcx))?;
                 }
-                Ok(())
-            })?;
-        }
+            }
+            Ok(())
+        })?;
 
         write!(f, " }}")
     }

src/librustc/hir/map/mod.rs

@@ -927,6 +927,8 @@ pub fn map_decoded_item<'ast, F: FoldOps>(map: &Map<'ast>,
                                           ii: InlinedItem,
                                           fold_ops: F)
                                           -> &'ast InlinedItem {
+    let _ignore = map.forest.dep_graph.in_ignore();
+
     let mut fld = IdAndSpanUpdater::new(fold_ops);
     let ii = match ii {
         II::Item(d, i) => II::Item(fld.fold_ops.new_def_id(d),

src/librustc/lib.rs

@@ -24,6 +24,7 @@
 #![cfg_attr(not(stage0), deny(warnings))]
 
 #![feature(associated_consts)]
+#![feature(borrow_state)]
 #![feature(box_patterns)]
 #![feature(box_syntax)]
 #![feature(collections)]

src/librustc/middle/cstore.rs

@@ -233,7 +233,7 @@ fn def_index_for_def_key(&self,
                              def: DefKey)
                              -> Option<DefIndex>;
     fn def_key(&self, def: DefId) -> hir_map::DefKey;
-    fn relative_def_path(&self, def: DefId) -> hir_map::DefPath;
+    fn relative_def_path(&self, def: DefId) -> Option<hir_map::DefPath>;
     fn variant_kind(&self, def_id: DefId) -> Option<VariantKind>;
     fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option<DefId>;
     fn tuple_struct_definition_if_ctor(&self, did: DefId) -> Option<DefId>;
@@ -430,7 +430,9 @@ fn is_no_builtins(&self, cnum: ast::CrateNum) -> bool { bug!("is_no_builtins") }
 
     // resolve
     fn def_key(&self, def: DefId) -> hir_map::DefKey { bug!("def_key") }
-    fn relative_def_path(&self, def: DefId) -> hir_map::DefPath { bug!("relative_def_path") }
+    fn relative_def_path(&self, def: DefId) -> Option<hir_map::DefPath> {
+        bug!("relative_def_path")
+    }
     fn variant_kind(&self, def_id: DefId) -> Option<VariantKind> { bug!("variant_kind") }
     fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option<DefId>
         { bug!("struct_ctor_def_id") }

src/librustc/ty/maps.rs

@@ -39,7 +39,7 @@ fn to_dep_node(key: &$key) -> DepNode<DefId> { DepNode::$node_name(*key) }
 dep_map_ty! { TraitDefs: ItemSignature(DefId) -> &'tcx ty::TraitDef<'tcx> }
 dep_map_ty! { AdtDefs: ItemSignature(DefId) -> ty::AdtDefMaster<'tcx> }
 dep_map_ty! { ItemVariances: ItemSignature(DefId) -> Rc<Vec<ty::Variance>> }
-dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Rc<Vec<DefId>> }
+dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Vec<DefId> }
 dep_map_ty! { ImplItems: ImplItems(DefId) -> Vec<ty::ImplOrTraitItemId> }
 dep_map_ty! { TraitItems: TraitItems(DefId) -> Rc<Vec<ty::ImplOrTraitItem<'tcx>>> }
 dep_map_ty! { ReprHints: ReprHints(DefId) -> Rc<Vec<attr::ReprAttr>> }

src/librustc/ty/mod.rs

@@ -2465,12 +2465,41 @@ pub fn def_key(self, id: DefId) -> ast_map::DefKey {
         }
     }
 
-    /// Returns the `DefPath` of an item. Note that if `id` is not
-    /// local to this crate -- or is inlined into this crate -- the
-    /// result will be a non-local `DefPath`.
+    /// Convert a `DefId` into its fully expanded `DefPath` (every
+    /// `DefId` is really just an interned def-path).
+    ///
+    /// Note that if `id` is not local to this crate -- or is
+    /// inlined into this crate -- the result will be a non-local
+    /// `DefPath`.
+    ///
+    /// This function is only safe to use when you are sure that the
+    /// full def-path is accessible. Examples that are known to be
+    /// safe are local def-ids or items; see `opt_def_path` for more
+    /// details.
     pub fn def_path(self, id: DefId) -> ast_map::DefPath {
+        self.opt_def_path(id).unwrap_or_else(|| {
+            bug!("could not load def-path for {:?}", id)
+        })
+    }
+
+    /// Convert a `DefId` into its fully expanded `DefPath` (every
+    /// `DefId` is really just an interned def-path).
+    ///
+    /// When going across crates, we do not save the full info for
+    /// every cross-crate def-id, and hence we may not always be able
+    /// to create a def-path. Therefore, this returns
+    /// `Option<DefPath>` to cover that possibility. It will always
+    /// return `Some` for local def-ids, however, as well as for
+    /// items. The problems arise with "minor" def-ids like those
+    /// associated with a pattern, `impl Trait`, or other internal
+    /// detail to a fn.
+    ///
+    /// Note that if `id` is not local to this crate -- or is
+    /// inlined into this crate -- the result will be a non-local
+    /// `DefPath`.
+    pub fn opt_def_path(self, id: DefId) -> Option<ast_map::DefPath> {
         if id.is_local() {
-            self.map.def_path(id)
+            Some(self.map.def_path(id))
         } else {
             self.sess.cstore.relative_def_path(id)
         }
@@ -2693,7 +2722,7 @@ pub fn populate_inherent_implementations_for_type_if_necessary(self,
             self.impl_items.borrow_mut().insert(impl_def_id, impl_items);
         }
 
-        self.inherent_impls.borrow_mut().insert(type_id, Rc::new(inherent_impls));
+        self.inherent_impls.borrow_mut().insert(type_id, inherent_impls);
         self.populated_external_types.borrow_mut().insert(type_id);
     }
 

src/librustc_incremental/assert_dep_graph.rs

@@ -26,19 +26,20 @@
 //! used to check when paths exist or do not.
 //!
 //! The full form of the `rustc_if_this_changed` annotation is
-//! `#[rustc_if_this_changed(id)]`. The `"id"` is optional and
-//! defaults to `"id"` if omitted.
+//! `#[rustc_if_this_changed("foo")]`, which will report a
+//! source node of `foo(def_id)`. The `"foo"` is optional and
+//! defaults to `"Hir"` if omitted.
 //!
 //! Example:
 //!
 //! ```
-//! #[rustc_if_this_changed]
+//! #[rustc_if_this_changed(Hir)]
 //! fn foo() { }
 //!
-//! #[rustc_then_this_would_need("trans")] //~ ERROR no path from `foo`
+//! #[rustc_then_this_would_need(trans)] //~ ERROR no path from `foo`
 //! fn bar() { }
 //!
-//! #[rustc_then_this_would_need("trans")] //~ ERROR OK
+//! #[rustc_then_this_would_need(trans)] //~ ERROR OK
 //! fn baz() { foo(); }
 //! ```
 
@@ -47,7 +48,7 @@
 use rustc::dep_graph::debug::{DepNodeFilter, EdgeFilter};
 use rustc::hir::def_id::DefId;
 use rustc::ty::TyCtxt;
-use rustc_data_structures::fnv::{FnvHashMap, FnvHashSet};
+use rustc_data_structures::fnv::FnvHashSet;
 use rustc_data_structures::graph::{Direction, INCOMING, OUTGOING, NodeIndex};
 use rustc::hir;
 use rustc::hir::intravisit::Visitor;
@@ -61,7 +62,6 @@
 
 const IF_THIS_CHANGED: &'static str = "rustc_if_this_changed";
 const THEN_THIS_WOULD_NEED: &'static str = "rustc_then_this_would_need";
-const ID: &'static str = "id";
 
 pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
     let _ignore = tcx.dep_graph.in_ignore();
@@ -80,8 +80,9 @@ pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
     // Find annotations supplied by user (if any).
     let (if_this_changed, then_this_would_need) = {
         let mut visitor = IfThisChanged { tcx: tcx,
-                                          if_this_changed: FnvHashMap(),
-                                          then_this_would_need: FnvHashMap() };
+                                          if_this_changed: vec![],
+                                          then_this_would_need: vec![] };
+        visitor.process_attrs(ast::CRATE_NODE_ID, &tcx.map.krate().attrs);
         tcx.map.krate().visit_all_items(&mut visitor);
         (visitor.if_this_changed, visitor.then_this_would_need)
     };
@@ -97,58 +98,51 @@ pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
     check_paths(tcx, &if_this_changed, &then_this_would_need);
 }
 
-type SourceHashMap =
-    FnvHashMap<InternedString,
-               FnvHashSet<(Span, DefId, DepNode<DefId>)>>;
-type TargetHashMap =
-    FnvHashMap<InternedString,
-               FnvHashSet<(Span, InternedString, ast::NodeId, DepNode<DefId>)>>;
+type Sources = Vec<(Span, DefId, DepNode<DefId>)>;
+type Targets = Vec<(Span, InternedString, ast::NodeId, DepNode<DefId>)>;
 
 struct IfThisChanged<'a, 'tcx:'a> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
-    if_this_changed: SourceHashMap,
-    then_this_would_need: TargetHashMap,
+    if_this_changed: Sources,
+    then_this_would_need: Targets,
 }
 
 impl<'a, 'tcx> IfThisChanged<'a, 'tcx> {
-    fn process_attrs(&mut self, node_id: ast::NodeId, def_id: DefId) {
-        for attr in self.tcx.get_attrs(def_id).iter() {
+    fn argument(&self, attr: &ast::Attribute) -> Option<InternedString> {
+        let mut value = None;
+        for list_item in attr.meta_item_list().unwrap_or_default() {
+            match list_item.word() {
+                Some(word) if value.is_none() =>
+                    value = Some(word.name().clone()),
+                _ =>
+                    // FIXME better-encapsulate meta_item (don't directly access `node`)
+                    span_bug!(list_item.span(), "unexpected meta-item {:?}", list_item.node),
+            }
+        }
+        value
+    }
+
+    fn process_attrs(&mut self, node_id: ast::NodeId, attrs: &[ast::Attribute]) {
+        let def_id = self.tcx.map.local_def_id(node_id);
+        for attr in attrs {
             if attr.check_name(IF_THIS_CHANGED) {
-                let mut id = None;
-                for list_item in attr.meta_item_list().unwrap_or_default() {
-                    match list_item.word() {
-                        Some(word) if id.is_none() => {
-                            id = Some(word.name().clone())
-                        },
-                        _ => {
-                            // FIXME better-encapsulate meta_item (don't directly access `node`)
-                            span_bug!(list_item.span(), "unexpected list-item {:?}", list_item.node)
+                let dep_node_interned = self.argument(attr);
+                let dep_node = match dep_node_interned {
+                    None => DepNode::Hir(def_id),
+                    Some(ref n) => {
+                        match DepNode::from_label_string(&n[..], def_id) {
+                            Ok(n) => n,
+                            Err(()) => {
+                                self.tcx.sess.span_fatal(
+                                    attr.span,
+                                    &format!("unrecognized DepNode variant {:?}", n));
+                            }
                         }
                     }
-                }
-
-                let id = id.unwrap_or(InternedString::new(ID));
-                self.if_this_changed.entry(id)
-                                    .or_insert(FnvHashSet())
-                                    .insert((attr.span, def_id, DepNode::Hir(def_id)));
+                };
+                self.if_this_changed.push((attr.span, def_id, dep_node));
             } else if attr.check_name(THEN_THIS_WOULD_NEED) {
-                let mut dep_node_interned = None;
-                let mut id = None;
-                for list_item in attr.meta_item_list().unwrap_or_default() {
-                    match list_item.word() {
-                        Some(word) if dep_node_interned.is_none() => {
-                            dep_node_interned = Some(word.name().clone());
-                        },
-                        Some(word) if id.is_none() => {
-                            id = Some(word.name().clone())
-                        },
-                        _ => {
-                            // FIXME better-encapsulate meta_item (don't directly access `node`)
-                            span_bug!(list_item.span(), "unexpected meta-item {:?}", list_item.node)
-                        }
-                    }
-                }
-
+                let dep_node_interned = self.argument(attr);
                 let dep_node = match dep_node_interned {
                     Some(ref n) => {
                         match DepNode::from_label_string(&n[..], def_id) {
@@ -166,11 +160,10 @@ fn process_attrs(&mut self, node_id: ast::NodeId, def_id: DefId) {
                             &format!("missing DepNode variant"));
                     }
                 };
-                let id = id.unwrap_or(InternedString::new(ID));
-                self.then_this_would_need
-                    .entry(id)
-                    .or_insert(FnvHashSet())
-                    .insert((attr.span, dep_node_interned.clone().unwrap(), node_id, dep_node));
+                self.then_this_would_need.push((attr.span,
+                                                dep_node_interned.clone().unwrap(),
+                                                node_id,
+                                                dep_node));
             }
         }
     }
@@ -178,47 +171,38 @@ fn process_attrs(&mut self, node_id: ast::NodeId, def_id: DefId) {
 
 impl<'a, 'tcx> Visitor<'tcx> for IfThisChanged<'a, 'tcx> {
     fn visit_item(&mut self, item: &'tcx hir::Item) {
-        let def_id = self.tcx.map.local_def_id(item.id);
-        self.process_attrs(item.id, def_id);
+        self.process_attrs(item.id, &item.attrs);
     }
 }
 
 fn check_paths<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                         if_this_changed: &SourceHashMap,
-                         then_this_would_need: &TargetHashMap)
+                         if_this_changed: &Sources,
+                         then_this_would_need: &Targets)
 {
     // Return early here so as not to construct the query, which is not cheap.
     if if_this_changed.is_empty() {
+        for &(target_span, _, _, _) in then_this_would_need {
+            tcx.sess.span_err(
+                target_span,
+                &format!("no #[rustc_if_this_changed] annotation detected"));
+
+        }
         return;
     }
     let query = tcx.dep_graph.query();
-    for (id, sources) in if_this_changed {
-        let targets = match then_this_would_need.get(id) {
-            Some(targets) => targets,
-            None => {
-                for &(source_span, ..) in sources.iter().take(1) {
-                    tcx.sess.span_err(
-                        source_span,
-                        &format!("no targets for id `{}`", id));
-                }
-                continue;
-            }
-        };
-
-        for &(_, source_def_id, ref source_dep_node) in sources {
-            let dependents = query.transitive_successors(source_dep_node);
-            for &(target_span, ref target_pass, _, ref target_dep_node) in targets {
-                if !dependents.contains(&target_dep_node) {
-                    tcx.sess.span_err(
-                        target_span,
-                        &format!("no path from `{}` to `{}`",
-                                 tcx.item_path_str(source_def_id),
-                                 target_pass));
-                } else {
-                    tcx.sess.span_err(
-                        target_span,
-                        &format!("OK"));
-                }
+    for &(_, source_def_id, ref source_dep_node) in if_this_changed {
+        let dependents = query.transitive_successors(source_dep_node);
+        for &(target_span, ref target_pass, _, ref target_dep_node) in then_this_would_need {
+            if !dependents.contains(&target_dep_node) {
+                tcx.sess.span_err(
+                    target_span,
+                    &format!("no path from `{}` to `{}`",
+                             tcx.item_path_str(source_def_id),
+                             target_pass));
+            } else {
+                tcx.sess.span_err(
+                    target_span,
+                    &format!("OK"));
             }
         }
     }

src/librustc_metadata/common.rs

@@ -149,9 +149,9 @@ pub enum astencode_tag { // Reserves 0x50 -- 0x6f
 pub const tag_items_data_item_inherent_impl: usize = 0x79;
 // GAP 0x7a
 pub const tag_mod_child: usize = 0x7b;
-pub const tag_misc_info: usize = 0x108; // top-level only
-pub const tag_misc_info_crate_items: usize = 0x7c;
+// GAP 0x7c
 
+// GAP 0x108
 pub const tag_impls: usize = 0x109; // top-level only
 pub const tag_impls_trait: usize = 0x7d;
 pub const tag_impls_trait_impl: usize = 0x7e;

src/librustc_metadata/csearch.rs

@@ -425,13 +425,21 @@ fn def_index_for_def_key(&self,
     /// parent `DefId` as well as some idea of what kind of data the
     /// `DefId` refers to.
     fn def_key(&self, def: DefId) -> hir_map::DefKey {
-        self.dep_graph.read(DepNode::MetaData(def));
+        // Note: loading the def-key (or def-path) for a def-id is not
+        // a *read* of its metadata. This is because the def-id is
+        // really just an interned shorthand for a def-path, which is the
+        // canonical name for an item.
+        //
+        // self.dep_graph.read(DepNode::MetaData(def));
         let cdata = self.get_crate_data(def.krate);
         decoder::def_key(&cdata, def.index)
     }
 
-    fn relative_def_path(&self, def: DefId) -> hir_map::DefPath {
-        self.dep_graph.read(DepNode::MetaData(def));
+    fn relative_def_path(&self, def: DefId) -> Option<hir_map::DefPath> {
+        // See `Note` above in `def_key()` for why this read is
+        // commented out:
+        //
+        // self.dep_graph.read(DepNode::MetaData(def));
         let cdata = self.get_crate_data(def.krate);
         decoder::def_path(&cdata, def.index)
     }

src/librustc_metadata/decoder.rs

@@ -23,6 +23,7 @@
 use tls_context;
 use tydecode::TyDecoder;
 
+use rustc::hir::def_id::CRATE_DEF_INDEX;
 use rustc::hir::svh::Svh;
 use rustc::hir::map as hir_map;
 use rustc::hir::map::DefKey;
@@ -727,15 +728,7 @@ pub fn each_top_level_item_of_crate<F, G>(cdata: Cmd, get_crate_data: G, callbac
     where F: FnMut(DefLike, ast::Name, ty::Visibility),
           G: FnMut(ast::CrateNum) -> Rc<CrateMetadata>,
 {
-    let root_doc = rbml::Doc::new(cdata.data());
-    let misc_info_doc = reader::get_doc(root_doc, tag_misc_info);
-    let crate_items_doc = reader::get_doc(misc_info_doc,
-                                          tag_misc_info_crate_items);
-
-    each_child_of_item_or_crate(cdata,
-                                crate_items_doc,
-                                get_crate_data,
-                                callback)
+    each_child_of_item(cdata, CRATE_DEF_INDEX, get_crate_data, callback)
 }
 
 pub fn get_item_name(cdata: Cmd, id: DefIndex) -> ast::Name {
@@ -761,7 +754,7 @@ pub fn maybe_get_item_ast<'a, 'tcx>(cdata: Cmd, tcx: TyCtxt<'a, 'tcx, 'tcx>, id:
         krate: cdata.cnum,
         index: def_key(cdata, id).parent.unwrap()
     };
-    let mut parent_def_path = def_path(cdata, id);
+    let mut parent_def_path = def_path(cdata, id).unwrap();
     parent_def_path.data.pop();
     if let Some(ast_doc) = reader::maybe_get_doc(item_doc, tag_ast as usize) {
         let ii = decode_inlined_item(cdata,
@@ -1628,9 +1621,16 @@ fn item_def_key(item_doc: rbml::Doc) -> hir_map::DefKey {
     }
 }
 
-pub fn def_path(cdata: Cmd, id: DefIndex) -> hir_map::DefPath {
+// Returns the path leading to the thing with this `id`. Note that
+// some def-ids don't wind up in the metadata, so `def_path` sometimes
+// returns `None`
+pub fn def_path(cdata: Cmd, id: DefIndex) -> Option<hir_map::DefPath> {
     debug!("def_path(id={:?})", id);
-    hir_map::DefPath::make(cdata.cnum, id, |parent| def_key(cdata, parent))
+    if cdata.get_item(id).is_some() {
+        Some(hir_map::DefPath::make(cdata.cnum, id, |parent| def_key(cdata, parent)))
+    } else {
+        None
+    }
 }
 
 pub fn get_panic_strategy(data: &[u8]) -> PanicStrategy {

src/librustc_metadata/encoder.rs

@@ -1693,30 +1693,6 @@ fn encode_impls<'a>(ecx: &'a EncodeContext,
     rbml_w.end_tag();
 }
 
-fn encode_misc_info(ecx: &EncodeContext,
-                    krate: &hir::Crate,
-                    rbml_w: &mut Encoder) {
-    rbml_w.start_tag(tag_misc_info);
-    rbml_w.start_tag(tag_misc_info_crate_items);
-    for item_id in &krate.module.item_ids {
-        rbml_w.wr_tagged_u64(tag_mod_child,
-                             def_to_u64(ecx.tcx.map.local_def_id(item_id.id)));
-
-        let item = ecx.tcx.map.expect_item(item_id.id);
-        each_auxiliary_node_id(item, |auxiliary_node_id| {
-            rbml_w.wr_tagged_u64(tag_mod_child,
-                                 def_to_u64(ecx.tcx.map.local_def_id(auxiliary_node_id)));
-            true
-        });
-    }
-
-    // Encode reexports for the root module.
-    encode_reexports(ecx, rbml_w, 0);
-
-    rbml_w.end_tag();
-    rbml_w.end_tag();
-}
-
 // Encodes all reachable symbols in this crate into the metadata.
 //
 // This pass is seeded off the reachability list calculated in the
@@ -1861,7 +1837,7 @@ struct Stats {
         codemap_bytes: u64,
         macro_defs_bytes: u64,
         impl_bytes: u64,
-        misc_bytes: u64,
+        reachable_bytes: u64,
         item_bytes: u64,
         index_bytes: u64,
         xref_bytes: u64,
@@ -1877,7 +1853,7 @@ struct Stats {
         codemap_bytes: 0,
         macro_defs_bytes: 0,
         impl_bytes: 0,
-        misc_bytes: 0,
+        reachable_bytes: 0,
         item_bytes: 0,
         index_bytes: 0,
         xref_bytes: 0,
@@ -1931,11 +1907,10 @@ struct Stats {
     encode_impls(&ecx, krate, rbml_w);
     stats.impl_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i;
 
-    // Encode miscellaneous info.
+    // Encode reachability info.
     i = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap();
-    encode_misc_info(&ecx, krate, rbml_w);
     encode_reachable(&ecx, rbml_w);
-    stats.misc_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i;
+    stats.reachable_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i;
 
     // Encode and index the items.
     rbml_w.start_tag(tag_items);
@@ -1972,7 +1947,7 @@ struct Stats {
         println!("         codemap bytes: {}", stats.codemap_bytes);
         println!("       macro def bytes: {}", stats.macro_defs_bytes);
         println!("            impl bytes: {}", stats.impl_bytes);
-        println!("            misc bytes: {}", stats.misc_bytes);
+        println!("       reachable bytes: {}", stats.reachable_bytes);
         println!("            item bytes: {}", stats.item_bytes);
         println!("           index bytes: {}", stats.index_bytes);
         println!("            xref bytes: {}", stats.xref_bytes);

src/librustc_typeck/coherence/mod.rs

@@ -32,10 +32,7 @@
 use middle::free_region::FreeRegionMap;
 use CrateCtxt;
 use rustc::infer::{self, InferCtxt, TypeOrigin};
-use std::cell::RefCell;
-use std::rc::Rc;
 use syntax_pos::Span;
-use util::nodemap::{DefIdMap, FnvHashMap};
 use rustc::dep_graph::DepNode;
 use rustc::hir::map as hir_map;
 use rustc::hir::intravisit;
@@ -49,7 +46,6 @@
 struct CoherenceChecker<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     crate_context: &'a CrateCtxt<'a, 'gcx>,
     inference_context: InferCtxt<'a, 'gcx, 'tcx>,
-    inherent_impls: RefCell<DefIdMap<Rc<RefCell<Vec<DefId>>>>>,
 }
 
 struct CoherenceCheckVisitor<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
@@ -107,15 +103,6 @@ fn check(&self) {
             DepNode::CoherenceCheckImpl,
             &mut CoherenceCheckVisitor { cc: self });
 
-        // Copy over the inherent impls we gathered up during the walk into
-        // the tcx.
-        let mut tcx_inherent_impls =
-            self.crate_context.tcx.inherent_impls.borrow_mut();
-        for (k, v) in self.inherent_impls.borrow().iter() {
-            tcx_inherent_impls.insert((*k).clone(),
-                                      Rc::new((*v.borrow()).clone()));
-        }
-
         // Populate the table of destructors. It might seem a bit strange to
         // do this here, but it's actually the most convenient place, since
         // the coherence tables contain the trait -> type mappings.
@@ -173,14 +160,8 @@ fn check_implementation(&self, item: &Item) {
     }
 
     fn add_inherent_impl(&self, base_def_id: DefId, impl_def_id: DefId) {
-        if let Some(implementation_list) = self.inherent_impls.borrow().get(&base_def_id) {
-            implementation_list.borrow_mut().push(impl_def_id);
-            return;
-        }
-
-        self.inherent_impls.borrow_mut().insert(
-            base_def_id,
-            Rc::new(RefCell::new(vec!(impl_def_id))));
+        let tcx = self.crate_context.tcx;
+        tcx.inherent_impls.borrow_mut().push(base_def_id, impl_def_id);
     }
 
     fn add_trait_impl(&self, impl_trait_ref: ty::TraitRef<'gcx>, impl_def_id: DefId) {
@@ -553,7 +534,6 @@ pub fn check_coherence(ccx: &CrateCtxt) {
         CoherenceChecker {
             crate_context: ccx,
             inference_context: infcx,
-            inherent_impls: RefCell::new(FnvHashMap()),
         }.check();
     });
     unsafety::check(ccx.tcx);

src/test/incremental/krate-inherent.rs

+// Copyright 2014 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.
+
+// revisions: rpass1 rpass2
+// compile-flags: -Z query-dep-graph
+
+#![allow(warnings)]
+#![feature(rustc_attrs)]
+#![rustc_partition_reused(module="krate_inherent-x", cfg="rpass2")]
+
+fn main() { }
+
+mod x {
+    struct Foo;
+    impl Foo {
+        fn foo(&self) { }
+    }
+
+    fn method() {
+        let x: Foo = Foo;
+        x.foo(); // inherent methods used to add an edge from Krate
+    }
+}
+
+#[cfg(rpass1)]
+fn bar() { } // remove this unrelated fn in rpass2, which should not affect `x::method`
+

src/test/incremental/krate-inlined.rs

+// Copyright 2014 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.
+
+// Regr. test that using HIR inlined from another krate does *not* add
+// a dependency from the local Krate node.
+
+// revisions: cfail1
+// compile-flags: -Z query-dep-graph
+
+#![allow(warnings)]
+#![feature(rustc_attrs)]
+
+#![rustc_if_this_changed(Krate)]
+
+fn main() { }
+
+mod x {
+    #[rustc_then_this_would_need(TransCrateItem)] //[cfail1]~ ERROR no path
+    fn method() {
+        // use some methods that require inlining HIR from another crate:
+        let mut v = vec![];
+        v.push(1);
+    }
+}

src/test/incremental/remove-private-item-cross-crate/auxiliary/a.rs

+// Copyright 2016 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.
+
+#![allow(warnings)]
+#![crate_name = "a"]
+#![crate_type = "rlib"]
+
+pub fn foo(b: u8) -> u32 { b as u32 }
+
+#[cfg(rpass1)]
+fn bar() { }

src/test/incremental/remove-private-item-cross-crate/main.rs

+// Copyright 2016 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.
+
+// Test that we are able to reuse `main` even though a private
+// item was removed from the root module of crate`a`.
+
+// revisions:rpass1 rpass2
+// aux-build:a.rs
+
+#![feature(rustc_attrs)]
+#![crate_type = "bin"]
+#![rustc_partition_reused(module="main", cfg="rpass2")]
+
+extern crate a;
+
+pub fn main() {
+    let vec: Vec<u8> = vec![0, 1, 2, 3];
+    for &b in &vec {
+        println!("{}", a::foo(b));
+    }
+}