Auto merge of #23038 - nikomatsakis:issue-22978-defaulted-coherence, …

…r=flaper87

Fixes #22978.

r? @flaper87

src/librustc/metadata/common.rs

@@ -254,3 +254,5 @@ pub const tag_codemap: uint = 0xa1;
 pub const tag_codemap_filemap: uint = 0xa2;
 
 pub const tag_item_super_predicates: uint = 0xa3;
+
+pub const tag_defaulted_trait: uint = 0xa4;

src/librustc/metadata/csearch.rs

@@ -407,7 +407,7 @@ pub fn is_associated_type(cstore: &cstore::CStore, def: ast::DefId) -> bool {
     decoder::is_associated_type(&*cdata, def.node)
 }
 
-pub fn is_default_trait(cstore: &cstore::CStore, def: ast::DefId) -> bool {
-    let cdata = cstore.get_crate_data(def.krate);
-    decoder::is_default_trait(&*cdata, def.node)
+pub fn is_defaulted_trait(cstore: &cstore::CStore, trait_def_id: ast::DefId) -> bool {
+    let cdata = cstore.get_crate_data(trait_def_id.krate);
+    decoder::is_defaulted_trait(&*cdata, trait_def_id.node)
 }

src/librustc/metadata/decoder.rs

@@ -1537,12 +1537,11 @@ pub fn is_associated_type(cdata: Cmd, id: ast::NodeId) -> bool {
     }
 }
 
-pub fn is_default_trait<'tcx>(cdata: Cmd, id: ast::NodeId) -> bool {
-    let item_doc = lookup_item(id, cdata.data());
-    match item_family(item_doc) {
-        Family::DefaultImpl => true,
-        _ => false
-    }
+pub fn is_defaulted_trait<'tcx>(cdata: Cmd, trait_id: ast::NodeId) -> bool {
+    let trait_doc = lookup_item(trait_id, cdata.data());
+    assert!(item_family(trait_doc) == Family::Trait);
+    let defaulted_doc = reader::get_doc(trait_doc, tag_defaulted_trait);
+    reader::doc_as_u8(defaulted_doc) != 0
 }
 
 pub fn get_imported_filemaps(metadata: &[u8]) -> Vec<codemap::FileMap> {

src/librustc/metadata/encoder.rs

@@ -1288,6 +1288,7 @@ fn encode_info_for_item(ecx: &EncodeContext,
         let trait_predicates = ty::lookup_predicates(tcx, def_id);
         encode_unsafety(rbml_w, trait_def.unsafety);
         encode_paren_sugar(rbml_w, trait_def.paren_sugar);
+        encode_defaulted(rbml_w, ty::trait_has_default_impl(tcx, def_id));
         encode_associated_type_names(rbml_w, &trait_def.associated_type_names);
         encode_generics(rbml_w, ecx, &trait_def.generics, &trait_predicates, tag_item_generics);
         encode_predicates(rbml_w, ecx, &ty::lookup_super_predicates(tcx, def_id),
@@ -1660,6 +1661,11 @@ fn encode_paren_sugar(rbml_w: &mut Encoder, paren_sugar: bool) {
     rbml_w.wr_tagged_u8(tag_paren_sugar, byte);
 }
 
+fn encode_defaulted(rbml_w: &mut Encoder, is_defaulted: bool) {
+    let byte: u8 = if is_defaulted {1} else {0};
+    rbml_w.wr_tagged_u8(tag_defaulted_trait, byte);
+}
+
 fn encode_associated_type_names(rbml_w: &mut Encoder, names: &[ast::Name]) {
     rbml_w.start_tag(tag_associated_type_names);
     for &name in names {

src/librustc/middle/ty.rs

@@ -757,8 +757,8 @@ pub struct ctxt<'tcx> {
     /// Maps a trait onto a list of impls of that trait.
     pub trait_impls: RefCell<DefIdMap<Rc<RefCell<Vec<ast::DefId>>>>>,
 
-    /// Maps a trait onto a list of *default* trait implementations
-    default_trait_impls: RefCell<DefIdMap<ast::DefId>>,
+    /// A set of traits that have a default impl
+    traits_with_default_impls: RefCell<DefIdMap<()>>,
 
     /// Maps a DefId of a type to a list of its inherent impls.
     /// Contains implementations of methods that are inherent to a type.
@@ -2591,7 +2591,7 @@ pub fn mk_ctxt<'tcx>(s: Session,
         destructor_for_type: RefCell::new(DefIdMap()),
         destructors: RefCell::new(DefIdSet()),
         trait_impls: RefCell::new(DefIdMap()),
-        default_trait_impls: RefCell::new(DefIdMap()),
+        traits_with_default_impls: RefCell::new(DefIdMap()),
         inherent_impls: RefCell::new(DefIdMap()),
         impl_items: RefCell::new(DefIdMap()),
         used_unsafe: RefCell::new(NodeSet()),
@@ -5975,32 +5975,22 @@ pub fn item_variances(tcx: &ctxt, item_id: ast::DefId) -> Rc<ItemVariances> {
         || Rc::new(csearch::get_item_variances(&tcx.sess.cstore, item_id)))
 }
 
-pub fn trait_default_impl(tcx: &ctxt, trait_def_id: DefId) -> Option<ast::DefId> {
-    match tcx.default_trait_impls.borrow().get(&trait_def_id) {
-        Some(id) => Some(*id),
-        None => None
-    }
-}
-
 pub fn trait_has_default_impl(tcx: &ctxt, trait_def_id: DefId) -> bool {
+    populate_implementations_for_trait_if_necessary(tcx, trait_def_id);
     match tcx.lang_items.to_builtin_kind(trait_def_id) {
         Some(BoundSend) | Some(BoundSync) => true,
-        _ => tcx.default_trait_impls.borrow().contains_key(&trait_def_id)
+        _ => tcx.traits_with_default_impls.borrow().contains_key(&trait_def_id),
     }
 }
 
 /// Records a trait-to-implementation mapping.
-pub fn record_default_trait_implementation(tcx: &ctxt,
-                                           trait_def_id: DefId,
-                                           impl_def_id: DefId) {
-
+pub fn record_trait_has_default_impl(tcx: &ctxt, trait_def_id: DefId) {
     // We're using the latest implementation found as the reference one.
     // Duplicated implementations are caught and reported in the coherence
     // step.
-    tcx.default_trait_impls.borrow_mut().insert(trait_def_id, impl_def_id);
+    tcx.traits_with_default_impls.borrow_mut().insert(trait_def_id, ());
 }
 
-
 /// Records a trait-to-implementation mapping.
 pub fn record_trait_implementation(tcx: &ctxt,
                                    trait_def_id: DefId,
@@ -6031,8 +6021,7 @@ pub fn populate_implementations_for_type_if_necessary(tcx: &ctxt,
     debug!("populate_implementations_for_type_if_necessary: searching for {:?}", type_id);
 
     let mut inherent_impls = Vec::new();
-    csearch::each_implementation_for_type(&tcx.sess.cstore, type_id,
-            |impl_def_id| {
+    csearch::each_implementation_for_type(&tcx.sess.cstore, type_id, |impl_def_id| {
         let impl_items = csearch::get_impl_items(&tcx.sess.cstore, impl_def_id);
 
         // Record the trait->implementation mappings, if applicable.
@@ -6078,27 +6067,20 @@ pub fn populate_implementations_for_trait_if_necessary(
     if trait_id.krate == LOCAL_CRATE {
         return
     }
+
     if tcx.populated_external_traits.borrow().contains(&trait_id) {
         return
     }
 
-    csearch::each_implementation_for_trait(&tcx.sess.cstore, trait_id,
-            |implementation_def_id|{
-        let impl_items = csearch::get_impl_items(&tcx.sess.cstore, implementation_def_id);
+    if csearch::is_defaulted_trait(&tcx.sess.cstore, trait_id) {
+        record_trait_has_default_impl(tcx, trait_id);
+    }
 
-        if csearch::is_default_trait(&tcx.sess.cstore, implementation_def_id) {
-            record_default_trait_implementation(tcx, trait_id,
-                                                implementation_def_id);
-            tcx.populated_external_traits.borrow_mut().insert(trait_id);
+    csearch::each_implementation_for_trait(&tcx.sess.cstore, trait_id, |implementation_def_id| {
+        let impl_items = csearch::get_impl_items(&tcx.sess.cstore, implementation_def_id);
 
-            // Nothing else to do for default trait implementations since
-            // they are not allowed to have type parameters, methods, or any
-            // other item that could be associated to a trait implementation.
-            return;
-        } else {
-            // Record the trait->implementation mapping.
-            record_trait_implementation(tcx, trait_id, implementation_def_id);
-        }
+        // Record the trait->implementation mapping.
+        record_trait_implementation(tcx, trait_id, implementation_def_id);
 
         // For any methods that use a default implementation, add them to
         // the map. This is a bit unfortunate.
@@ -6108,8 +6090,8 @@ pub fn populate_implementations_for_trait_if_necessary(
                 MethodTraitItem(method) => {
                     if let Some(source) = method.provided_source {
                         tcx.provided_method_sources
-                           .borrow_mut()
-                           .insert(method_def_id, source);
+                            .borrow_mut()
+                            .insert(method_def_id, source);
                     }
                 }
                 TypeTraitItem(_) => {}

src/librustc_typeck/coherence/mod.rs

@@ -49,7 +49,6 @@ use syntax::visit;
 use util::nodemap::{DefIdMap, FnvHashMap};
 use util::ppaux::Repr;
 
-mod impls;
 mod orphan;
 mod overlap;
 mod unsafety;
@@ -583,7 +582,6 @@ pub fn check_coherence(crate_context: &CrateCtxt) {
         inference_context: new_infer_ctxt(crate_context.tcx),
         inherent_impls: RefCell::new(FnvHashMap()),
     }.check(crate_context.tcx.map.krate());
-    impls::check(crate_context.tcx);
     unsafety::check(crate_context.tcx);
     orphan::check(crate_context.tcx);
     overlap::check(crate_context.tcx);

src/librustc_typeck/coherence/orphan.rs

@@ -37,10 +37,13 @@ impl<'cx, 'tcx> OrphanChecker<'cx, 'tcx> {
                        a trait or new type instead");
         }
     }
-}
 
-impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
-    fn visit_item(&mut self, item: &ast::Item) {
+    /// Checks exactly one impl for orphan rules and other such
+    /// restrictions.  In this fn, it can happen that multiple errors
+    /// apply to a specific impl, so just return after reporting one
+    /// to prevent inundating the user with a bunch of similar error
+    /// reports.
+    fn check_item(&self, item: &ast::Item) {
         let def_id = ast_util::local_def(item.id);
         match item.node {
             ast::ItemImpl(_, _, _, None, _, _) => {
@@ -63,13 +66,15 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
                         span_err!(self.tcx.sess, item.span, E0118,
                                   "no base type found for inherent implementation; \
                                    implement a trait or new type instead");
+                        return;
                     }
                 }
             }
             ast::ItemImpl(_, _, _, Some(_), _, _) => {
                 // "Trait" impl
                 debug!("coherence2::orphan check: trait impl {}", item.repr(self.tcx));
-                let trait_def_id = ty::impl_trait_ref(self.tcx, def_id).unwrap().def_id;
+                let trait_ref = ty::impl_trait_ref(self.tcx, def_id).unwrap();
+                let trait_def_id = trait_ref.def_id;
                 match traits::orphan_check(self.tcx, def_id) {
                     Ok(()) => { }
                     Err(traits::OrphanCheckErr::NoLocalInputType) => {
@@ -80,6 +85,7 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
                                  types defined in this crate; \
                                  only traits defined in the current crate can be \
                                  implemented for arbitrary types");
+                            return;
                         }
                     }
                     Err(traits::OrphanCheckErr::UncoveredTy(param_ty)) => {
@@ -89,9 +95,100 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
                                      some local type (e.g. `MyStruct<T>`); only traits defined in \
                                      the current crate can be implemented for a type parameter",
                                     param_ty.user_string(self.tcx));
+                            return;
                         }
                     }
                 }
+
+                // In addition to the above rules, we restrict impls of defaulted traits
+                // so that they can only be implemented on structs/enums. To see why this
+                // restriction exists, consider the following example (#22978). Imagine
+                // that crate A defines a defaulted trait `Foo` and a fn that operates
+                // on pairs of types:
+                //
+                // ```
+                // // Crate A
+                // trait Foo { }
+                // impl Foo for .. { }
+                // fn two_foos<A:Foo,B:Foo>(..) {
+                //     one_foo::<(A,B)>(..)
+                // }
+                // fn one_foo<T:Foo>(..) { .. }
+                // ```
+                //
+                // This type-checks fine; in particular the fn
+                // `two_foos` is able to conclude that `(A,B):Foo`
+                // because `A:Foo` and `B:Foo`.
+                //
+                // Now imagine that crate B comes along and does the following:
+                //
+                // ```
+                // struct A { }
+                // struct B { }
+                // impl Foo for A { }
+                // impl Foo for B { }
+                // impl !Send for (A, B) { }
+                // ```
+                //
+                // This final impl is legal according to the orpan
+                // rules, but it invalidates the reasoning from
+                // `two_foos` above.
+                debug!("trait_ref={} trait_def_id={} trait_has_default_impl={}",
+                       trait_ref.repr(self.tcx),
+                       trait_def_id.repr(self.tcx),
+                       ty::trait_has_default_impl(self.tcx, trait_def_id));
+                if
+                    ty::trait_has_default_impl(self.tcx, trait_def_id) &&
+                    trait_def_id.krate != ast::LOCAL_CRATE
+                {
+                    let self_ty = trait_ref.self_ty();
+                    let opt_self_def_id = match self_ty.sty {
+                        ty::ty_struct(self_def_id, _) | ty::ty_enum(self_def_id, _) =>
+                            Some(self_def_id),
+                        ty::ty_uniq(..) =>
+                            self.tcx.lang_items.owned_box(),
+                        _ =>
+                            None
+                    };
+
+                    let msg = match opt_self_def_id {
+                        // We only want to permit structs/enums, but not *all* structs/enums.
+                        // They must be local to the current crate, so that people
+                        // can't do `unsafe impl Send for Rc<SomethingLocal>` or
+                        // `impl !Send for Box<SomethingLocalAndSend>`.
+                        Some(self_def_id) => {
+                            if self_def_id.krate == ast::LOCAL_CRATE {
+                                None
+                            } else {
+                                Some(format!(
+                                    "cross-crate traits with a default impl, like `{}`, \
+                                     can only be implemented for a struct/enum type \
+                                     defined in the current crate",
+                                    ty::item_path_str(self.tcx, trait_def_id)))
+                            }
+                        }
+                        _ => {
+                            Some(format!(
+                                "cross-crate traits with a default impl, like `{}`, \
+                                 can only be implemented for a struct/enum type, \
+                                 not `{}`",
+                                ty::item_path_str(self.tcx, trait_def_id),
+                                self_ty.user_string(self.tcx)))
+                        }
+                    };
+
+                    if let Some(msg) = msg {
+                        span_err!(self.tcx.sess, item.span, E0321, "{}", msg);
+                        return;
+                    }
+                }
+
+                // Disallow *all* explicit impls of `Sized` for now.
+                if Some(trait_def_id) == self.tcx.lang_items.sized_trait() {
+                    span_err!(self.tcx.sess, item.span, E0322,
+                              "explicit impls for the `Sized` trait are not permitted");
+                    return;
+                }
             }
             ast::ItemDefaultImpl(..) => {
                 // "Trait" impl
@@ -100,14 +197,20 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
                 if trait_ref.def_id.krate != ast::LOCAL_CRATE {
                     span_err!(self.tcx.sess, item.span, E0318,
                               "cannot create default implementations for traits outside the \
-                               crate they're defined in; define a new trait instead.");
+                               crate they're defined in; define a new trait instead");
+                    return;
                 }
             }
             _ => {
                 // Not an impl
             }
         }
+    }
+}
 
+impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> {
+    fn visit_item(&mut self, item: &ast::Item) {
+        self.check_item(item);
         visit::walk_item(self, item);
     }
 }