From a43533a1f548dad19b1b6bad573e660acc9ed88f Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 18 Oct 2015 19:15:57 +0300 Subject: [PATCH 1/8] simplify and reduce the size of EvaluationResult --- src/librustc/middle/traits/mod.rs | 1 + src/librustc/middle/traits/select.rs | 154 +++++++++++++-------- src/librustc/middle/ty/mod.rs | 6 + src/librustc/middle/ty/structural_impls.rs | 1 + src/test/compile-fail/issue-29147.rs | 32 +++++ src/test/run-pass/issue-29147.rs | 35 +++++ 6 files changed, 173 insertions(+), 56 deletions(-) create mode 100644 src/test/compile-fail/issue-29147.rs create mode 100644 src/test/run-pass/issue-29147.rs diff --git a/src/librustc/middle/traits/mod.rs b/src/librustc/middle/traits/mod.rs index a037621f5c025..6e6eeae93607c 100644 --- a/src/librustc/middle/traits/mod.rs +++ b/src/librustc/middle/traits/mod.rs @@ -44,6 +44,7 @@ pub use self::object_safety::object_safety_violations; pub use self::object_safety::ObjectSafetyViolation; pub use self::object_safety::MethodViolationCode; pub use self::object_safety::is_vtable_safe_method; +pub use self::select::EvaluationCache; pub use self::select::SelectionContext; pub use self::select::SelectionCache; pub use self::select::{MethodMatchResult, MethodMatched, MethodAmbiguous, MethodDidNotMatch}; diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index f6e35cf739d62..a24f027e74a57 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -236,11 +236,24 @@ enum BuiltinBoundConditions<'tcx> { AmbiguousBuiltin } -#[derive(Debug)] -enum EvaluationResult<'tcx> { +#[derive(Copy, Clone, Debug, PartialOrd, Ord, PartialEq, Eq)] +/// The result of trait evaluation. The order is important +/// here as the evaluation of a list is the maximum of the +/// evaluations. +enum EvaluationResult { + /// Evaluation successful EvaluatedToOk, + /// Evaluation failed because of recursion - treated as ambiguous + EvaluatedToUnknown, + /// Evaluation is known to be ambiguous EvaluatedToAmbig, - EvaluatedToErr(SelectionError<'tcx>), + /// Evaluation failed + EvaluatedToErr, +} + +#[derive(Clone)] +pub struct EvaluationCache<'tcx> { + hashmap: RefCell, EvaluationResult>> } impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { @@ -381,6 +394,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // The result is "true" if the obligation *may* hold and "false" if // we can be sure it does not. + /// Evaluates whether the obligation `obligation` can be satisfied (by any means). pub fn evaluate_obligation(&mut self, obligation: &PredicateObligation<'tcx>) @@ -393,41 +407,22 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { .may_apply() } - fn evaluate_builtin_bound_recursively<'o>(&mut self, - bound: ty::BuiltinBound, - previous_stack: &TraitObligationStack<'o, 'tcx>, - ty: Ty<'tcx>) - -> EvaluationResult<'tcx> - { - let obligation = - util::predicate_for_builtin_bound( - self.tcx(), - previous_stack.obligation.cause.clone(), - bound, - previous_stack.obligation.recursion_depth + 1, - ty); - - match obligation { - Ok(obligation) => { - self.evaluate_predicate_recursively(previous_stack.list(), &obligation) - } - Err(ErrorReported) => { - EvaluatedToOk - } - } - } - fn evaluate_predicates_recursively<'a,'o,I>(&mut self, stack: TraitObligationStackList<'o, 'tcx>, predicates: I) - -> EvaluationResult<'tcx> + -> EvaluationResult where I : Iterator>, 'tcx:'a { let mut result = EvaluatedToOk; for obligation in predicates { match self.evaluate_predicate_recursively(stack, obligation) { - EvaluatedToErr(e) => { return EvaluatedToErr(e); } + EvaluatedToErr => { return EvaluatedToErr; } EvaluatedToAmbig => { result = EvaluatedToAmbig; } + EvaluatedToUnknown => { + if result < EvaluatedToUnknown { + result = EvaluatedToUnknown; + } + } EvaluatedToOk => { } } } @@ -437,7 +432,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fn evaluate_predicate_recursively<'o>(&mut self, previous_stack: TraitObligationStackList<'o, 'tcx>, obligation: &PredicateObligation<'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { debug!("evaluate_predicate_recursively({:?})", obligation); @@ -464,7 +459,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { }); match result { Ok(()) => EvaluatedToOk, - Err(_) => EvaluatedToErr(Unimplemented), + Err(_) => EvaluatedToErr } } @@ -489,7 +484,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { if object_safety::is_object_safe(self.tcx(), trait_def_id) { EvaluatedToOk } else { - EvaluatedToErr(Unimplemented) + EvaluatedToErr } } @@ -505,7 +500,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { EvaluatedToAmbig } Err(_) => { - EvaluatedToErr(Unimplemented) + EvaluatedToErr } } }) @@ -516,22 +511,33 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fn evaluate_obligation_recursively<'o>(&mut self, previous_stack: TraitObligationStackList<'o, 'tcx>, obligation: &TraitObligation<'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { debug!("evaluate_obligation_recursively({:?})", obligation); let stack = self.push_stack(previous_stack, obligation); + let fresh_trait_ref = stack.fresh_trait_ref; + if let Some(result) = self.check_evaluation_cache(fresh_trait_ref) { + debug!("CACHE HIT: EVAL({:?})={:?}", + fresh_trait_ref, + result); + return result; + } let result = self.evaluate_stack(&stack); - debug!("result: {:?}", result); + debug!("CACHE MISS: EVAL({:?})={:?}", + fresh_trait_ref, + result); + self.insert_evaluation_cache(fresh_trait_ref, result); + result } fn evaluate_stack<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { // In intercrate mode, whenever any of the types are unbound, // there can always be an impl. Even if there are no impls in @@ -559,16 +565,19 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // precise still. let input_types = stack.fresh_trait_ref.0.input_types(); let unbound_input_types = input_types.iter().any(|ty| ty.is_fresh()); - if - unbound_input_types && - (self.intercrate || + if unbound_input_types && self.intercrate { + debug!("evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous", + stack.fresh_trait_ref); + return EvaluatedToAmbig; + } + if unbound_input_types && stack.iter().skip(1).any( |prev| self.match_fresh_trait_refs(&stack.fresh_trait_ref, - &prev.fresh_trait_ref))) + &prev.fresh_trait_ref)) { - debug!("evaluate_stack({:?}) --> unbound argument, recursion --> ambiguous", + debug!("evaluate_stack({:?}) --> unbound argument, recursive --> giving up", stack.fresh_trait_ref); - return EvaluatedToAmbig; + return EvaluatedToUnknown; } // If there is any previous entry on the stack that precisely @@ -603,7 +612,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { match self.candidate_from_obligation(stack) { Ok(Some(c)) => self.winnow_candidate(stack, &c), Ok(None) => EvaluatedToAmbig, - Err(e) => EvaluatedToErr(e), + Err(..) => EvaluatedToErr } } @@ -637,6 +646,34 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { }) } + fn pick_evaluation_cache(&self) -> &EvaluationCache<'tcx> { + &self.param_env().evaluation_cache + } + + fn check_evaluation_cache(&self, trait_ref: ty::PolyTraitRef<'tcx>) + -> Option + { + let cache = self.pick_evaluation_cache(); + cache.hashmap.borrow().get(&trait_ref).cloned() + } + + fn insert_evaluation_cache(&mut self, + trait_ref: ty::PolyTraitRef<'tcx>, + result: EvaluationResult) + { + // Avoid caching results that depend on more than just the trait-ref: + // The stack can create EvaluatedToUnknown, and closure signatures + // being yet uninferred can create "spurious" EvaluatedToAmbig. + if result == EvaluatedToUnknown || + (result == EvaluatedToAmbig && trait_ref.has_closure_types()) + { + return; + } + + let cache = self.pick_evaluation_cache(); + cache.hashmap.borrow_mut().insert(trait_ref, result); + } + /////////////////////////////////////////////////////////////////////////// // CANDIDATE ASSEMBLY // @@ -669,7 +706,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { match self.check_candidate_cache(&cache_fresh_trait_pred) { Some(c) => { - debug!("CACHE HIT: cache_fresh_trait_pred={:?}, candidate={:?}", + debug!("CACHE HIT: SELECT({:?})={:?}", cache_fresh_trait_pred, c); return c; @@ -681,7 +718,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { let candidate = self.candidate_from_obligation_no_cache(stack); if self.should_update_candidate_cache(&cache_fresh_trait_pred, &candidate) { - debug!("CACHE MISS: cache_fresh_trait_pred={:?}, candidate={:?}", + debug!("CACHE MISS: SELECT({:?})={:?}", cache_fresh_trait_pred, candidate); self.insert_candidate_cache(cache_fresh_trait_pred, candidate.clone()); } @@ -1138,7 +1175,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fn evaluate_where_clause<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>, where_clause_trait_ref: ty::PolyTraitRef<'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { self.infcx().probe(move |_| { match self.match_where_clause_trait_ref(stack.obligation, where_clause_trait_ref) { @@ -1146,7 +1183,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { self.evaluate_predicates_recursively(stack.list(), obligations.iter()) } Err(()) => { - EvaluatedToErr(Unimplemented) + EvaluatedToErr } } }) @@ -1492,7 +1529,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fn winnow_candidate<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>, candidate: &SelectionCandidate<'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { debug!("winnow_candidate: candidate={:?}", candidate); let result = self.infcx.probe(|_| { @@ -1500,7 +1537,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { match self.confirm_candidate(stack.obligation, candidate) { Ok(selection) => self.winnow_selection(stack.list(), selection), - Err(error) => EvaluatedToErr(error), + Err(..) => EvaluatedToErr } }); debug!("winnow_candidate depth={} result={:?}", @@ -1511,7 +1548,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fn winnow_selection<'o>(&mut self, stack: TraitObligationStackList<'o,'tcx>, selection: Selection<'tcx>) - -> EvaluationResult<'tcx> + -> EvaluationResult { self.evaluate_predicates_recursively(stack, selection.nested_obligations().iter()) @@ -2956,6 +2993,14 @@ impl<'tcx> SelectionCache<'tcx> { } } +impl<'tcx> EvaluationCache<'tcx> { + pub fn new() -> EvaluationCache<'tcx> { + EvaluationCache { + hashmap: RefCell::new(FnvHashMap()) + } + } +} + impl<'o,'tcx> TraitObligationStack<'o,'tcx> { fn list(&'o self) -> TraitObligationStackList<'o,'tcx> { TraitObligationStackList::with(self) @@ -3001,17 +3046,14 @@ impl<'o,'tcx> fmt::Debug for TraitObligationStack<'o,'tcx> { } } -impl<'tcx> EvaluationResult<'tcx> { +impl EvaluationResult { fn may_apply(&self) -> bool { match *self { EvaluatedToOk | EvaluatedToAmbig | - EvaluatedToErr(OutputTypeParameterMismatch(..)) | - EvaluatedToErr(TraitNotObjectSafe(_)) => - true, + EvaluatedToUnknown => true, - EvaluatedToErr(Unimplemented) => - false, + EvaluatedToErr => false } } } diff --git a/src/librustc/middle/ty/mod.rs b/src/librustc/middle/ty/mod.rs index aa18974470178..0a9fa1d6ce394 100644 --- a/src/librustc/middle/ty/mod.rs +++ b/src/librustc/middle/ty/mod.rs @@ -1091,6 +1091,9 @@ pub struct ParameterEnvironment<'a, 'tcx:'a> { /// for things that have to do with the parameters in scope. pub selection_cache: traits::SelectionCache<'tcx>, + /// Caches the results of trait evaluation. + pub evaluation_cache: traits::EvaluationCache<'tcx>, + /// Scope that is attached to free regions for this scope. This /// is usually the id of the fn body, but for more abstract scopes /// like structs we often use the node-id of the struct. @@ -1112,6 +1115,7 @@ impl<'a, 'tcx> ParameterEnvironment<'a, 'tcx> { implicit_region_bound: self.implicit_region_bound, caller_bounds: caller_bounds, selection_cache: traits::SelectionCache::new(), + evaluation_cache: traits::EvaluationCache::new(), free_id: self.free_id, } } @@ -2584,6 +2588,7 @@ impl<'tcx> ctxt<'tcx> { caller_bounds: Vec::new(), implicit_region_bound: ty::ReEmpty, selection_cache: traits::SelectionCache::new(), + evaluation_cache: traits::EvaluationCache::new(), // for an empty parameter // environment, there ARE no free @@ -2673,6 +2678,7 @@ impl<'tcx> ctxt<'tcx> { implicit_region_bound: ty::ReScope(free_id_outlive), caller_bounds: predicates, selection_cache: traits::SelectionCache::new(), + evaluation_cache: traits::EvaluationCache::new(), free_id: free_id, }; diff --git a/src/librustc/middle/ty/structural_impls.rs b/src/librustc/middle/ty/structural_impls.rs index 176dc5a743d05..41303b46dfd09 100644 --- a/src/librustc/middle/ty/structural_impls.rs +++ b/src/librustc/middle/ty/structural_impls.rs @@ -822,6 +822,7 @@ impl<'a, 'tcx> TypeFoldable<'tcx> for ty::ParameterEnvironment<'a, 'tcx> where ' implicit_region_bound: self.implicit_region_bound.fold_with(folder), caller_bounds: self.caller_bounds.fold_with(folder), selection_cache: traits::SelectionCache::new(), + evaluation_cache: traits::EvaluationCache::new(), free_id: self.free_id, } } diff --git a/src/test/compile-fail/issue-29147.rs b/src/test/compile-fail/issue-29147.rs new file mode 100644 index 0000000000000..64bfa232f3ffd --- /dev/null +++ b/src/test/compile-fail/issue-29147.rs @@ -0,0 +1,32 @@ +// 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. + +#![recursion_limit="1024"] + +pub struct S0(T,T); +pub struct S1(Option>>>,Option>>>); +pub struct S2(Option>>>,Option>>>); +pub struct S3(Option>>>,Option>>>); +pub struct S4(Option>>>,Option>>>); +pub struct S5(Option>>>,Option>>>,Option); + +trait Foo { fn xxx(&self); } +trait Bar {} // anything local or #[fundamental] + +impl Foo for T where T: Bar, T: Sync { + fn xxx(&self) {} +} + +impl Foo for S5 { fn xxx(&self) {} } +impl Foo for S5 { fn xxx(&self) {} } + +fn main() { + let _ = >::xxx; //~ ERROR cannot resolve `S5<_> : Foo` +} diff --git a/src/test/run-pass/issue-29147.rs b/src/test/run-pass/issue-29147.rs new file mode 100644 index 0000000000000..460e92967e8e6 --- /dev/null +++ b/src/test/run-pass/issue-29147.rs @@ -0,0 +1,35 @@ +// 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. + +#![recursion_limit="1024"] + +use std::mem; + +pub struct S0(T,T); +pub struct S1(Option>>>,Option>>>); +pub struct S2(Option>>>,Option>>>); +pub struct S3(Option>>>,Option>>>); +pub struct S4(Option>>>,Option>>>); +pub struct S5(Option>>>,Option>>>,Option); + +trait Foo { fn xxx(&self); } +trait Bar {} // anything local or #[fundamental] + +impl Foo for T where T: Bar, T: Sync { + fn xxx(&self) {} +} + +impl Foo for S5 { fn xxx(&self) {} } + +fn main() { + let s = S5(None,None,None); + s.xxx(); + assert_eq!(mem::size_of_val(&s.2), mem::size_of::>()); +} From f37b4fe84fa2a2aac501a5e9a2f2361ffead0418 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 20 Oct 2015 17:15:58 +0300 Subject: [PATCH 2/8] put projections in RFC447 order --- src/librustc/middle/traits/select.rs | 24 ++-- src/librustc_typeck/collect.rs | 58 ++++++--- .../constrained_type_params.rs | 114 +++++++++++++----- 3 files changed, 137 insertions(+), 59 deletions(-) diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index a24f027e74a57..07edaf585f5fa 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -663,9 +663,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { { // Avoid caching results that depend on more than just the trait-ref: // The stack can create EvaluatedToUnknown, and closure signatures - // being yet uninferred can create "spurious" EvaluatedToAmbig. + // being yet uninferred can create "spurious" EvaluatedToAmbig + // and EvaluatedToOk. if result == EvaluatedToUnknown || - (result == EvaluatedToAmbig && trait_ref.has_closure_types()) + ((result == EvaluatedToAmbig || result == EvaluatedToOk) + && trait_ref.has_closure_types()) { return; } @@ -2297,6 +2299,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { impl_def_id, impl_obligations); + // Because of RFC447, the impl-trait-ref and obligations + // are sufficient to determine the impl substs, without + // relying on projections in the impl-trait-ref. + // + // e.g. `impl> Foo<::T> for V` impl_obligations.append(&mut substs.obligations); VtableImplData { impl_def_id: impl_def_id, @@ -2933,12 +2940,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { let predicates = self.tcx().lookup_predicates(def_id); let predicates = predicates.instantiate(self.tcx(), substs); let predicates = normalize_with_depth(self, cause.clone(), recursion_depth, &predicates); - let mut predicates = self.infcx().plug_leaks(skol_map, snapshot, &predicates); - let mut obligations = - util::predicates_for_generics(cause, - recursion_depth, - &predicates.value); - obligations.append(&mut predicates.obligations); + let predicates = self.infcx().plug_leaks(skol_map, snapshot, &predicates); + + let mut obligations = predicates.obligations; + obligations.append( + &mut util::predicates_for_generics(cause, + recursion_depth, + &predicates.value)); obligations } diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 30eb468d9f805..185623a440253 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -775,31 +775,33 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { ref impl_items) => { // Create generics from the generics specified in the impl head. debug!("convert: ast_generics={:?}", generics); + let def_id = ccx.tcx.map.local_def_id(it.id); let ty_generics = ty_generics_for_type_or_impl(ccx, generics); - let ty_predicates = ty_generic_predicates_for_type_or_impl(ccx, generics); + let mut ty_predicates = ty_generic_predicates_for_type_or_impl(ccx, generics); debug!("convert: impl_bounds={:?}", ty_predicates); let selfty = ccx.icx(&ty_predicates).to_ty(&ExplicitRscope, &**selfty); write_ty_to_tcx(tcx, it.id, selfty); - tcx.register_item_type(ccx.tcx.map.local_def_id(it.id), + tcx.register_item_type(def_id, TypeScheme { generics: ty_generics.clone(), ty: selfty }); - tcx.predicates.borrow_mut().insert(ccx.tcx.map.local_def_id(it.id), - ty_predicates.clone()); if let &Some(ref ast_trait_ref) = opt_trait_ref { tcx.impl_trait_refs.borrow_mut().insert( - ccx.tcx.map.local_def_id(it.id), + def_id, Some(astconv::instantiate_mono_trait_ref(&ccx.icx(&ty_predicates), &ExplicitRscope, ast_trait_ref, Some(selfty))) ); } else { - tcx.impl_trait_refs.borrow_mut().insert(ccx.tcx.map.local_def_id(it.id), None); + tcx.impl_trait_refs.borrow_mut().insert(def_id, None); } + enforce_impl_params_are_constrained(tcx, generics, &mut ty_predicates, def_id); + tcx.predicates.borrow_mut().insert(def_id, ty_predicates.clone()); + // If there is a trait reference, treat the methods as always public. // This is to work around some incorrect behavior in privacy checking: @@ -844,7 +846,7 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { generics: ty_generics.clone(), ty: ty, }); - convert_associated_const(ccx, ImplContainer(ccx.tcx.map.local_def_id(it.id)), + convert_associated_const(ccx, ImplContainer(def_id), impl_item.name, impl_item.id, impl_item.vis.inherit_from(parent_visibility), ty, true /* has_value */); @@ -861,7 +863,7 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { let typ = ccx.icx(&ty_predicates).to_ty(&ExplicitRscope, ty); - convert_associated_type(ccx, ImplContainer(ccx.tcx.map.local_def_id(it.id)), + convert_associated_type(ccx, ImplContainer(def_id), impl_item.name, impl_item.id, impl_item.vis, Some(typ)); } @@ -880,7 +882,7 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { } }); convert_methods(ccx, - ImplContainer(ccx.tcx.map.local_def_id(it.id)), + ImplContainer(def_id), methods, selfty, &ty_generics, @@ -898,10 +900,7 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { } } - enforce_impl_params_are_constrained(tcx, - generics, - ccx.tcx.map.local_def_id(it.id), - impl_items); + enforce_impl_lifetimes_are_constrained(tcx, generics, def_id, impl_items); }, hir::ItemTrait(_, _, _, ref trait_items) => { let trait_def = trait_def_of_item(ccx, it); @@ -2377,13 +2376,15 @@ fn check_method_self_type<'a, 'tcx, RS:RegionScope>( /// Checks that all the type parameters on an impl fn enforce_impl_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, ast_generics: &hir::Generics, - impl_def_id: DefId, - impl_items: &[P]) + impl_predicates: &mut ty::GenericPredicates<'tcx>, + impl_def_id: DefId) { let impl_scheme = tcx.lookup_item_type(impl_def_id); - let impl_predicates = tcx.lookup_predicates(impl_def_id); let impl_trait_ref = tcx.impl_trait_ref(impl_def_id); + assert!(impl_predicates.predicates.is_empty_in(FnSpace)); + assert!(impl_predicates.predicates.is_empty_in(SelfSpace)); + // The trait reference is an input, so find all type parameters // reachable from there, to start (if this is an inherent impl, // then just examine the self type). @@ -2393,10 +2394,10 @@ fn enforce_impl_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, input_parameters.extend(ctp::parameters_for_trait_ref(trait_ref)); } - ctp::identify_constrained_type_params(tcx, - impl_predicates.predicates.as_slice(), - impl_trait_ref, - &mut input_parameters); + ctp::setup_constraining_predicates(tcx, + impl_predicates.predicates.get_mut_slice(TypeSpace), + impl_trait_ref, + &mut input_parameters); for (index, ty_param) in ast_generics.ty_params.iter().enumerate() { let param_ty = ty::ParamTy { space: TypeSpace, @@ -2406,8 +2407,25 @@ fn enforce_impl_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, report_unused_parameter(tcx, ty_param.span, "type", ¶m_ty.to_string()); } } +} +fn enforce_impl_lifetimes_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, + ast_generics: &hir::Generics, + impl_def_id: DefId, + impl_items: &[P]) +{ // Every lifetime used in an associated type must be constrained. + let impl_scheme = tcx.lookup_item_type(impl_def_id); + let impl_predicates = tcx.lookup_predicates(impl_def_id); + let impl_trait_ref = tcx.impl_trait_ref(impl_def_id); + + let mut input_parameters: HashSet<_> = + ctp::parameters_for_type(impl_scheme.ty).into_iter().collect(); + if let Some(ref trait_ref) = impl_trait_ref { + input_parameters.extend(ctp::parameters_for_trait_ref(trait_ref)); + } + ctp::identify_constrained_type_params(tcx, + &impl_predicates.predicates.as_slice(), impl_trait_ref, &mut input_parameters); let lifetimes_in_associated_types: HashSet<_> = impl_items.iter() diff --git a/src/librustc_typeck/constrained_type_params.rs b/src/librustc_typeck/constrained_type_params.rs index 7844d71462cfc..39d5872b3dc7a 100644 --- a/src/librustc_typeck/constrained_type_params.rs +++ b/src/librustc_typeck/constrained_type_params.rs @@ -84,40 +84,92 @@ pub fn identify_constrained_type_params<'tcx>(_tcx: &ty::ctxt<'tcx>, impl_trait_ref: Option>, input_parameters: &mut HashSet) { - loop { - let num_inputs = input_parameters.len(); - - let poly_projection_predicates = // : iterator over PolyProjectionPredicate - predicates.iter() - .filter_map(|predicate| { - match *predicate { - ty::Predicate::Projection(ref data) => Some(data.clone()), - _ => None, - } - }); - - for poly_projection in poly_projection_predicates { - // Note that we can skip binder here because the impl - // trait ref never contains any late-bound regions. - let projection = poly_projection.skip_binder(); - - // Special case: watch out for some kind of sneaky attempt - // to project out an associated type defined by this very - // trait. - let unbound_trait_ref = &projection.projection_ty.trait_ref; - if Some(unbound_trait_ref.clone()) == impl_trait_ref { - continue; - } + let mut predicates = predicates.to_owned(); + setup_constraining_predicates(_tcx, &mut predicates, impl_trait_ref, input_parameters); +} + - let inputs = parameters_for_trait_ref(&projection.projection_ty.trait_ref); - let relies_only_on_inputs = inputs.iter().all(|p| input_parameters.contains(&p)); - if relies_only_on_inputs { +/// Order the predicates in `predicates` such that each parameter is +/// constrained before it is used, if that is possible, and add the +/// paramaters so constrained to `input_parameters`. For example, +/// imagine the following impl: +/// +/// impl> Trait for U +/// +/// The impl's predicates are collected from left to right. Ignoring +/// the implicit `Sized` bounds, these are +/// * T: Debug +/// * U: Iterator +/// * ::Item = T -- a desugared ProjectionPredicate +/// +/// When we, for example, try to go over the trait-reference +/// `IntoIter as Trait`, we substitute the impl parameters with fresh +/// variables and match them with the impl trait-ref, so we know that +/// `$U = IntoIter`. +/// +/// However, in order to process the `$T: Debug` predicate, we must first +/// know the value of `$T` - which is only given by processing the +/// projection. As we occasionally want to process predicates in a single +/// pass, we want the projection to come first. In fact, as projections +/// can (acyclically) depend on one another - see RFC447 for details - we +/// need to topologically sort them. +pub fn setup_constraining_predicates<'tcx>(_tcx: &ty::ctxt<'tcx>, + predicates: &mut [ty::Predicate<'tcx>], + impl_trait_ref: Option>, + input_parameters: &mut HashSet) +{ + // The canonical way of doing the needed topological sort + // would be a DFS, but getting the graph and its ownership + // right is annoying, so I am using an in-place fixed-point iteration, + // which is `O(nt)` where `t` is the depth of type-parameter constraints, + // remembering that `t` should be less than 7 in practice. + // + // Basically, I iterate over all projections and swap every + // "ready" projection to the start of the list, such that + // all of the projections before `i` are topologically sorted + // and constrain all the parameters in `input_parameters`. + // + // In the example, `input_parameters` starts by containing `U` - which + // is constrained by the trait-ref - and so on the first pass we + // observe that `::Item = T` is a "ready" projection that + // constrains `T` and swap it to front. As it is the sole projection, + // no more swaps can take place afterwards, with the result being + // * ::Item = T + // * T: Debug + // * U: Iterator + let mut i = 0; + let mut changed = true; + while changed { + changed = false; + + for j in i..predicates.len() { + + if let ty::Predicate::Projection(ref poly_projection) = predicates[j] { + // Note that we can skip binder here because the impl + // trait ref never contains any late-bound regions. + let projection = poly_projection.skip_binder(); + + // Special case: watch out for some kind of sneaky attempt + // to project out an associated type defined by this very + // trait. + let unbound_trait_ref = &projection.projection_ty.trait_ref; + if Some(unbound_trait_ref.clone()) == impl_trait_ref { + continue; + } + + let inputs = parameters_for_trait_ref(&projection.projection_ty.trait_ref); + let relies_only_on_inputs = inputs.iter().all(|p| input_parameters.contains(&p)); + if !relies_only_on_inputs { + continue; + } input_parameters.extend(parameters_for_type(projection.ty)); + } else { + continue; } - } - - if input_parameters.len() == num_inputs { - break; + // fancy control flow to bypass borrow checker + predicates.swap(i, j); + i += 1; + changed = true; } } } From 9c6d35d03782259b0ac89976a182f2435a79e8ec Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 20 Oct 2015 18:23:46 +0300 Subject: [PATCH 3/8] evaluate projections outside the outer probe in recursive evaluation --- src/librustc/middle/traits/select.rs | 202 ++++-------------- .../run-pass/coherence-rfc447-constrained.rs | 31 +++ 2 files changed, 77 insertions(+), 156 deletions(-) create mode 100644 src/test/run-pass/coherence-rfc447-constrained.rs diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index 07edaf585f5fa..d4154b5953770 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -9,7 +9,6 @@ // except according to those terms. //! See `README.md` for high-level documentation -#![allow(dead_code)] // FIXME -- just temporarily pub use self::MethodMatchResult::*; pub use self::MethodMatchedData::*; @@ -190,7 +189,6 @@ pub enum MethodMatchedData { /// parameter environment. #[derive(PartialEq,Eq,Debug,Clone)] enum SelectionCandidate<'tcx> { - PhantomFnCandidate, BuiltinCandidate(ty::BuiltinBound), ParamCandidate(ty::PolyTraitRef<'tcx>), ImplCandidate(DefId), @@ -403,8 +401,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!("evaluate_obligation({:?})", obligation); - self.evaluate_predicate_recursively(TraitObligationStackList::empty(), obligation) - .may_apply() + self.infcx.probe(|_| { + self.evaluate_predicate_recursively(TraitObligationStackList::empty(), obligation) + .may_apply() + }) } fn evaluate_predicates_recursively<'a,'o,I>(&mut self, @@ -415,7 +415,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { { let mut result = EvaluatedToOk; for obligation in predicates { - match self.evaluate_predicate_recursively(stack, obligation) { + let eval = self.evaluate_predicate_recursively(stack, obligation); + debug!("evaluate_predicate_recursively({:?}) = {:?}", + obligation, eval); + match eval { EvaluatedToErr => { return EvaluatedToErr; } EvaluatedToAmbig => { result = EvaluatedToAmbig; } EvaluatedToUnknown => { @@ -454,10 +457,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } ty::Predicate::Equate(ref p) => { - let result = self.infcx.probe(|_| { - self.infcx.equality_predicate(obligation.cause.span, p) - }); - match result { + // does this code ever run? + match self.infcx.equality_predicate(obligation.cause.span, p) { Ok(()) => EvaluatedToOk, Err(_) => EvaluatedToErr } @@ -489,21 +490,19 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } ty::Predicate::Projection(ref data) => { - self.infcx.probe(|_| { - let project_obligation = obligation.with(data.clone()); - match project::poly_project_and_unify_type(self, &project_obligation) { - Ok(Some(subobligations)) => { - self.evaluate_predicates_recursively(previous_stack, - subobligations.iter()) - } - Ok(None) => { - EvaluatedToAmbig - } - Err(_) => { - EvaluatedToErr - } + let project_obligation = obligation.with(data.clone()); + match project::poly_project_and_unify_type(self, &project_obligation) { + Ok(Some(subobligations)) => { + self.evaluate_predicates_recursively(previous_stack, + subobligations.iter()) } - }) + Ok(None) => { + EvaluatedToAmbig + } + Err(_) => { + EvaluatedToErr + } + } } } } @@ -610,40 +609,36 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } match self.candidate_from_obligation(stack) { - Ok(Some(c)) => self.winnow_candidate(stack, &c), + Ok(Some(c)) => self.evaluate_candidate(stack, &c), Ok(None) => EvaluatedToAmbig, Err(..) => EvaluatedToErr } } - /// Evaluates whether the impl with id `impl_def_id` could be applied to the self type - /// `obligation_self_ty`. This can be used either for trait or inherent impls. - pub fn evaluate_impl(&mut self, - impl_def_id: DefId, - obligation: &TraitObligation<'tcx>) - -> bool + /// Further evaluate `candidate` to decide whether all type parameters match and whether nested + /// obligations are met. Returns true if `candidate` remains viable after this further + /// scrutiny. + fn evaluate_candidate<'o>(&mut self, + stack: &TraitObligationStack<'o, 'tcx>, + candidate: &SelectionCandidate<'tcx>) + -> EvaluationResult { - debug!("evaluate_impl(impl_def_id={:?}, obligation={:?})", - impl_def_id, - obligation); - - self.infcx.probe(|snapshot| { - match self.match_impl(impl_def_id, obligation, snapshot) { - Ok((substs, skol_map)) => { - let vtable_impl = self.vtable_impl(impl_def_id, - substs, - obligation.cause.clone(), - obligation.recursion_depth + 1, - skol_map, - snapshot); - self.winnow_selection(TraitObligationStackList::empty(), - VtableImpl(vtable_impl)).may_apply() - } - Err(()) => { - false + debug!("evaluate_candidate: depth={} candidate={:?}", + stack.obligation.recursion_depth, candidate); + let result = self.infcx.probe(|_| { + let candidate = (*candidate).clone(); + match self.confirm_candidate(stack.obligation, candidate) { + Ok(selection) => { + self.evaluate_predicates_recursively( + stack.list(), + selection.nested_obligations().iter()) } + Err(..) => EvaluatedToErr } - }) + }); + debug!("evaluate_candidate: depth={} result={:?}", + stack.obligation.recursion_depth, result); + result } fn pick_evaluation_cache(&self) -> &EvaluationCache<'tcx> { @@ -779,7 +774,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // Instead, we select the right impl now but report `Bar does // not implement Clone`. if candidates.len() > 1 { - candidates.retain(|c| self.winnow_candidate(stack, c).may_apply()) + candidates.retain(|c| self.evaluate_candidate(stack, c).may_apply()) } // If there are STILL multiple candidate, we can further reduce @@ -1184,9 +1179,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { Ok(obligations) => { self.evaluate_predicates_recursively(stack.list(), obligations.iter()) } - Err(()) => { - EvaluatedToErr - } + Err(()) => EvaluatedToErr } }) } @@ -1525,37 +1518,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // attempt to evaluate recursive bounds to see if they are // satisfied. - /// Further evaluate `candidate` to decide whether all type parameters match and whether nested - /// obligations are met. Returns true if `candidate` remains viable after this further - /// scrutiny. - fn winnow_candidate<'o>(&mut self, - stack: &TraitObligationStack<'o, 'tcx>, - candidate: &SelectionCandidate<'tcx>) - -> EvaluationResult - { - debug!("winnow_candidate: candidate={:?}", candidate); - let result = self.infcx.probe(|_| { - let candidate = (*candidate).clone(); - match self.confirm_candidate(stack.obligation, candidate) { - Ok(selection) => self.winnow_selection(stack.list(), - selection), - Err(..) => EvaluatedToErr - } - }); - debug!("winnow_candidate depth={} result={:?}", - stack.obligation.recursion_depth, result); - result - } - - fn winnow_selection<'o>(&mut self, - stack: TraitObligationStackList<'o,'tcx>, - selection: Selection<'tcx>) - -> EvaluationResult - { - self.evaluate_predicates_recursively(stack, - selection.nested_obligations().iter()) - } - /// Returns true if `candidate_i` should be dropped in favor of /// `candidate_j`. Generally speaking we will drop duplicate /// candidates and prefer where-clause candidates. @@ -1581,9 +1543,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { "default implementations shouldn't be recorded \ when there are other valid candidates"); } - &PhantomFnCandidate => { - self.tcx().sess.bug("PhantomFn didn't short-circuit selection"); - } &ImplCandidate(..) | &ClosureCandidate(..) | &FnPointerCandidate(..) | @@ -2013,7 +1972,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { try!(self.confirm_builtin_candidate(obligation, builtin_bound)))) } - PhantomFnCandidate | ErrorCandidate => { Ok(VtableBuiltin(VtableBuiltinData { nested: vec![] })) } @@ -2788,74 +2746,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } - /// Determines whether the self type declared against - /// `impl_def_id` matches `obligation_self_ty`. If successful, - /// returns the substitutions used to make them match. See - /// `match_impl()`. For example, if `impl_def_id` is declared - /// as: - /// - /// impl Foo for Box { ... } - /// - /// and `obligation_self_ty` is `int`, we'd get back an `Err(_)` - /// result. But if `obligation_self_ty` were `Box`, we'd get - /// back `Ok(T=int)`. - fn match_inherent_impl(&mut self, - impl_def_id: DefId, - obligation_cause: &ObligationCause, - obligation_self_ty: Ty<'tcx>) - -> Result,()> - { - // Create fresh type variables for each type parameter declared - // on the impl etc. - let impl_substs = util::fresh_type_vars_for_impl(self.infcx, - obligation_cause.span, - impl_def_id); - - // Find the self type for the impl. - let impl_self_ty = self.tcx().lookup_item_type(impl_def_id).ty; - let impl_self_ty = impl_self_ty.subst(self.tcx(), &impl_substs); - - debug!("match_impl_self_types(obligation_self_ty={:?}, impl_self_ty={:?})", - obligation_self_ty, - impl_self_ty); - - match self.match_self_types(obligation_cause, - impl_self_ty, - obligation_self_ty) { - Ok(()) => { - debug!("Matched impl_substs={:?}", impl_substs); - Ok(impl_substs) - } - Err(()) => { - debug!("NoMatch"); - Err(()) - } - } - } - - fn match_self_types(&mut self, - cause: &ObligationCause, - - // The self type provided by the impl/caller-obligation: - provided_self_ty: Ty<'tcx>, - - // The self type the obligation is for: - required_self_ty: Ty<'tcx>) - -> Result<(),()> - { - // FIXME(#5781) -- equating the types is stronger than - // necessary. Should consider variance of trait w/r/t Self. - - let origin = infer::RelateSelfType(cause.span); - match self.infcx.eq_types(false, - origin, - provided_self_ty, - required_self_ty) { - Ok(()) => Ok(()), - Err(_) => Err(()), - } - } - /////////////////////////////////////////////////////////////////////////// // Miscellany diff --git a/src/test/run-pass/coherence-rfc447-constrained.rs b/src/test/run-pass/coherence-rfc447-constrained.rs new file mode 100644 index 0000000000000..4b52378e50889 --- /dev/null +++ b/src/test/run-pass/coherence-rfc447-constrained.rs @@ -0,0 +1,31 @@ +// 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. + +// check that trait matching can handle impls whose types are only +// constrained by a projection. + +trait IsU32 {} +impl IsU32 for u32 {} + +trait Mirror { type Image: ?Sized; } +impl Mirror for T { type Image = T; } + +trait Bar {} +impl, L: Mirror> Bar for V + where U::Image: IsU32 {} + +trait Foo { fn name() -> &'static str; } +impl Foo for u64 { fn name() -> &'static str { "u64" } } +impl Foo for T { fn name() -> &'static str { "Bar" }} + +fn main() { + assert_eq!(::name(), "u64"); + assert_eq!(::name(), "Bar"); +} From 8943d5a4a43c2c48474c8061b9eb2cfd6d8c0f22 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 20 Oct 2015 19:14:05 +0300 Subject: [PATCH 4/8] introduce evaluate_obligation_conservatively and use it --- src/librustc/middle/traits/mod.rs | 15 +++++++++++++++ src/librustc/middle/traits/select.rs | 17 +++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/librustc/middle/traits/mod.rs b/src/librustc/middle/traits/mod.rs index 6e6eeae93607c..9d8138dc38ad5 100644 --- a/src/librustc/middle/traits/mod.rs +++ b/src/librustc/middle/traits/mod.rs @@ -340,6 +340,21 @@ pub fn type_known_to_meet_builtin_bound<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>, ty, bound); + if !ty.has_infer_types() && !ty.has_closure_types() { + let cause = ObligationCause::misc(span, ast::DUMMY_NODE_ID); + let obligation = + util::predicate_for_builtin_bound(infcx.tcx, cause, bound, 0, ty); + let obligation = match obligation { + Ok(o) => o, + Err(..) => return false + }; + let result = SelectionContext::new(infcx) + .evaluate_obligation_conservatively(&obligation); + debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} => {:?}", + ty, bound, result); + return result; + } + let mut fulfill_cx = FulfillmentContext::new(false); // We can use a dummy node-id here because we won't pay any mind diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index d4154b5953770..befc74baacc73 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -407,6 +407,23 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { }) } + /// Evaluates whether the obligation `obligation` can be satisfied, + /// and returns `false` if not certain. However, this is not entirely + /// accurate if inference variables are involved. + pub fn evaluate_obligation_conservatively(&mut self, + obligation: &PredicateObligation<'tcx>) + -> bool + { + debug!("evaluate_obligation_conservatively({:?})", + obligation); + + self.infcx.probe(|_| { + self.evaluate_predicate_recursively(TraitObligationStackList::empty(), obligation) + == EvaluatedToOk + }) + } + + fn evaluate_predicates_recursively<'a,'o,I>(&mut self, stack: TraitObligationStackList<'o, 'tcx>, predicates: I) From c998057770737a6419880b9177317f5fced75912 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 21 Oct 2015 14:50:38 +0300 Subject: [PATCH 5/8] add a global evaluation cache --- src/librustc/middle/traits/select.rs | 10 +++++++++- src/librustc/middle/ty/context.rs | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index befc74baacc73..2bc1e8faddb23 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -659,7 +659,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } fn pick_evaluation_cache(&self) -> &EvaluationCache<'tcx> { - &self.param_env().evaluation_cache + // see comment in `pick_candidate_cache` + if self.intercrate || + !self.param_env().caller_bounds.is_empty() + { + &self.param_env().evaluation_cache + } else + { + &self.tcx().evaluation_cache + } } fn check_evaluation_cache(&self, trait_ref: ty::PolyTraitRef<'tcx>) diff --git a/src/librustc/middle/ty/context.rs b/src/librustc/middle/ty/context.rs index d91cac4cc75b9..e02a120a5c60f 100644 --- a/src/librustc/middle/ty/context.rs +++ b/src/librustc/middle/ty/context.rs @@ -332,6 +332,11 @@ pub struct ctxt<'tcx> { /// for things that do not have to do with the parameters in scope. pub selection_cache: traits::SelectionCache<'tcx>, + /// Caches the results of trait evaluation. This cache is used + /// for things that do not have to do with the parameters in scope. + /// Merge this with `selection_cache`? + pub evaluation_cache: traits::EvaluationCache<'tcx>, + /// A set of predicates that have been fulfilled *somewhere*. /// This is used to avoid duplicate work. Predicates are only /// added to this set when they mention only "global" names @@ -512,6 +517,7 @@ impl<'tcx> ctxt<'tcx> { transmute_restrictions: RefCell::new(Vec::new()), stability: RefCell::new(stability), selection_cache: traits::SelectionCache::new(), + evaluation_cache: traits::EvaluationCache::new(), repr_hint_cache: RefCell::new(DefIdMap()), const_qualif_map: RefCell::new(NodeMap()), custom_coerce_unsized_kinds: RefCell::new(DefIdMap()), From 4a16b562a8c761bc1c5359cb86753c010148f83c Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 21 Oct 2015 19:01:58 +0300 Subject: [PATCH 6/8] fix remaining bugs --- src/librustc/middle/traits/mod.rs | 82 ++++++++++--------- src/librustc/middle/traits/select.rs | 41 +++++++--- src/test/compile-fail/cast-rfc0401.rs | 5 +- .../compile-fail/infinite-instantiation.rs | 2 +- src/test/compile-fail/recursion.rs | 8 +- src/test/run-pass/trait-copy-guessing.rs | 46 +++++++++++ 6 files changed, 125 insertions(+), 59 deletions(-) create mode 100644 src/test/run-pass/trait-copy-guessing.rs diff --git a/src/librustc/middle/traits/mod.rs b/src/librustc/middle/traits/mod.rs index 9d8138dc38ad5..691bac0cef865 100644 --- a/src/librustc/middle/traits/mod.rs +++ b/src/librustc/middle/traits/mod.rs @@ -340,47 +340,53 @@ pub fn type_known_to_meet_builtin_bound<'a,'tcx>(infcx: &InferCtxt<'a,'tcx>, ty, bound); - if !ty.has_infer_types() && !ty.has_closure_types() { - let cause = ObligationCause::misc(span, ast::DUMMY_NODE_ID); - let obligation = - util::predicate_for_builtin_bound(infcx.tcx, cause, bound, 0, ty); - let obligation = match obligation { - Ok(o) => o, - Err(..) => return false - }; - let result = SelectionContext::new(infcx) - .evaluate_obligation_conservatively(&obligation); - debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} => {:?}", - ty, bound, result); - return result; - } - - let mut fulfill_cx = FulfillmentContext::new(false); - - // We can use a dummy node-id here because we won't pay any mind - // to region obligations that arise (there shouldn't really be any - // anyhow). let cause = ObligationCause::misc(span, ast::DUMMY_NODE_ID); + let obligation = + util::predicate_for_builtin_bound(infcx.tcx, cause, bound, 0, ty); + let obligation = match obligation { + Ok(o) => o, + Err(..) => return false + }; + let result = SelectionContext::new(infcx) + .evaluate_obligation_conservatively(&obligation); + debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} => {:?}", + ty, bound, result); + + if result && (ty.has_infer_types() || ty.has_closure_types()) { + // Because of inference "guessing", selection can sometimes claim + // to succeed while the success requires a guess. To ensure + // this function's result remains infallible, we must confirm + // that guess. While imperfect, I believe this is sound. + + let mut fulfill_cx = FulfillmentContext::new(false); + + // We can use a dummy node-id here because we won't pay any mind + // to region obligations that arise (there shouldn't really be any + // anyhow). + let cause = ObligationCause::misc(span, ast::DUMMY_NODE_ID); - fulfill_cx.register_builtin_bound(infcx, ty, bound, cause); - - // Note: we only assume something is `Copy` if we can - // *definitively* show that it implements `Copy`. Otherwise, - // assume it is move; linear is always ok. - match fulfill_cx.select_all_or_error(infcx) { - Ok(()) => { - debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} success", - ty, - bound); - true - } - Err(e) => { - debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} errors={:?}", - ty, - bound, - e); - false + fulfill_cx.register_builtin_bound(infcx, ty, bound, cause); + + // Note: we only assume something is `Copy` if we can + // *definitively* show that it implements `Copy`. Otherwise, + // assume it is move; linear is always ok. + match fulfill_cx.select_all_or_error(infcx) { + Ok(()) => { + debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} success", + ty, + bound); + true + } + Err(e) => { + debug!("type_known_to_meet_builtin_bound: ty={:?} bound={:?} errors={:?}", + ty, + bound, + e); + false + } } + } else { + result } } diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index 2bc1e8faddb23..98ee94fe328e3 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -2851,18 +2851,37 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { -> Vec> { debug!("impl_or_trait_obligations(def_id={:?})", def_id); + let tcx = self.tcx(); - let predicates = self.tcx().lookup_predicates(def_id); - let predicates = predicates.instantiate(self.tcx(), substs); - let predicates = normalize_with_depth(self, cause.clone(), recursion_depth, &predicates); - let predicates = self.infcx().plug_leaks(skol_map, snapshot, &predicates); - - let mut obligations = predicates.obligations; - obligations.append( - &mut util::predicates_for_generics(cause, - recursion_depth, - &predicates.value)); - obligations + // To allow for one-pass evaluation of the nested obligation, + // each predicate must be preceded by the obligations required + // to normalize it. + // for example, if we have: + // impl> Foo for V where U::Item: Copy + // the impl will have the following predicates: + // ::Item = U, + // U: Iterator, U: Sized, + // V: Iterator, V: Sized, + // ::Item: Copy + // When we substitute, say, `V => IntoIter, U => $0`, the last + // obligation will normalize to `<$0 as Iterator>::Item = $1` and + // `$1: Copy`, so we must ensure the obligations are emitted in + // that order. + let predicates = tcx + .lookup_predicates(def_id) + .predicates.iter() + .flat_map(|predicate| { + let predicate = + normalize_with_depth(self, cause.clone(), recursion_depth, + &predicate.subst(tcx, substs)); + predicate.obligations.into_iter().chain( + Some(Obligation { + cause: cause.clone(), + recursion_depth: recursion_depth, + predicate: predicate.value + })) + }).collect(); + self.infcx().plug_leaks(skol_map, snapshot, &predicates) } #[allow(unused_comparisons)] diff --git a/src/test/compile-fail/cast-rfc0401.rs b/src/test/compile-fail/cast-rfc0401.rs index 4603ed0034734..d14b0fa9e6602 100644 --- a/src/test/compile-fail/cast-rfc0401.rs +++ b/src/test/compile-fail/cast-rfc0401.rs @@ -37,6 +37,7 @@ fn main() let f: f32 = 1.2; let v = 0 as *const u8; let fat_v : *const [u8] = unsafe { &*(0 as *const [u8; 1])}; + let fat_sv : *const [i8] = unsafe { &*(0 as *const [i8; 1])}; let foo: &Foo = &f; let _ = v as &u8; //~ ERROR non-scalar @@ -94,7 +95,7 @@ fn main() let _ = main as *mut str; //~ ERROR casting let _ = &f as *mut f32; //~ ERROR casting let _ = &f as *const f64; //~ ERROR casting - let _ = fat_v as usize; + let _ = fat_sv as usize; //~^ ERROR casting //~^^ HELP through a thin pointer first @@ -106,7 +107,7 @@ fn main() let _ = main.f as *const u32; //~ ERROR attempted access of field let cf: *const Foo = &0; - let _ = cf as *const [u8]; + let _ = cf as *const [u16]; //~^ ERROR casting //~^^ NOTE vtable kinds let _ = cf as *const Bar; diff --git a/src/test/compile-fail/infinite-instantiation.rs b/src/test/compile-fail/infinite-instantiation.rs index 9b02bf4cb85a8..28806b6e2ab8c 100644 --- a/src/test/compile-fail/infinite-instantiation.rs +++ b/src/test/compile-fail/infinite-instantiation.rs @@ -8,7 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//~^^^^^^^^^^ ERROR overflow // // We get an error message at the top of file (dummy span). // This is not helpful, but also kind of annoying to prevent, @@ -32,6 +31,7 @@ impl ToOpt for Option { } fn function(counter: usize, t: T) { +//~^ ERROR reached the recursion limit during monomorphization if counter > 0 { function(counter - 1, t.to_option()); // FIXME(#4287) Error message should be here. It should be diff --git a/src/test/compile-fail/recursion.rs b/src/test/compile-fail/recursion.rs index 55f3b99533653..b1d45a82276a8 100644 --- a/src/test/compile-fail/recursion.rs +++ b/src/test/compile-fail/recursion.rs @@ -8,12 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//~^^^^^^^^^^ ERROR overflow -// -// We get an error message at the top of file (dummy span). -// This is not helpful, but also kind of annoying to prevent, -// so for now just live with it. - enum Nil {NilValue} struct Cons {head:isize, tail:T} trait Dot {fn dot(&self, other:Self) -> isize;} @@ -26,7 +20,7 @@ impl Dot for Cons { } } fn test (n:isize, i:isize, first:T, second:T) ->isize { - match n { 0 => {first.dot(second)} + match n { 0 => {first.dot(second)} //~ ERROR overflow // FIXME(#4287) Error message should be here. It should be // a type error to instantiate `test` at a type other than T. _ => {test (n-1, i+1, Cons {head:2*i+1, tail:first}, Cons{head:i*i, tail:second})} diff --git a/src/test/run-pass/trait-copy-guessing.rs b/src/test/run-pass/trait-copy-guessing.rs new file mode 100644 index 0000000000000..71cd3c9441e10 --- /dev/null +++ b/src/test/run-pass/trait-copy-guessing.rs @@ -0,0 +1,46 @@ +// 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. + +// "guessing" in trait selection can affect `copy_or_move`. Check that this +// is correctly handled. I am not sure what is the "correct" behaviour, +// but we should at least not ICE. + +use std::mem; + +struct U([u8; 1337]); + +struct S<'a,T:'a>(&'a T); +impl<'a, T> Clone for S<'a, T> { fn clone(&self) -> Self { S(self.0) } } +/// This impl triggers inference "guessing" - S<_>: Copy => _ = U +impl<'a> Copy for S<'a, Option> {} + +fn assert_impls_fnR>(_: &T){} + +fn main() { + let n = None; + let e = S(&n); + let f = || { + // S being copy is critical for this to work + drop(e); + mem::size_of_val(e.0) + }; + assert_impls_fn(&f); + assert_eq!(f(), 1337+1); + + assert_eq!((|| { + // S being Copy is not critical here, but + // we check it anyway. + let n = None; + let e = S(&n); + let ret = mem::size_of_val(e.0); + drop(e); + ret + })(), 1337+1); +} From f921982e6019497f1dd5b35c79b5077c35331b5a Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 22 Oct 2015 18:25:02 +0300 Subject: [PATCH 7/8] fix pretty --- src/test/run-pass/issue-29147.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/run-pass/issue-29147.rs b/src/test/run-pass/issue-29147.rs index 460e92967e8e6..026b98905d099 100644 --- a/src/test/run-pass/issue-29147.rs +++ b/src/test/run-pass/issue-29147.rs @@ -20,7 +20,8 @@ pub struct S4(Option>>>,Option>>>); pub struct S5(Option>>>,Option>>>,Option); trait Foo { fn xxx(&self); } -trait Bar {} // anything local or #[fundamental] +/// some local of #[fundamental] trait +trait Bar {} impl Foo for T where T: Bar, T: Sync { fn xxx(&self) {} From 5982594c7e6fd1ddd7084c9d6d125426a180c964 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 15 Nov 2015 20:11:42 +0200 Subject: [PATCH 8/8] address review comments --- src/librustc/middle/traits/select.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index 98ee94fe328e3..ba69632fde1b1 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -423,7 +423,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { }) } - + /// Evaluates the predicates in `predicates` recursively. Note that + /// this applies projections in the predicates, and therefore + /// is run within an inference probe. fn evaluate_predicates_recursively<'a,'o,I>(&mut self, stack: TraitObligationStackList<'o, 'tcx>, predicates: I)