diff --git a/src/librustc/middle/traits/doc.rs b/src/librustc/middle/traits/doc.rs index d69f340ca17..7af046401ad 100644 --- a/src/librustc/middle/traits/doc.rs +++ b/src/librustc/middle/traits/doc.rs @@ -434,87 +434,11 @@ 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. +`select.rs`. At the moment, we use a very simple, conservative rule: +if there are any where-clauses in scope, then we use the local cache. +We used to try and draw finer-grained distinctions, but that led to a +serious of annoying and weird bugs like #22019 and #18290. This simple +rule seems to be pretty clearly safe and also still retains a very +high hit rate (~95% when compiling rustc). */ diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index 2ea16d55343..a04aa9c5607 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -705,14 +705,17 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { Ok(Some(candidate)) } - fn pick_candidate_cache(&self, - cache_fresh_trait_pred: &ty::PolyTraitPredicate<'tcx>) - -> &SelectionCache<'tcx> - { - // 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. + fn pick_candidate_cache(&self) -> &SelectionCache<'tcx> { + // If there are any where-clauses in scope, then we always use + // a cache local to this particular scope. Otherwise, we + // switch to a global cache. We used to try and draw + // finer-grained distinctions, but that led to a serious of + // annoying and weird bugs like #22019 and #18290. This simple + // rule seems to be pretty clearly safe and also still retains + // a very high hit rate (~95% when compiling rustc). + if !self.param_env().caller_bounds.is_empty() { + return &self.param_env().selection_cache; + } // Avoid using the master cache during coherence and just rely // on the local cache. This effectively disables caching @@ -725,28 +728,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { return &self.param_env().selection_cache; } - // If the trait refers to any parameters in scope, then use - // the cache of the param-environment. - if - cache_fresh_trait_pred.0.input_types().iter().any( - |&t| ty::type_has_self(t) || ty::type_has_params(t)) - { - 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_bounds.is_empty() && - cache_fresh_trait_pred.0.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 } @@ -755,7 +736,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { cache_fresh_trait_pred: &ty::PolyTraitPredicate<'tcx>) -> Option>> { - let cache = self.pick_candidate_cache(cache_fresh_trait_pred); + let cache = self.pick_candidate_cache(); let hashmap = cache.hashmap.borrow(); hashmap.get(&cache_fresh_trait_pred.0.trait_ref).map(|c| (*c).clone()) } @@ -764,7 +745,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { cache_fresh_trait_pred: ty::PolyTraitPredicate<'tcx>, candidate: SelectionResult<'tcx, SelectionCandidate<'tcx>>) { - let cache = self.pick_candidate_cache(&cache_fresh_trait_pred); + let cache = self.pick_candidate_cache(); let mut hashmap = cache.hashmap.borrow_mut(); hashmap.insert(cache_fresh_trait_pred.0.trait_ref.clone(), candidate); } diff --git a/src/test/run-pass/traits-issue-22019.rs b/src/test/run-pass/traits-issue-22019.rs new file mode 100644 index 00000000000..5d3195e1937 --- /dev/null +++ b/src/test/run-pass/traits-issue-22019.rs @@ -0,0 +1,41 @@ +// Copyright 2015 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 an issue where global caching was causing free regions from +// distinct scopes to be compared (`'g` and `'h`). The only important +// thing is that compilation succeeds here. + +#![allow(missing_copy_implementations)] +#![allow(unused_variables)] + +use std::borrow::ToOwned; + +pub struct CFGNode; + +pub type Node<'a> = &'a CFGNode; + +pub trait GraphWalk<'c, N> { + /// Returns all the nodes in this graph. + fn nodes(&'c self) where [N]:ToOwned>; +} + +impl<'g> GraphWalk<'g, Node<'g>> for u32 +{ + fn nodes(&'g self) where [Node<'g>]:ToOwned>> + { loop { } } +} + +impl<'h> GraphWalk<'h, Node<'h>> for u64 +{ + fn nodes(&'h self) where [Node<'h>]:ToOwned>> + { loop { } } +} + +fn main() { }