Auto merge of rust-lang#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

Author: bors

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 @@ impl EdgeFilter {
             })
         }
     }
+
+    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 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
     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 @@ impl 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 edges;
 mod graph;
 mod query;
 mod raii;
+mod shadow;
 mod thread;
 mod visit;
 

src/librustc/dep_graph/raii.rs

@@ -47,3 +47,4 @@ impl<'graph> Drop for IgnoreTask<'graph> {
         self.data.enqueue(DepMessage::PopIgnore);
     }
 }
+

src/librustc/dep_graph/shadow.rs

@@ -0,0 +1,145 @@
+// 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 @@ impl 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 @@ impl DepGraphThreadData {
     /// 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 @@ impl DepGraphThreadData {
             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 @@ impl fmt::Debug for DefId {
         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 @@ pub trait CrateStore<'tcx> {
                              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 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
 
     // 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 @@ dep_map_ty! { ImplTraitRefs: ItemSignature(DefId) -> Option<ty::TraitRef<'tcx>>
 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>> }