Address review comments

aturon · aturon · commit 8f0e73ef55f6 · 2016-03-14T15:05:13.000-07:00
diff --git 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
@@ -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<T, U> Foo for (T, U) { ... }  // target impl
-// impl<V> 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<I>
-//    where I: Iterator<Item=&'a T>, 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<T, U> Foo for (T, U) { ... }  // target impl
+/// impl<V> 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<I>
+///    where I: Iterator<Item=&'a T>, 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<Substs<'tcx>, ()>
-{
-    infcx.probe(|_| {
+                                 -> Result<Substs<'tcx>, ()> {
+    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
@@ -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
@@ -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
@@ -25,10 +25,22 @@ impl Foo for u8 {
 }
 
 fn generic<T>() -> <T as Foo>::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::<u8>();
-    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
@@ -22,7 +22,7 @@ trait Example {
 impl<T> Example for T {
     default type Output = Box<T>;
     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: T) -> Box<T> {
-    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
@@ -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<T> 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
diff --git a/src/test/compile-fail/specialization-no-default.rs b/src/test/compile-fail/specialization-no-default.rs

Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ trait Example {`
`22`	`22`	`impl<T> Example for T {`
`23`	`23`	`default type Output = Box<T>;`
`24`	`24`	`default fn generate(self) -> Self::Output {`
`25`		`- Box::new(self) //~ ERROR`
	`25`	`+ Box::new(self) //~ ERROR E0308`
`26`	`26`	`}`
`27`	`27`	`}`
`28`	`28`
`@@ -32,7 +32,7 @@ impl Example for bool {`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`fn trouble<T>(t: T) -> Box<T> {`
`35`		`- Example::generate(t) //~ ERROR`
	`35`	`+ Example::generate(t) //~ ERROR E0308`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`fn weaponize() -> bool {`
Original file line number	Diff line number	Diff line change
`@@ -8,12 +8,14 @@`
`8`	`8`	`// option. This file may not be copied, modified, or distributed`
`9`	`9`	`// except according to those terms.`
`10`	`10`
	`11`	+// Check that specialization must be ungated to use the `default` keyword
	`12`	`+`
`11`	`13`	`trait Foo {`
`12`	`14`	`fn foo(&self);`
`13`	`15`	`}`
`14`	`16`
`15`	`17`	`impl<T> Foo for T {`
`16`		`- default fn foo(&self) {} //~ ERROR`
	`18`	`+ default fn foo(&self) {} //~ ERROR specialization is unstable`
`17`	`19`	`}`
`18`	`20`
`19`	`21`	`fn main() {}`