diff --git a/src/librustc/middle/traits/doc.rs b/src/librustc/middle/traits/doc.rs index f24121d9a3a5f..c014bc0c164f2 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 *replay* 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..4a081c0db3733 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -585,17 +585,22 @@ 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. +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,15 +642,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: TypeFlags) -> bool { + tb.flags.intersects(flag) } 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 } @@ -886,7 +898,7 @@ mod primitives { pub static $name: t_box_ = t_box_ { sty: $sty, id: $id, - flags: 0, + flags: super::NO_TYPE_FLAGS, }; ) ) @@ -910,13 +922,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; @@ -1577,32 +1589,32 @@ pub fn mk_t(cx: &ctxt, st: sty) -> t { _ => () } - let mut flags = 0u; - fn rflags(r: Region) -> uint { - (has_regions as uint) | { + let mut flags = NO_TYPE_FLAGS; + fn rflags(r: Region) -> TypeFlags { + HAS_REGIONS | { match r { - ty::ReInfer(_) => needs_infer as uint, - _ => 0u + ty::ReInfer(_) => HAS_RE_INFER, + _ => 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 { @@ -1610,58 +1622,58 @@ 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 = flags | HAS_TY_BOT, + &ty_err => flags = flags | HAS_TY_ERR, &ty_param(ref p) => { if p.space == subst::SelfSpace { - flags |= has_self as uint; + flags = flags | HAS_SELF; } else { - flags |= has_params as uint; + flags = flags | HAS_PARAMS; } } - &ty_unboxed_closure(_, ref region) => flags |= rflags(*region), - &ty_infer(_) => flags |= needs_infer as uint, + &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 as uint); + 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 as uint); - flags |= flags_for_bounds(&f.bounds); + flags = flags - HAS_TY_BOT; + flags = flags | flags_for_bounds(&f.bounds); } } @@ -1976,18 +1988,20 @@ 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 as uint)) != 0 + get(ty).flags.intersects(HAS_TY_BOT) } pub fn type_is_error(ty: t) -> bool { - (get(ty).flags & (has_ty_err as uint)) != 0 + get(ty).flags.intersects(HAS_TY_ERR) } 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() { }