Auto merge of #31087 - nikomatsakis:incr-comp-fulfillment-cache, r=arielb1

This is a fix for #30741. It simplifies dep-graph tracking for trait matching. I was experimenting with having a greater resolution here, but decided to pare back to just have one dep node for "trait resolutions on trait `Foo`", which means that adding an impl to the trait `Foo` will invalidate all fns that had to do any trait matching at all on `Foo`. This seems like a reasonable starting place.

Independently, I realized I had neglected to record a dependency from trans on typeck -- this is obviously needed, since trans consumes a bunch of data structures that typeck produces (but which are not currently individually tracked) -- and because trans assumes that typeck has been done. Eventually those are going to go away and be replaced with MIR, which will be tracked, so this edge would presumably be derived automatically then, but it's an obvious enough thing to want for now.

r? @arielb1

cc @michaelwoerister -- this might indirectly fix the problem you observed with the trans cache, though it'd be nice to try and craft an independent test case for that.

Author: bors

Diff for: 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)]

Diff for: 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>

Diff for: 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())
     }

Diff for: 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;
         }
 

Diff for: 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())),

Diff for: 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)]

Diff for: 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);
                 });
 

Diff for: 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) { }
+}

Diff for: 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);
     }