From de06faf889f0939c7d9b24aeb724b46403b8dba4 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 22 Oct 2014 11:35:53 -0400 Subject: [PATCH 1/2] Use local cache when there are unbound type variables and where clauses in scope. Fixes #18209. --- src/librustc/middle/traits/doc.rs | 128 +++++++++++++++++++ src/librustc/middle/traits/select.rs | 51 ++++---- src/librustc/middle/ty.rs | 65 +++++----- src/test/run-pass/trait-cache-issue-18209.rs | 27 ++++ 4 files changed, 212 insertions(+), 59 deletions(-) create mode 100644 src/test/run-pass/trait-cache-issue-18209.rs diff --git a/src/librustc/middle/traits/doc.rs b/src/librustc/middle/traits/doc.rs index f24121d9a3a5f..a8fcdb360546b 100644 --- a/src/librustc/middle/traits/doc.rs +++ b/src/librustc/middle/traits/doc.rs @@ -279,4 +279,132 @@ selection. This is because it must account for the transformed self type of the receiver and various other complications. The procedure is described in `select.rs` in the "METHOD MATCHING" section. +# Caching and subtle considerations therewith + +In general we attempt to cache the results of trait selection. This +is a somewhat complex process. Part of the reason for this is that we +want to be able to cache results even when all the types in the trait +reference are not fully known. In that case, it may happen that the +trait selection process is also influencing type variables, so we have +to be able to not only cache the *result* of the selection process, +but *reply* its effects on the type variables. + +## An example + +The high-level idea of how the cache works is that we first replace +all unbound inference variables with skolemized versions. Therefore, +if we had a trait reference `uint : Foo<$1>`, where `$n` is an unbound +inference variable, we might replace it with `uint : Foo<%0>`, where +`%n` is a skolemized type. We would then look this up in the cache. +If we found a hit, the hit would tell us the immediate next step to +take in the selection process: i.e., apply impl #22, or apply where +clause `X : Foo`. Let's say in this case there is no hit. +Therefore, we search through impls and where clauses and so forth, and +we come to the conclusion that the only possible impl is this one, +with def-id 22: + + impl Foo for uint { ... } // Impl #22 + +We would then record in the cache `uint : Foo<%0> ==> +ImplCandidate(22)`. Next we would confirm `ImplCandidate(22)`, which +would (as a side-effect) unify `$1` with `int`. + +Now, at some later time, we might come along and see a `uint : +Foo<$3>`. When skolemized, this would yield `uint : Foo<%0>`, just as +before, and hence the cache lookup would succeed, yielding +`ImplCandidate(22)`. We would confirm `ImplCandidate(22)` which would +(as a side-effect) unify `$3` with `int`. + +## Where clauses and the local vs global cache + +One subtle interaction is that the results of trait lookup will vary +depending on what where clauses are in scope. Therefore, we actually +have *two* caches, a local and a global cache. The local cache is +attached to the `ParameterEnvironment` and the global cache attached +to the `tcx`. We use the local cache whenever the result might depend +on the where clauses that are in scope. The determination of which +cache to use is done by the method `pick_candidate_cache` in +`select.rs`. + +There are two cases where we currently use the local cache. The +current rules are probably more conservative than necessary. + +### Trait references that involve parameter types + +The most obvious case where you need the local environment is +when the trait reference includes parameter types. For example, +consider the following function: + + impl Vec { + fn foo(x: T) + where T : Foo + { ... } + + fn bar(x: T) + { ... } + } + +If there is an obligation `T : Foo`, or `int : Bar`, or whatever, +clearly the results from `foo` and `bar` are potentially different, +since the set of where clauses in scope are different. + +### Trait references with unbound variables when where clauses are in scope + +There is another less obvious interaction which involves unbound variables +where *only* where clauses are in scope (no impls). This manifested as +issue #18209 (`run-pass/trait-cache-issue-18209.rs`). Consider +this snippet: + +``` +pub trait Foo { + fn load_from() -> Box; + fn load() -> Box { + Foo::load_from() + } +} +``` + +The default method will incur an obligation `$0 : Foo` from the call +to `load_from`. If there are no impls, this can be eagerly resolved to +`VtableParam(Self : Foo)` and cached. Because the trait reference +doesn't involve any parameters types (only the resolution does), this +result was stored in the global cache, causing later calls to +`Foo::load_from()` to get nonsense. + +To fix this, we always use the local cache if there are unbound +variables and where clauses in scope. This is more conservative than +necessary as far as I can tell. However, it still seems to be a simple +rule and I observe ~99% hit rate on rustc, so it doesn't seem to hurt +us in particular. + +Here is an example of the kind of subtle case that I would be worried +about with a more complex rule (although this particular case works +out ok). Imagine the trait reference doesn't directly reference a +where clause, but the where clause plays a role in the winnowing +phase. Something like this: + +``` +pub trait Foo { ... } +pub trait Bar { ... } +impl Foo for T { ... } // Impl A +impl Foo for uint { ... } // Impl B +``` + +Now, in some function, we have no where clauses in scope, and we have +an obligation `$1 : Foo<$0>`. We might then conclude that `$0=char` +and `$1=uint`: this is because for impl A to apply, `uint:Bar` would +have to hold, and we know it does not or else the coherence check +would have failed. So we might enter into our global cache: `$1 : +Foo<$0> => Impl B`. Then we come along in a different scope, where a +generic type `A` is around with the bound `A:Bar`. Now suddenly the +impl is viable. + +The flaw in this imaginary DOOMSDAY SCENARIO is that we would not +currently conclude that `$1 : Foo<$0>` implies that `$0 == uint` and +`$1 == char`, even though it is true that (absent type parameters) +there is no other type the user could enter. However, it is not +*completely* implausible that we *could* draw this conclusion in the +future; we wouldn't have to guess types, in particular, we could be +led by the impls. + */ diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index f923cf1e5903b..aa183dabaa018 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -844,19 +844,36 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { cache_skol_trait_ref: &Rc) -> &SelectionCache { + // High-level idea: we have to decide whether to consult the + // cache that is specific to this scope, or to consult the + // global cache. We want the cache that is specific to this + // scope whenever where clauses might affect the result. + // If the trait refers to any parameters in scope, then use - // the cache of the param-environment. This is because the - // result will depend on the where clauses that are in - // scope. Otherwise, use the generic tcx cache, since the - // result holds across all environments. + // the cache of the param-environment. if cache_skol_trait_ref.input_types().iter().any( |&t| ty::type_has_self(t) || ty::type_has_params(t)) { - &self.param_env.selection_cache - } else { - &self.tcx().selection_cache + return &self.param_env.selection_cache; } + + // If the trait refers to unbound type variables, and there + // are where clauses in scope, then use the local environment. + // If there are no where clauses in scope, which is a very + // common case, then we can use the global environment. + // See the discussion in doc.rs for more details. + if + !self.param_env.caller_obligations.is_empty() + && + cache_skol_trait_ref.input_types().iter().any( + |&t| ty::type_has_ty_infer(t)) + { + return &self.param_env.selection_cache; + } + + // Otherwise, we can use the global cache. + &self.tcx().selection_cache } fn check_candidate_cache(&mut self, @@ -1935,26 +1952,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { util::obligations_for_generics(self.tcx(), cause, recursion_depth, &impl_generics, impl_substs) } - - fn contains_skolemized_types(&self, - ty: ty::t) - -> bool - { - /*! - * True if the type contains skolemized variables. - */ - - let mut found_skol = false; - - ty::walk_ty(ty, |t| { - match ty::get(t).sty { - ty::ty_infer(ty::SkolemizedTy(_)) => { found_skol = true; } - _ => { } - } - }); - - found_skol - } } impl Repr for Candidate { diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index a7ce93279bd83..52ec97ab647be 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -585,18 +585,18 @@ pub struct ctxt<'tcx> { pub repr_hint_cache: RefCell>>>, } -pub enum tbox_flag { - has_params = 1, - has_self = 2, - needs_infer = 4, - has_regions = 8, - has_ty_err = 16, - has_ty_bot = 32, - - // a meta-pub flag: subst may be required if the type has parameters, a self - // type, or references bound regions - needs_subst = 1 | 2 | 8 -} +// Flags that we track on types. These flags are propagated upwards +// through the type during type construction, so that we can quickly +// check whether the type has various kinds of types in it without +// recursing over the type itself. +const HAS_PARAMS: uint = 1; +const HAS_SELF: uint = 2; +const HAS_TY_INFER: uint = 4; +const HAS_RE_INFER: uint = 8; +const HAS_REGIONS: uint = 16; +const HAS_TY_ERR: uint = 32; +const HAS_TY_BOT: uint = 64; +const NEEDS_SUBST: uint = HAS_PARAMS | HAS_SELF | HAS_REGIONS; pub type t_box = &'static t_box_; @@ -631,15 +631,16 @@ pub fn get(t: t) -> t_box { } } -pub fn tbox_has_flag(tb: t_box, flag: tbox_flag) -> bool { - (tb.flags & (flag as uint)) != 0u +fn tbox_has_flag(tb: t_box, flag: uint) -> bool { + (tb.flags & flag) != 0u } pub fn type_has_params(t: t) -> bool { - tbox_has_flag(get(t), has_params) + tbox_has_flag(get(t), HAS_PARAMS) } -pub fn type_has_self(t: t) -> bool { tbox_has_flag(get(t), has_self) } +pub fn type_has_self(t: t) -> bool { tbox_has_flag(get(t), HAS_SELF) } +pub fn type_has_ty_infer(t: t) -> bool { tbox_has_flag(get(t), HAS_TY_INFER) } pub fn type_needs_infer(t: t) -> bool { - tbox_has_flag(get(t), needs_infer) + tbox_has_flag(get(t), HAS_TY_INFER | HAS_RE_INFER) } pub fn type_id(t: t) -> uint { get(t).id } @@ -910,13 +911,13 @@ mod primitives { pub static TY_BOT: t_box_ = t_box_ { sty: super::ty_bot, id: 16, - flags: super::has_ty_bot as uint, + flags: super::HAS_TY_BOT, }; pub static TY_ERR: t_box_ = t_box_ { sty: super::ty_err, id: 17, - flags: super::has_ty_err as uint, + flags: super::HAS_TY_ERR, }; pub const LAST_PRIMITIVE_ID: uint = 18; @@ -1579,9 +1580,9 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { let mut flags = 0u; fn rflags(r: Region) -> uint { - (has_regions as uint) | { + HAS_REGIONS | { match r { - ty::ReInfer(_) => needs_infer as uint, + ty::ReInfer(_) => HAS_RE_INFER, _ => 0u } } @@ -1610,22 +1611,22 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { &ty_str => {} // You might think that we could just return ty_err for // any type containing ty_err as a component, and get - // rid of the has_ty_err flag -- likewise for ty_bot (with + // rid of the HAS_TY_ERR flag -- likewise for ty_bot (with // the exception of function types that return bot). // But doing so caused sporadic memory corruption, and // neither I (tjc) nor nmatsakis could figure out why, // so we're doing it this way. - &ty_bot => flags |= has_ty_bot as uint, - &ty_err => flags |= has_ty_err as uint, + &ty_bot => flags |= HAS_TY_BOT, + &ty_err => flags |= HAS_TY_ERR, &ty_param(ref p) => { if p.space == subst::SelfSpace { - flags |= has_self as uint; + flags |= HAS_SELF; } else { - flags |= has_params as uint; + flags |= HAS_PARAMS; } } &ty_unboxed_closure(_, ref region) => flags |= rflags(*region), - &ty_infer(_) => flags |= needs_infer as uint, + &ty_infer(_) => flags |= HAS_TY_INFER, &ty_enum(_, ref substs) | &ty_struct(_, ref substs) => { flags |= sflags(substs); } @@ -1648,7 +1649,7 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { for a in f.sig.inputs.iter() { flags |= get(*a).flags; } flags |= get(f.sig.output).flags; // T -> _|_ is *not* _|_ ! - flags &= !(has_ty_bot as uint); + flags &= !HAS_TY_BOT; } &ty_closure(ref f) => { match f.store { @@ -1660,7 +1661,7 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { for a in f.sig.inputs.iter() { flags |= get(*a).flags; } flags |= get(f.sig.output).flags; // T -> _|_ is *not* _|_ ! - flags &= !(has_ty_bot as uint); + flags &= !HAS_TY_BOT; flags |= flags_for_bounds(&f.bounds); } } @@ -1979,15 +1980,15 @@ impl ItemSubsts { pub fn type_is_nil(ty: t) -> bool { get(ty).sty == ty_nil } pub fn type_is_bot(ty: t) -> bool { - (get(ty).flags & (has_ty_bot as uint)) != 0 + (get(ty).flags & HAS_TY_BOT) != 0 } pub fn type_is_error(ty: t) -> bool { - (get(ty).flags & (has_ty_err as uint)) != 0 + (get(ty).flags & HAS_TY_ERR) != 0 } pub fn type_needs_subst(ty: t) -> bool { - tbox_has_flag(get(ty), needs_subst) + tbox_has_flag(get(ty), NEEDS_SUBST) } pub fn trait_ref_contains_error(tref: &ty::TraitRef) -> bool { diff --git a/src/test/run-pass/trait-cache-issue-18209.rs b/src/test/run-pass/trait-cache-issue-18209.rs new file mode 100644 index 0000000000000..a5efb32079de8 --- /dev/null +++ b/src/test/run-pass/trait-cache-issue-18209.rs @@ -0,0 +1,27 @@ +// Copyright 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test that the cache results from the default method do not pollute +// the cache for the later call in `load()`. +// +// See issue #18209. + +pub trait Foo { + fn load_from() -> Box; + fn load() -> Box { + Foo::load_from() + } +} + +pub fn load() -> Box { + Foo::load() +} + +fn main() { } From 4a8d712345f30cba4f33bce9e0ece0eac64e8764 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 24 Oct 2014 10:20:02 -0400 Subject: [PATCH 2/2] Use type-safe wrapper for TypeFlags --- src/librustc/middle/traits/doc.rs | 2 +- src/librustc/middle/ty.rs | 103 +++++++++++++++++------------- 2 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/librustc/middle/traits/doc.rs b/src/librustc/middle/traits/doc.rs index a8fcdb360546b..c014bc0c164f2 100644 --- a/src/librustc/middle/traits/doc.rs +++ b/src/librustc/middle/traits/doc.rs @@ -287,7 +287,7 @@ want to be able to cache results even when all the types in the trait reference are not fully known. In that case, it may happen that the trait selection process is also influencing type variables, so we have to be able to not only cache the *result* of the selection process, -but *reply* its effects on the type variables. +but *replay* its effects on the type variables. ## An example diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 52ec97ab647be..4a081c0db3733 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -589,14 +589,19 @@ pub struct ctxt<'tcx> { // through the type during type construction, so that we can quickly // check whether the type has various kinds of types in it without // recursing over the type itself. -const HAS_PARAMS: uint = 1; -const HAS_SELF: uint = 2; -const HAS_TY_INFER: uint = 4; -const HAS_RE_INFER: uint = 8; -const HAS_REGIONS: uint = 16; -const HAS_TY_ERR: uint = 32; -const HAS_TY_BOT: uint = 64; -const NEEDS_SUBST: uint = HAS_PARAMS | HAS_SELF | HAS_REGIONS; +bitflags! { + flags TypeFlags: u32 { + const NO_TYPE_FLAGS = 0b0, + const HAS_PARAMS = 0b1, + const HAS_SELF = 0b10, + const HAS_TY_INFER = 0b100, + const HAS_RE_INFER = 0b1000, + const HAS_REGIONS = 0b10000, + const HAS_TY_ERR = 0b100000, + const HAS_TY_BOT = 0b1000000, + const NEEDS_SUBST = HAS_PARAMS.bits | HAS_SELF.bits | HAS_REGIONS.bits, + } +} pub type t_box = &'static t_box_; @@ -604,7 +609,13 @@ pub type t_box = &'static t_box_; pub struct t_box_ { pub sty: sty, pub id: uint, - pub flags: uint, + pub flags: TypeFlags, +} + +impl fmt::Show for TypeFlags { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.bits) + } } // To reduce refcounting cost, we're representing types as unsafe pointers @@ -631,8 +642,8 @@ pub fn get(t: t) -> t_box { } } -fn tbox_has_flag(tb: t_box, flag: uint) -> bool { - (tb.flags & flag) != 0u +fn tbox_has_flag(tb: t_box, flag: TypeFlags) -> bool { + tb.flags.intersects(flag) } pub fn type_has_params(t: t) -> bool { tbox_has_flag(get(t), HAS_PARAMS) @@ -887,7 +898,7 @@ mod primitives { pub static $name: t_box_ = t_box_ { sty: $sty, id: $id, - flags: 0, + flags: super::NO_TYPE_FLAGS, }; ) ) @@ -1578,32 +1589,32 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { _ => () } - let mut flags = 0u; - fn rflags(r: Region) -> uint { + let mut flags = NO_TYPE_FLAGS; + fn rflags(r: Region) -> TypeFlags { HAS_REGIONS | { match r { ty::ReInfer(_) => HAS_RE_INFER, - _ => 0u + _ => NO_TYPE_FLAGS, } } } - fn sflags(substs: &Substs) -> uint { - let mut f = 0u; + fn sflags(substs: &Substs) -> TypeFlags { + let mut f = NO_TYPE_FLAGS; let mut i = substs.types.iter(); for tt in i { - f |= get(*tt).flags; + f = f | get(*tt).flags; } match substs.regions { subst::ErasedRegions => {} subst::NonerasedRegions(ref regions) => { for r in regions.iter() { - f |= rflags(*r) + f = f | rflags(*r) } } } return f; } - fn flags_for_bounds(bounds: &ExistentialBounds) -> uint { + fn flags_for_bounds(bounds: &ExistentialBounds) -> TypeFlags { rflags(bounds.region_bound) } match &st { @@ -1616,53 +1627,53 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { // But doing so caused sporadic memory corruption, and // neither I (tjc) nor nmatsakis could figure out why, // so we're doing it this way. - &ty_bot => flags |= HAS_TY_BOT, - &ty_err => flags |= HAS_TY_ERR, + &ty_bot => flags = flags | HAS_TY_BOT, + &ty_err => flags = flags | HAS_TY_ERR, &ty_param(ref p) => { if p.space == subst::SelfSpace { - flags |= HAS_SELF; + flags = flags | HAS_SELF; } else { - flags |= HAS_PARAMS; + flags = flags | HAS_PARAMS; } } - &ty_unboxed_closure(_, ref region) => flags |= rflags(*region), - &ty_infer(_) => flags |= HAS_TY_INFER, + &ty_unboxed_closure(_, ref region) => flags = flags | rflags(*region), + &ty_infer(_) => flags = flags | HAS_TY_INFER, &ty_enum(_, ref substs) | &ty_struct(_, ref substs) => { - flags |= sflags(substs); + flags = flags | sflags(substs); } &ty_trait(box TyTrait { ref substs, ref bounds, .. }) => { - flags |= sflags(substs); - flags |= flags_for_bounds(bounds); + flags = flags | sflags(substs); + flags = flags | flags_for_bounds(bounds); } &ty_uniq(tt) | &ty_vec(tt, _) | &ty_open(tt) => { - flags |= get(tt).flags + flags = flags | get(tt).flags } &ty_ptr(ref m) => { - flags |= get(m.ty).flags; + flags = flags | get(m.ty).flags; } &ty_rptr(r, ref m) => { - flags |= rflags(r); - flags |= get(m.ty).flags; + flags = flags | rflags(r); + flags = flags | get(m.ty).flags; } - &ty_tup(ref ts) => for tt in ts.iter() { flags |= get(*tt).flags; }, + &ty_tup(ref ts) => for tt in ts.iter() { flags = flags | get(*tt).flags; }, &ty_bare_fn(ref f) => { - for a in f.sig.inputs.iter() { flags |= get(*a).flags; } - flags |= get(f.sig.output).flags; + for a in f.sig.inputs.iter() { flags = flags | get(*a).flags; } + flags = flags | get(f.sig.output).flags; // T -> _|_ is *not* _|_ ! - flags &= !HAS_TY_BOT; + flags = flags - HAS_TY_BOT; } &ty_closure(ref f) => { match f.store { RegionTraitStore(r, _) => { - flags |= rflags(r); + flags = flags | rflags(r); } _ => {} } - for a in f.sig.inputs.iter() { flags |= get(*a).flags; } - flags |= get(f.sig.output).flags; + for a in f.sig.inputs.iter() { flags = flags | get(*a).flags; } + flags = flags | get(f.sig.output).flags; // T -> _|_ is *not* _|_ ! - flags &= !HAS_TY_BOT; - flags |= flags_for_bounds(&f.bounds); + flags = flags - HAS_TY_BOT; + flags = flags | flags_for_bounds(&f.bounds); } } @@ -1977,14 +1988,16 @@ impl ItemSubsts { // Type utilities -pub fn type_is_nil(ty: t) -> bool { get(ty).sty == ty_nil } +pub fn type_is_nil(ty: t) -> bool { + get(ty).sty == ty_nil +} pub fn type_is_bot(ty: t) -> bool { - (get(ty).flags & HAS_TY_BOT) != 0 + get(ty).flags.intersects(HAS_TY_BOT) } pub fn type_is_error(ty: t) -> bool { - (get(ty).flags & HAS_TY_ERR) != 0 + get(ty).flags.intersects(HAS_TY_ERR) } pub fn type_needs_subst(ty: t) -> bool {