From 8f0e73ef55f6cf837555a0c144c00844eefa97b9 Mon Sep 17 00:00:00 2001 From: Aaron Turon Date: Tue, 8 Mar 2016 15:23:52 -0800 Subject: [PATCH] Address review comments --- src/librustc/middle/traits/project.rs | 10 +- src/librustc/middle/traits/specialize/mod.rs | 140 +++++++----------- .../traits/specialize/specialization_graph.rs | 21 +-- src/librustc_typeck/check/mod.rs | 4 +- .../specialization-default-projection.rs | 16 +- .../specialization-default-types.rs | 4 +- .../specialization-feature-gate-default.rs | 4 +- .../specialization-feature-gate-overlap.rs | 4 +- .../compile-fail/specialization-no-default.rs | 3 + 9 files changed, 99 insertions(+), 107 deletions(-) diff --git a/src/librustc/middle/traits/project.rs b/src/librustc/middle/traits/project.rs index 58d36b45ecb..e1d811d450d 100644 --- a/src/librustc/middle/traits/project.rs +++ b/src/librustc/middle/traits/project.rs @@ -100,21 +100,21 @@ pub enum ProjectionMode { } impl ProjectionMode { - pub fn topmost(&self) -> bool { + pub fn is_topmost(&self) -> bool { match *self { ProjectionMode::Topmost => true, _ => false, } } - pub fn any_final(&self) -> bool { + pub fn is_any_final(&self) -> bool { match *self { ProjectionMode::AnyFinal => true, _ => false, } } - pub fn any(&self) -> bool { + pub fn is_any(&self) -> bool { match *self { ProjectionMode::Any => true, _ => false, @@ -665,7 +665,7 @@ fn project_type<'cx,'tcx>( // In Any (i.e. trans) mode, all projections succeed; // otherwise, we need to be sensitive to `default` and // specialization. - if !selcx.projection_mode().any() { + if !selcx.projection_mode().is_any() { if let ProjectionTyCandidate::Impl(ref impl_data) = candidate { if let Some(node_item) = assoc_ty_def(selcx, impl_data.impl_def_id, @@ -1116,7 +1116,7 @@ fn assoc_ty_def<'cx, 'tcx>(selcx: &SelectionContext<'cx, 'tcx>, impl_def_id: Def { let trait_def_id = selcx.tcx().impl_trait_ref(impl_def_id).unwrap().def_id; - if selcx.projection_mode().topmost() { + if selcx.projection_mode().is_topmost() { let impl_node = specialization_graph::Node::Impl(impl_def_id); for item in impl_node.items(selcx.tcx()) { if let ty::TypeTraitItem(assoc_ty) = item { diff --git a/src/librustc/middle/traits/specialize/mod.rs b/src/librustc/middle/traits/specialize/mod.rs index 9cdcc870404..b557db9a8b2 100644 --- a/src/librustc/middle/traits/specialize/mod.rs +++ b/src/librustc/middle/traits/specialize/mod.rs @@ -25,7 +25,7 @@ use middle::def_id::DefId; use middle::infer::{self, InferCtxt, TypeOrigin}; use middle::region; use middle::subst::{Subst, Substs}; -use middle::traits::{self, ProjectionMode}; +use middle::traits::ProjectionMode; use middle::ty; use syntax::codemap::DUMMY_SP; @@ -41,45 +41,43 @@ pub struct Overlap<'a, 'tcx: 'a> { /// Given a subst for the requested impl, translate it to a subst /// appropriate for the actual item definition (whether it be in that impl, /// a parent impl, or the trait). -// -// When we have selected one impl, but are actually using item definitions from -// a parent impl providing a default, we need a way to translate between the -// type parameters of the two impls. Here the `source_impl` is the one we've -// selected, and `source_substs` is a substitution of its generics (and -// possibly some relevant `FnSpace` variables as well). And `target_node` is -// the impl/trait we're actually going to get the definition from. The resulting -// substitution will map from `target_node`'s generics to `source_impl`'s -// generics as instantiated by `source_subst`. -// -// For example, consider the following scenario: -// -// ```rust -// trait Foo { ... } -// impl Foo for (T, U) { ... } // target impl -// impl Foo for (V, V) { ... } // source impl -// ``` -// -// Suppose we have selected "source impl" with `V` instantiated with `u32`. -// This function will produce a substitution with `T` and `U` both mapping to `u32`. -// -// Where clauses add some trickiness here, because they can be used to "define" -// an argument indirectly: -// -// ```rust -// impl<'a, I, T: 'a> Iterator for Cloned -// where I: Iterator, T: Clone -// ``` -// -// In a case like this, the substitution for `T` is determined indirectly, -// through associated type projection. We deal with such cases by using -// *fulfillment* to relate the two impls, requiring that all projections are -// resolved. +/// When we have selected one impl, but are actually using item definitions from +/// a parent impl providing a default, we need a way to translate between the +/// type parameters of the two impls. Here the `source_impl` is the one we've +/// selected, and `source_substs` is a substitution of its generics (and +/// possibly some relevant `FnSpace` variables as well). And `target_node` is +/// the impl/trait we're actually going to get the definition from. The resulting +/// substitution will map from `target_node`'s generics to `source_impl`'s +/// generics as instantiated by `source_subst`. +/// +/// For example, consider the following scenario: +/// +/// ```rust +/// trait Foo { ... } +/// impl Foo for (T, U) { ... } // target impl +/// impl Foo for (V, V) { ... } // source impl +/// ``` +/// +/// Suppose we have selected "source impl" with `V` instantiated with `u32`. +/// This function will produce a substitution with `T` and `U` both mapping to `u32`. +/// +/// Where clauses add some trickiness here, because they can be used to "define" +/// an argument indirectly: +/// +/// ```rust +/// impl<'a, I, T: 'a> Iterator for Cloned +/// where I: Iterator, T: Clone +/// ``` +/// +/// In a case like this, the substitution for `T` is determined indirectly, +/// through associated type projection. We deal with such cases by using +/// *fulfillment* to relate the two impls, requiring that all projections are +/// resolved. pub fn translate_substs<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, source_impl: DefId, source_substs: Substs<'tcx>, target_node: specialization_graph::Node) - -> Substs<'tcx> -{ + -> Substs<'tcx> { let source_trait_ref = infcx.tcx .impl_trait_ref(source_impl) .unwrap() @@ -116,29 +114,16 @@ pub fn translate_substs<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, target_substs.with_method_from_subst(&source_substs) } - -fn skolemizing_subst_for_impl<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, - impl_def_id: DefId) - -> Substs<'tcx> -{ - let impl_generics = infcx.tcx.lookup_item_type(impl_def_id).generics; - - let types = impl_generics.types.map(|def| infcx.tcx.mk_param_from_def(def)); - - // TODO: figure out what we actually want here - let regions = impl_generics.regions.map(|_| ty::Region::ReStatic); - // |d| infcx.next_region_var(infer::RegionVariableOrigin::EarlyBoundRegion(span, d.name))); - - Substs::new(types, regions) -} - /// Is impl1 a specialization of impl2? /// /// Specialization is determined by the sets of types to which the impls apply; /// impl1 specializes impl2 if it applies to a subset of the types impl2 applies /// to. pub fn specializes(tcx: &ty::ctxt, impl1_def_id: DefId, impl2_def_id: DefId) -> bool { - if !tcx.sess.features.borrow().specialization { + // The feature gate should prevent introducing new specializations, but not + // taking advantage of upstream ones. + if !tcx.sess.features.borrow().specialization && + (impl1_def_id.is_local() || impl2_def_id.is_local()) { return false; } @@ -156,33 +141,24 @@ pub fn specializes(tcx: &ty::ctxt, impl1_def_id: DefId, impl2_def_id: DefId) -> // Currently we do not allow e.g. a negative impl to specialize a positive one if tcx.trait_impl_polarity(impl1_def_id) != tcx.trait_impl_polarity(impl2_def_id) { - return false + return false; } let mut infcx = infer::normalizing_infer_ctxt(tcx, &tcx.tables, ProjectionMode::Topmost); - // Skiolemize impl1: we want to prove that "for all types matched by impl1, - // those types are also matched by impl2". - let impl1_substs = skolemizing_subst_for_impl(&infcx, impl1_def_id); - let (impl1_trait_ref, impl1_obligations) = { - let selcx = &mut SelectionContext::new(&infcx); - impl_trait_ref_and_oblig(selcx, impl1_def_id, &impl1_substs) - }; + // create a parameter environment corresponding to a (skolemized) instantiation of impl1 + let scheme = tcx.lookup_item_type(impl1_def_id); + let predicates = tcx.lookup_predicates(impl1_def_id); + let penv = tcx.construct_parameter_environment(DUMMY_SP, + &scheme.generics, + &predicates, + region::DUMMY_CODE_EXTENT); + let impl1_trait_ref = tcx.impl_trait_ref(impl1_def_id) + .unwrap() + .subst(tcx, &penv.free_substs); - // Add impl1's obligations as assumptions to the environment. - let impl1_predicates: Vec<_> = impl1_obligations.iter() - .cloned() - .map(|oblig| oblig.predicate) - .collect(); - infcx.parameter_environment = ty::ParameterEnvironment { - tcx: tcx, - free_substs: impl1_substs, - implicit_region_bound: ty::ReEmpty, // TODO: is this OK? - caller_bounds: impl1_predicates, - selection_cache: traits::SelectionCache::new(), - evaluation_cache: traits::EvaluationCache::new(), - free_id_outlive: region::DUMMY_CODE_EXTENT, // TODO: is this OK? - }; + // Install the parameter environment, which means we take the predicates of impl1 as assumptions: + infcx.parameter_environment = penv; // Attempt to prove that impl2 applies, given all of the above. fulfill_implication(&infcx, impl1_trait_ref, impl2_def_id).is_ok() @@ -196,9 +172,8 @@ pub fn specializes(tcx: &ty::ctxt, impl1_def_id: DefId, impl2_def_id: DefId) -> fn fulfill_implication<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, source_trait_ref: ty::TraitRef<'tcx>, target_impl: DefId) - -> Result, ()> -{ - infcx.probe(|_| { + -> Result, ()> { + infcx.commit_if_ok(|_| { let selcx = &mut SelectionContext::new(&infcx); let target_substs = fresh_type_vars_for_impl(&infcx, DUMMY_SP, target_impl); let (target_trait_ref, obligations) = impl_trait_ref_and_oblig(selcx, @@ -227,23 +202,20 @@ fn fulfill_implication<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, if let Err(errors) = infer::drain_fulfillment_cx(&infcx, &mut fulfill_cx, &()) { // no dice! - debug!("fulfill_implication: for impls on {:?} and {:?}, could not fulfill: {:?} \ - given {:?}", + debug!("fulfill_implication: for impls on {:?} and {:?}, could not fulfill: {:?} given \ + {:?}", source_trait_ref, target_trait_ref, errors, infcx.parameter_environment.caller_bounds); Err(()) } else { - debug!("fulfill_implication: an impl for {:?} specializes {:?} (`where` clauses \ - elided)", + debug!("fulfill_implication: an impl for {:?} specializes {:?} (`where` clauses elided)", source_trait_ref, target_trait_ref); // Now resolve the *substitution* we built for the target earlier, replacing // the inference variables inside with whatever we got from fulfillment. - - // TODO: should this use `fully_resolve` instead? Ok(infcx.resolve_type_vars_if_possible(&target_substs)) } }) diff --git a/src/librustc/middle/traits/specialize/specialization_graph.rs b/src/librustc/middle/traits/specialize/specialization_graph.rs index def50766f4e..77588a3e070 100644 --- a/src/librustc/middle/traits/specialize/specialization_graph.rs +++ b/src/librustc/middle/traits/specialize/specialization_graph.rs @@ -65,14 +65,15 @@ impl Graph { let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap(); let trait_def_id = trait_ref.def_id; - debug!("inserting TraitRef {:?} into specialization graph", trait_ref); + debug!("insert({:?}): inserting TraitRef {:?} into specialization graph", + impl_def_id, trait_ref); // if the reference itself contains an earlier error (e.g., due to a // resolution failure), then we just insert the impl at the top level of // the graph and claim that there's no overlap (in order to supress // bogus errors). if trait_ref.references_error() { - debug!("Inserting dummy node for erroneous TraitRef {:?}, \ + debug!("insert: inserting dummy node for erroneous TraitRef {:?}, \ impl_def_id={:?}, trait_def_id={:?}", trait_ref, impl_def_id, trait_def_id); @@ -101,15 +102,15 @@ impl Graph { let ge = specializes(tcx, possible_sibling, impl_def_id); if le && !ge { - let parent_trait_ref = tcx.impl_trait_ref(possible_sibling).unwrap(); - debug!("descending as child of TraitRef {:?}", parent_trait_ref); + debug!("descending as child of TraitRef {:?}", + tcx.impl_trait_ref(possible_sibling).unwrap()); // the impl specializes possible_sibling parent = possible_sibling; continue 'descend; } else if ge && !le { - let child_trait_ref = tcx.impl_trait_ref(possible_sibling).unwrap(); - debug!("placing as parent of TraitRef {:?}", child_trait_ref); + debug!("placing as parent of TraitRef {:?}", + tcx.impl_trait_ref(possible_sibling).unwrap()); // possible_sibling specializes the impl *slot = impl_def_id; @@ -158,10 +159,10 @@ impl Graph { } } -#[derive(Debug, Copy, Clone)] /// A node in the specialization graph is either an impl or a trait /// definition; either can serve as a source of item definitions. /// There is always exactly one trait definition node: the root. +#[derive(Debug, Copy, Clone)] pub enum Node { Impl(DefId), Trait(DefId), @@ -316,7 +317,7 @@ impl<'a, 'tcx> Iterator for ConstDefs<'a, 'tcx> { } impl<'a, 'tcx> Ancestors<'a, 'tcx> { - /// Seach the items from the given ancestors, returning each type definition + /// Search the items from the given ancestors, returning each type definition /// with the given name. pub fn type_defs(self, tcx: &'a ty::ctxt<'tcx>, name: Name) -> TypeDefs<'a, 'tcx> { let iter = self.flat_map(move |node| { @@ -337,7 +338,7 @@ impl<'a, 'tcx> Ancestors<'a, 'tcx> { TypeDefs { iter: Box::new(iter) } } - /// Seach the items from the given ancestors, returning each fn definition + /// Search the items from the given ancestors, returning each fn definition /// with the given name. pub fn fn_defs(self, tcx: &'a ty::ctxt<'tcx>, name: Name) -> FnDefs<'a, 'tcx> { let iter = self.flat_map(move |node| { @@ -358,7 +359,7 @@ impl<'a, 'tcx> Ancestors<'a, 'tcx> { FnDefs { iter: Box::new(iter) } } - /// Seach the items from the given ancestors, returning each const + /// Search the items from the given ancestors, returning each const /// definition with the given name. pub fn const_defs(self, tcx: &'a ty::ctxt<'tcx>, name: Name) -> ConstDefs<'a, 'tcx> { let iter = self.flat_map(move |node| { diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index cdff78d01e3..534f10103b5 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -870,8 +870,8 @@ fn report_forbidden_specialization(tcx: &ty::ctxt, { let mut err = struct_span_err!( tcx.sess, impl_item.span, E0520, - "item `{}` is provided by an implementation that specializes \ - another, but the item in the parent implementations is not \ + "item `{}` is provided by an `impl` that specializes \ + another, but the item in the parent `impl` is not \ marked `default` and so it cannot be specialized.", impl_item.name); diff --git a/src/test/compile-fail/specialization-default-projection.rs b/src/test/compile-fail/specialization-default-projection.rs index 3f85a503f62..377838f2a08 100644 --- a/src/test/compile-fail/specialization-default-projection.rs +++ b/src/test/compile-fail/specialization-default-projection.rs @@ -25,10 +25,22 @@ impl Foo for u8 { } fn generic() -> ::Assoc { - () //~ ERROR + // `T` could be some downstream crate type that specializes (or, + // for that matter, `u8`). + + () //~ ERROR E0308 +} + +fn monomorphic() -> () { + // Even though we know that `()` is not specialized in a + // downstream crate, typeck refuses to project here. + + generic::<()>() //~ ERROR E0308 } fn main() { + // No error here, we CAN project from `u8`, as there is no `default` + // in that impl. let s: String = generic::(); - println!("{}", s); // bad news + println!("{}", s); // bad news if this all compiles } diff --git a/src/test/compile-fail/specialization-default-types.rs b/src/test/compile-fail/specialization-default-types.rs index dce1db06a92..3c2e3d5a36c 100644 --- a/src/test/compile-fail/specialization-default-types.rs +++ b/src/test/compile-fail/specialization-default-types.rs @@ -22,7 +22,7 @@ trait Example { impl Example for T { default type Output = Box; default fn generate(self) -> Self::Output { - Box::new(self) //~ ERROR + Box::new(self) //~ ERROR E0308 } } @@ -32,7 +32,7 @@ impl Example for bool { } fn trouble(t: T) -> Box { - Example::generate(t) //~ ERROR + Example::generate(t) //~ ERROR E0308 } fn weaponize() -> bool { diff --git a/src/test/compile-fail/specialization-feature-gate-default.rs b/src/test/compile-fail/specialization-feature-gate-default.rs index caac21278d4..e7c194ce84d 100644 --- a/src/test/compile-fail/specialization-feature-gate-default.rs +++ b/src/test/compile-fail/specialization-feature-gate-default.rs @@ -8,12 +8,14 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Check that specialization must be ungated to use the `default` keyword + trait Foo { fn foo(&self); } impl Foo for T { - default fn foo(&self) {} //~ ERROR + default fn foo(&self) {} //~ ERROR specialization is unstable } fn main() {} diff --git a/src/test/compile-fail/specialization-feature-gate-overlap.rs b/src/test/compile-fail/specialization-feature-gate-overlap.rs index a7918e4426d..d11ab56ff7e 100644 --- a/src/test/compile-fail/specialization-feature-gate-overlap.rs +++ b/src/test/compile-fail/specialization-feature-gate-overlap.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Check that writing an overlapping impl is not allow unless specialization is ungated. + trait Foo { fn foo(&self); } @@ -16,7 +18,7 @@ impl Foo for T { fn foo(&self) {} } -impl Foo for u8 { //~ ERROR +impl Foo for u8 { //~ ERROR E0119 fn foo(&self) {} } diff --git a/src/test/compile-fail/specialization-no-default.rs b/src/test/compile-fail/specialization-no-default.rs index 88cd00e69c4..96156168543 100644 --- a/src/test/compile-fail/specialization-no-default.rs +++ b/src/test/compile-fail/specialization-no-default.rs @@ -10,6 +10,9 @@ #![feature(specialization)] +// Check a number of scenarios in which one impl tries to override another, +// without correctly using `default`. + //////////////////////////////////////////////////////////////////////////////// // Test 1: one layer of specialization, multiple methods, missing `default` ////////////////////////////////////////////////////////////////////////////////