Fix dep graph edges around the fulfillment cache in the tcx

src/librustc/dep_graph/mod.rs

@@ -11,7 +11,6 @@
 use self::thread::{DepGraphThreadData, DepMessage};
 use middle::def_id::DefId;
 use middle::ty;
-use middle::ty::fast_reject::SimplifiedType;
 use rustc_front::hir;
 use rustc_front::intravisit::Visitor;
 use std::rc::Rc;
@@ -102,7 +101,7 @@ pub enum DepNode {
     // which would yield an overly conservative dep-graph.
     TraitItems(DefId),
     ReprHints(DefId),
-    TraitSelect(DefId, Option<SimplifiedType>),
+    TraitSelect(DefId),
 }
 
 #[derive(Clone)]

src/librustc/middle/traits/fulfill.rs

@@ -8,6 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+use dep_graph::DepGraph;
 use middle::infer::InferCtxt;
 use middle::ty::{self, Ty, TypeFoldable};
 use rustc_data_structures::obligation_forest::{Backtrace, ObligationForest, Error};
@@ -30,7 +31,12 @@ use super::select::SelectionContext;
 use super::Unimplemented;
 use super::util::predicate_for_builtin_bound;
 
-pub struct FulfilledPredicates<'tcx> {
+pub struct GlobalFulfilledPredicates<'tcx> {
+    set: FnvHashSet<ty::PolyTraitPredicate<'tcx>>,
+    dep_graph: DepGraph,
+}
+
+pub struct LocalFulfilledPredicates<'tcx> {
     set: FnvHashSet<ty::Predicate<'tcx>>
 }
 
@@ -56,7 +62,7 @@ pub struct FulfillmentContext<'tcx> {
     // initially-distinct type variables are unified after being
     // inserted. Deduplicating the predicate set on selection had a
     // significant performance cost the last time I checked.
-    duplicate_set: FulfilledPredicates<'tcx>,
+    duplicate_set: LocalFulfilledPredicates<'tcx>,
 
     // A list of all obligations that have been registered with this
     // fulfillment context.
@@ -106,7 +112,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
     /// Creates a new fulfillment context.
     pub fn new() -> FulfillmentContext<'tcx> {
         FulfillmentContext {
-            duplicate_set: FulfilledPredicates::new(),
+            duplicate_set: LocalFulfilledPredicates::new(),
             predicates: ObligationForest::new(),
             region_obligations: NodeMap(),
         }
@@ -240,7 +246,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
         // local cache).  This is because the tcx cache maintains the
         // invariant that it only contains things that have been
         // proven, and we have not yet proven that `predicate` holds.
-        if predicate.is_global() && tcx.fulfilled_predicates.borrow().is_duplicate(predicate) {
+        if tcx.fulfilled_predicates.borrow().check_duplicate(predicate) {
             return true;
         }
 
@@ -283,10 +289,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
             // these are obligations that were proven to be true.
             for pending_obligation in outcome.completed {
                 let predicate = &pending_obligation.obligation.predicate;
-                if predicate.is_global() {
-                    selcx.tcx().fulfilled_predicates.borrow_mut()
-                                                    .is_duplicate_or_add(predicate);
-                }
+                selcx.tcx().fulfilled_predicates.borrow_mut().add_if_global(predicate);
             }
 
             errors.extend(
@@ -329,17 +332,16 @@ fn process_predicate<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>,
             // However, this is a touch tricky, so I'm doing something
             // a bit hackier for now so that the `huge-struct.rs` passes.
 
+            let tcx = selcx.tcx();
+
             let retain_vec: Vec<_> = {
                 let mut dedup = FnvHashSet();
                 v.iter()
                  .map(|o| {
                      // Screen out obligations that we know globally
                      // are true. This should really be the DAG check
                      // mentioned above.
-                     if
-                         o.predicate.is_global() &&
-                         selcx.tcx().fulfilled_predicates.borrow().is_duplicate(&o.predicate)
-                     {
+                     if tcx.fulfilled_predicates.borrow().check_duplicate(&o.predicate) {
                          return false;
                      }
 
@@ -611,22 +613,62 @@ fn register_region_obligation<'tcx>(t_a: Ty<'tcx>,
 
 }
 
-impl<'tcx> FulfilledPredicates<'tcx> {
-    pub fn new() -> FulfilledPredicates<'tcx> {
-        FulfilledPredicates {
+impl<'tcx> LocalFulfilledPredicates<'tcx> {
+    pub fn new() -> LocalFulfilledPredicates<'tcx> {
+        LocalFulfilledPredicates {
             set: FnvHashSet()
         }
     }
 
-    pub fn is_duplicate(&self, key: &ty::Predicate<'tcx>) -> bool {
-        self.set.contains(key)
-    }
-
     fn is_duplicate_or_add(&mut self, key: &ty::Predicate<'tcx>) -> bool {
+        // For a `LocalFulfilledPredicates`, if we find a match, we
+        // don't need to add a read edge to the dep-graph. This is
+        // because it means that the predicate has already been
+        // considered by this `FulfillmentContext`, and hence the
+        // containing task will already have an edge. (Here we are
+        // assuming each `FulfillmentContext` only gets used from one
+        // task; but to do otherwise makes no sense)
         !self.set.insert(key.clone())
     }
 }
 
+impl<'tcx> GlobalFulfilledPredicates<'tcx> {
+    pub fn new(dep_graph: DepGraph) -> GlobalFulfilledPredicates<'tcx> {
+        GlobalFulfilledPredicates {
+            set: FnvHashSet(),
+            dep_graph: dep_graph,
+        }
+    }
+
+    pub fn check_duplicate(&self, key: &ty::Predicate<'tcx>) -> bool {
+        if let ty::Predicate::Trait(ref data) = *key {
+            // For the global predicate registry, when we find a match, it
+            // may have been computed by some other task, so we want to
+            // add a read from the node corresponding to the predicate
+            // processing to make sure we get the transitive dependencies.
+            if self.set.contains(data) {
+                debug_assert!(data.is_global());
+                self.dep_graph.read(data.dep_node());
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    fn add_if_global(&mut self, key: &ty::Predicate<'tcx>) {
+        if let ty::Predicate::Trait(ref data) = *key {
+            // We only add things to the global predicate registry
+            // after the current task has proved them, and hence
+            // already has the required read edges, so we don't need
+            // to add any more edges here.
+            if data.is_global() {
+                self.set.insert(data.clone());
+            }
+        }
+    }
+}
+
 fn to_fulfillment_error<'tcx>(
     error: Error<PendingPredicateObligation<'tcx>, FulfillmentErrorCode<'tcx>>)
     -> FulfillmentError<'tcx>

src/librustc/middle/traits/mod.rs

@@ -15,12 +15,10 @@ pub use self::FulfillmentErrorCode::*;
 pub use self::Vtable::*;
 pub use self::ObligationCauseCode::*;
 
-use dep_graph::DepNode;
 use middle::def_id::DefId;
 use middle::free_region::FreeRegionMap;
 use middle::subst;
 use middle::ty::{self, Ty, TypeFoldable};
-use middle::ty::fast_reject;
 use middle::infer::{self, fixup_err_to_string, InferCtxt};
 
 use std::rc::Rc;
@@ -37,7 +35,7 @@ pub use self::error_reporting::report_object_safety_error;
 pub use self::coherence::orphan_check;
 pub use self::coherence::overlapping_impls;
 pub use self::coherence::OrphanCheckErr;
-pub use self::fulfill::{FulfillmentContext, FulfilledPredicates, RegionObligation};
+pub use self::fulfill::{FulfillmentContext, GlobalFulfilledPredicates, RegionObligation};
 pub use self::project::MismatchedProjectionTypes;
 pub use self::project::normalize;
 pub use self::project::Normalized;
@@ -617,18 +615,6 @@ impl<'tcx> FulfillmentError<'tcx> {
 }
 
 impl<'tcx> TraitObligation<'tcx> {
-    /// Creates the dep-node for selecting/evaluating this trait reference.
-    fn dep_node(&self, tcx: &ty::ctxt<'tcx>) -> DepNode {
-        let simplified_ty =
-            fast_reject::simplify_type(tcx,
-                                       self.predicate.skip_binder().self_ty(), // (*)
-                                       true);
-
-        // (*) skip_binder is ok because `simplify_type` doesn't care about regions
-
-        DepNode::TraitSelect(self.predicate.def_id(), simplified_ty)
-    }
-
     fn self_ty(&self) -> ty::Binder<Ty<'tcx>> {
         ty::Binder(self.predicate.skip_binder().self_ty())
     }

src/librustc/middle/traits/select.rs

@@ -307,7 +307,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         debug!("select({:?})", obligation);
         assert!(!obligation.predicate.has_escaping_regions());
 
-        let dep_node = obligation.dep_node(self.tcx());
+        let dep_node = obligation.predicate.dep_node();
         let _task = self.tcx().dep_graph.in_task(dep_node);
 
         let stack = self.push_stack(TraitObligationStackList::empty(), obligation);
@@ -462,7 +462,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         // have been proven elsewhere. This cache only contains
         // predicates that are global in scope and hence unaffected by
         // the current environment.
-        if self.tcx().fulfilled_predicates.borrow().is_duplicate(&obligation.predicate) {
+        if self.tcx().fulfilled_predicates.borrow().check_duplicate(&obligation.predicate) {
             return EvaluatedToOk;
         }
 

src/librustc/middle/ty/context.rs

@@ -367,7 +367,7 @@ pub struct ctxt<'tcx> {
     /// This is used to avoid duplicate work. Predicates are only
     /// added to this set when they mention only "global" names
     /// (i.e., no type or lifetime parameters).
-    pub fulfilled_predicates: RefCell<traits::FulfilledPredicates<'tcx>>,
+    pub fulfilled_predicates: RefCell<traits::GlobalFulfilledPredicates<'tcx>>,
 
     /// Caches the representation hints for struct definitions.
     repr_hint_cache: RefCell<DepTrackingMap<maps::ReprHints<'tcx>>>,
@@ -510,6 +510,7 @@ impl<'tcx> ctxt<'tcx> {
         let interner = RefCell::new(FnvHashMap());
         let common_types = CommonTypes::new(&arenas.type_, &interner);
         let dep_graph = DepGraph::new(s.opts.incremental_compilation);
+        let fulfilled_predicates = traits::GlobalFulfilledPredicates::new(dep_graph.clone());
         tls::enter(ctxt {
             arenas: arenas,
             interner: interner,
@@ -532,7 +533,7 @@ impl<'tcx> ctxt<'tcx> {
             adt_defs: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
             predicates: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
             super_predicates: RefCell::new(DepTrackingMap::new(dep_graph.clone())),
-            fulfilled_predicates: RefCell::new(traits::FulfilledPredicates::new()),
+            fulfilled_predicates: RefCell::new(fulfilled_predicates),
             map: map,
             freevars: RefCell::new(freevars),
             tcache: RefCell::new(DepTrackingMap::new(dep_graph.clone())),

src/librustc/middle/ty/mod.rs

@@ -838,6 +838,11 @@ impl<'tcx> TraitPredicate<'tcx> {
         self.trait_ref.def_id
     }
 
+    /// Creates the dep-node for selecting/evaluating this trait reference.
+    fn dep_node(&self) -> DepNode {
+        DepNode::TraitSelect(self.def_id())
+    }
+
     pub fn input_types(&self) -> &[Ty<'tcx>] {
         self.trait_ref.substs.types.as_slice()
     }
@@ -849,8 +854,14 @@ impl<'tcx> TraitPredicate<'tcx> {
 
 impl<'tcx> PolyTraitPredicate<'tcx> {
     pub fn def_id(&self) -> DefId {
+        // ok to skip binder since trait def-id does not care about regions
         self.0.def_id()
     }
+
+    pub fn dep_node(&self) -> DepNode {
+        // ok to skip binder since depnode does not care about regions
+        self.0.dep_node()
+    }
 }
 
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]

src/librustc_trans/trans/base.rs

@@ -3323,6 +3323,14 @@ impl<'a, 'tcx, 'v> Visitor<'v> for TransItemsWithinModVisitor<'a, 'tcx> {
                 // giving `trans_item` access to this item, so also record a read.
                 tcx.dep_graph.with_task(DepNode::TransCrateItem(def_id), || {
                     tcx.dep_graph.read(DepNode::Hir(def_id));
+
+                    // We are going to be accessing various tables
+                    // generated by TypeckItemBody; we also assume
+                    // that the body passes type check. These tables
+                    // are not individually tracked, so just register
+                    // a read here.
+                    tcx.dep_graph.read(DepNode::TypeckItemBody(def_id));
+
                     trans_item(self.ccx, i);
                 });
 

src/test/compile-fail/dep-graph-assoc-type-trans.rs

@@ -0,0 +1,48 @@
+// Copyright 2012-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.
+
+// Test that when a trait impl changes, fns whose body uses that trait
+// must also be recompiled.
+
+// compile-flags: -Z incr-comp
+
+#![feature(rustc_attrs)]
+#![allow(warnings)]
+
+fn main() { }
+
+pub trait Foo: Sized {
+    type T;
+    fn method(self) { }
+}
+
+mod x {
+    use Foo;
+
+    #[rustc_if_this_changed]
+    impl Foo for char { type T = char; }
+
+    impl Foo for u32 { type T = u32; }
+}
+
+mod y {
+    use Foo;
+
+    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR OK
+    #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR OK
+    pub fn use_char_assoc() {
+        // Careful here: in the representation, <char as Foo>::T gets
+        // normalized away, so at a certain point we had no edge to
+        // trans.  (But now trans just depends on typeck.)
+        let x: <char as Foo>::T = 'a';
+    }
+
+    pub fn take_foo<T:Foo>(t: T) { }
+}

src/test/compile-fail/dep-graph-trait-impl.rs

@@ -40,9 +40,8 @@ mod y {
         char::method('a');
     }
 
-    // FIXME(#30741) tcx fulfillment cache not tracked
-    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR no path
-    #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR no path
+    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR OK
+    #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR OK
     pub fn take_foo_with_char() {
         take_foo::<char>('a');
     }
@@ -53,9 +52,8 @@ mod y {
         u32::method(22);
     }
 
-    // FIXME(#30741) tcx fulfillment cache not tracked
-    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR no path
-    #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR no path
+    #[rustc_then_this_would_need(TypeckItemBody)] //~ ERROR OK
+    #[rustc_then_this_would_need(TransCrateItem)] //~ ERROR OK
     pub fn take_foo_with_u32() {
         take_foo::<u32>(22);
     }