Change how dep-graph edges are handled in variance to

be more fine-grained, fixing the `dep-graph-struct-signature`
test.

Author: nikomatsakis

src/librustc_typeck/variance/README.md

@@ -93,6 +93,54 @@ failure, but rather because the target type `Foo<Y>` is itself just
 not well-formed. Basically we get to assume well-formedness of all
 types involved before considering variance.
 
+#### Dependency graph management
+
+Because variance works in two phases, if we are not careful, we wind
+up with a muddled mess of a dep-graph. Basically, when gathering up
+the constraints, things are fairly well-structured, but then we do a
+fixed-point iteration and write the results back where they
+belong. You can't give this fixed-point iteration a single task
+because it reads from (and writes to) the variance of all types in the
+crate. In principle, we *could* switch the "current task" in a very
+fine-grained way while propagating constraints in the fixed-point
+iteration and everything would be automatically tracked, but that
+would add some overhead and isn't really necessary anyway.
+
+Instead what we do is to add edges into the dependency graph as we
+construct the constraint set: so, if computing the constraints for
+node `X` requires loading the inference variables from node `Y`, then
+we can add an edge `Y -> X`, since the variance we ultimately infer
+for `Y` will affect the variance we ultimately infer for `X`.
+
+At this point, we've basically mirrored the inference graph in the
+dependency graph. This means we can just completely ignore the
+fixed-point iteration, since it is just shuffling values along this
+graph. In other words, if we added the fine-grained switching of tasks
+I described earlier, all it would show is that we repeatedly read the
+values described by the constraints, but those edges were already
+added when building the constraints in the first place.
+
+Here is how this is implemented (at least as of the time of this
+writing). The associated `DepNode` for the variance map is (at least
+presently) `Signature(DefId)`. This means that, in `constraints.rs`,
+when we visit an item to load up its constraints, we set
+`Signature(DefId)` as the current task (the "memoization" pattern
+described in the `dep-graph` README). Then whenever we find an
+embedded type or trait, we add a synthetic read of `Signature(DefId)`,
+which covers the variances we will compute for all of its
+parameters. This read is synthetic (i.e., we call
+`variance_map.read()`) because, in fact, the final variance is not yet
+computed -- the read *will* occur (repeatedly) during the fixed-point
+iteration phase.
+
+In fact, we don't really *need* this synthetic read. That's because we
+do wind up looking up the `TypeScheme` or `TraitDef` for all
+references types/traits, and those reads add an edge from
+`Signature(DefId)` (that is, they share the same dep node as
+variance). However, I've kept the synthetic reads in place anyway,
+just for future-proofing (in case we change the dep-nodes in the
+future), and because it makes the intention a bit clearer I think.
+
 ### Addendum: Variance on traits
 
 As mentioned above, we used to permit variance on traits. This was
@@ -250,3 +298,5 @@ validly derived as `&'a ()` for any `'a`:
 This change otoh means that `<'static () as Identity>::Out` is
 always `&'static ()` (which might then be upcast to `'a ()`,
 separately). This was helpful in solving #21750.
+
+

src/librustc_typeck/variance/constraints.rs

@@ -13,11 +13,13 @@
 //! The second pass over the AST determines the set of constraints.
 //! We walk the set of items and, for each member, generate new constraints.
 
+use dep_graph::DepTrackingMapConfig;
 use middle::def_id::DefId;
 use middle::resolve_lifetime as rl;
 use middle::subst;
 use middle::subst::ParamSpace;
 use middle::ty::{self, Ty};
+use middle::ty::maps::ItemVariances;
 use rustc::front::map as hir_map;
 use syntax::ast;
 use rustc_front::hir;
@@ -48,10 +50,10 @@ pub struct Constraint<'a> {
     pub variance: &'a VarianceTerm<'a>,
 }
 
-pub fn add_constraints_from_crate<'a, 'tcx>(terms_cx: TermsContext<'a, 'tcx>,
-                                            krate: &hir::Crate)
+pub fn add_constraints_from_crate<'a, 'tcx>(terms_cx: TermsContext<'a, 'tcx>)
                                             -> ConstraintContext<'a, 'tcx>
 {
+    let tcx = terms_cx.tcx;
     let covariant = terms_cx.arena.alloc(ConstantTerm(ty::Covariant));
     let contravariant = terms_cx.arena.alloc(ConstantTerm(ty::Contravariant));
     let invariant = terms_cx.arena.alloc(ConstantTerm(ty::Invariant));
@@ -64,7 +66,11 @@ pub fn add_constraints_from_crate<'a, 'tcx>(terms_cx: TermsContext<'a, 'tcx>,
         bivariant: bivariant,
         constraints: Vec::new(),
     };
-    krate.visit_all_items(&mut constraint_cx);
+
+    // See README.md for a discussion on dep-graph management.
+    tcx.visit_all_items_in_krate(|def_id| ItemVariances::to_dep_node(&def_id),
+                                 &mut constraint_cx);
+
     constraint_cx
 }
 
@@ -289,6 +295,11 @@ impl<'a, 'tcx> ConstraintContext<'a, 'tcx> {
 
         let trait_def = self.tcx().lookup_trait_def(trait_ref.def_id);
 
+        // This edge is actually implied by the call to
+        // `lookup_trait_def`, but I'm trying to be future-proof. See
+        // README.md for a discussion on dep-graph management.
+        self.tcx().dep_graph.read(ItemVariances::to_dep_node(&trait_ref.def_id));
+
         self.add_constraints_from_substs(
             generics,
             trait_ref.def_id,
@@ -345,6 +356,11 @@ impl<'a, 'tcx> ConstraintContext<'a, 'tcx> {
             ty::TyStruct(def, substs) => {
                 let item_type = self.tcx().lookup_item_type(def.did);
 
+                // This edge is actually implied by the call to
+                // `lookup_trait_def`, but I'm trying to be future-proof. See
+                // README.md for a discussion on dep-graph management.
+                self.tcx().dep_graph.read(ItemVariances::to_dep_node(&def.did));
+
                 // All type parameters on enums and structs should be
                 // in the TypeSpace.
                 assert!(item_type.generics.types.is_empty_in(subst::SelfSpace));
@@ -364,6 +380,12 @@ impl<'a, 'tcx> ConstraintContext<'a, 'tcx> {
             ty::TyProjection(ref data) => {
                 let trait_ref = &data.trait_ref;
                 let trait_def = self.tcx().lookup_trait_def(trait_ref.def_id);
+
+                // This edge is actually implied by the call to
+                // `lookup_trait_def`, but I'm trying to be future-proof. See
+                // README.md for a discussion on dep-graph management.
+                self.tcx().dep_graph.read(ItemVariances::to_dep_node(&trait_ref.def_id));
+
                 self.add_constraints_from_substs(
                     generics,
                     trait_ref.def_id,
@@ -424,7 +446,6 @@ impl<'a, 'tcx> ConstraintContext<'a, 'tcx> {
         }
     }
 
-
     /// Adds constraints appropriate for a nominal type (enum, struct,
     /// object, etc) appearing in a context with ambient variance `variance`
     fn add_constraints_from_substs(&mut self,

src/librustc_typeck/variance/mod.rs

@@ -12,7 +12,6 @@
 //! parameters. See README.md for details.
 
 use arena;
-use dep_graph::DepNode;
 use middle::ty;
 
 /// Defines the `TermsContext` basically houses an arena where we can
@@ -29,11 +28,9 @@ mod solve;
 mod xform;
 
 pub fn infer_variance(tcx: &ty::ctxt) {
-    let _task = tcx.dep_graph.in_task(DepNode::Variance);
-    let krate = tcx.map.krate();
     let mut arena = arena::TypedArena::new();
-    let terms_cx = terms::determine_parameters_to_be_inferred(tcx, &mut arena, krate);
-    let constraints_cx = constraints::add_constraints_from_crate(terms_cx, krate);
+    let terms_cx = terms::determine_parameters_to_be_inferred(tcx, &mut arena);
+    let constraints_cx = constraints::add_constraints_from_crate(terms_cx);
     solve::solve_constraints(constraints_cx);
     tcx.variance_computed.set(true);
 }

src/librustc_typeck/variance/solve.rs

@@ -96,6 +96,13 @@ impl<'a, 'tcx> SolveContext<'a, 'tcx> {
         // item id).
 
         let tcx = self.terms_cx.tcx;
+
+        // Ignore the writes here because the relevant edges were
+        // already accounted for in `constraints.rs`. See the section
+        // on dependency graph management in README.md for more
+        // information.
+        let _ignore = tcx.dep_graph.in_ignore();
+
         let solutions = &self.solutions;
         let inferred_infos = &self.terms_cx.inferred_infos;
         let mut index = 0;

src/librustc_typeck/variance/terms.rs

@@ -20,8 +20,10 @@
 // a variable.
 
 use arena::TypedArena;
+use dep_graph::DepTrackingMapConfig;
 use middle::subst::{ParamSpace, FnSpace, TypeSpace, SelfSpace, VecPerParamSpace};
 use middle::ty;
+use middle::ty::maps::ItemVariances;
 use std::fmt;
 use std::rc::Rc;
 use syntax::ast;
@@ -97,8 +99,7 @@ pub struct InferredInfo<'a> {
 
 pub fn determine_parameters_to_be_inferred<'a, 'tcx>(
     tcx: &'a ty::ctxt<'tcx>,
-    arena: &'a mut TypedArena<VarianceTerm<'a>>,
-    krate: &hir::Crate)
+    arena: &'a mut TypedArena<VarianceTerm<'a>>)
     -> TermsContext<'a, 'tcx>
 {
     let mut terms_cx = TermsContext {
@@ -117,7 +118,9 @@ pub fn determine_parameters_to_be_inferred<'a, 'tcx>(
         })
     };
 
-    krate.visit_all_items(&mut terms_cx);
+    // See README.md for a discussion on dep-graph management.
+    tcx.visit_all_items_in_krate(|def_id| ItemVariances::to_dep_node(&def_id),
+                                 &mut terms_cx);
 
     terms_cx
 }

src/test/compile-fail/dep-graph-struct-signature.rs

@@ -74,23 +74,17 @@ mod signatures {
     fn indirect(x: WillChanges) { }
 }
 
-// these are invalid dependencies, though sometimes we create edges
-// anyway.
 mod invalid_signatures {
     use WontChange;
 
-    // FIXME due to the variance pass having overly conservative edges,
-    // we incorrectly think changes are needed here
-    #[rustc_then_this_would_need(ItemSignature)] //~ ERROR OK
-    #[rustc_then_this_would_need(CollectItem)] //~ ERROR OK
+    #[rustc_then_this_would_need(ItemSignature)] //~ ERROR no path
+    #[rustc_then_this_would_need(CollectItem)] //~ ERROR no path
     trait A {
         fn do_something_else_twice(x: WontChange);
     }
 
-    // FIXME due to the variance pass having overly conservative edges,
-    // we incorrectly think changes are needed here
-    #[rustc_then_this_would_need(ItemSignature)] //~ ERROR OK
-    #[rustc_then_this_would_need(CollectItem)] //~ ERROR OK
+    #[rustc_then_this_would_need(ItemSignature)] //~ ERROR no path
+    #[rustc_then_this_would_need(CollectItem)] //~ ERROR no path
     fn b(x: WontChange) { }
 
     #[rustc_then_this_would_need(ItemSignature)] //~ ERROR no path from `WillChange`