From 01ebc37fa160328c2dc580cf62cf6b2194dc6f2f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 3 Feb 2016 16:00:23 -0500 Subject: [PATCH] Change how dep-graph edges are handled in variance to be more fine-grained, fixing the `dep-graph-struct-signature` test. --- src/librustc_typeck/variance/README.md | 50 +++++++++++++++++++ src/librustc_typeck/variance/constraints.rs | 29 +++++++++-- src/librustc_typeck/variance/mod.rs | 7 +-- src/librustc_typeck/variance/solve.rs | 7 +++ src/librustc_typeck/variance/terms.rs | 9 ++-- .../dep-graph-struct-signature.rs | 14 ++---- 6 files changed, 94 insertions(+), 22 deletions(-) diff --git a/src/librustc_typeck/variance/README.md b/src/librustc_typeck/variance/README.md index 4a4b070a53e..94d1ff91c37 100644 --- a/src/librustc_typeck/variance/README.md +++ b/src/librustc_typeck/variance/README.md @@ -93,6 +93,54 @@ failure, but rather because the target type `Foo` 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. + + diff --git a/src/librustc_typeck/variance/constraints.rs b/src/librustc_typeck/variance/constraints.rs index db54e80b9a6..2f243d0fd0f 100644 --- a/src/librustc_typeck/variance/constraints.rs +++ b/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, diff --git a/src/librustc_typeck/variance/mod.rs b/src/librustc_typeck/variance/mod.rs index d400ec059d9..3ce3a868f04 100644 --- a/src/librustc_typeck/variance/mod.rs +++ b/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); } diff --git a/src/librustc_typeck/variance/solve.rs b/src/librustc_typeck/variance/solve.rs index 2cfad597ed7..fd442a4547c 100644 --- a/src/librustc_typeck/variance/solve.rs +++ b/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; diff --git a/src/librustc_typeck/variance/terms.rs b/src/librustc_typeck/variance/terms.rs index 7d4a296467b..aa1e93c3d6b 100644 --- a/src/librustc_typeck/variance/terms.rs +++ b/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>, - krate: &hir::Crate) + arena: &'a mut TypedArena>) -> 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 } diff --git a/src/test/compile-fail/dep-graph-struct-signature.rs b/src/test/compile-fail/dep-graph-struct-signature.rs index 5cfb748b6f4..c16998cd33c 100644 --- a/src/test/compile-fail/dep-graph-struct-signature.rs +++ b/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`