From dbec033e29417ab01bae5749b58f8866b300f005 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 4 Mar 2015 15:19:27 -0500 Subject: [PATCH 1/4] Change the data structures for tracking defaulted traits. In the tcx, we now have a simple set of trait def-ids. During coherence, we use a separate table to track the default impls for any given trait so that we can report a nice error. This fixes various bugs in the metadata encoding that led to `ty::trait_has_default_impl` yielding the wrong values in the cross-crate case. (In particular, default impl def-ids were not included in the list of all impl def-ids; I debated fixing just that, but this approach seemed cleaner overall, since we usually treat the "defaulted" bit on traits as being a property of the trait, and now iterating over a list of impls doesn't intermingle default impls with normal impls.) --- src/librustc/metadata/common.rs | 2 + src/librustc/metadata/csearch.rs | 6 +-- src/librustc/metadata/decoder.rs | 11 +++-- src/librustc/metadata/encoder.rs | 6 +++ src/librustc/middle/ty.rs | 54 ++++++++---------------- src/librustc_typeck/coherence/overlap.rs | 33 +++++++-------- src/librustc_typeck/collect.rs | 2 +- 7 files changed, 51 insertions(+), 63 deletions(-) diff --git a/src/librustc/metadata/common.rs b/src/librustc/metadata/common.rs index b28106a72e048..081c64ecae881 100644 --- a/src/librustc/metadata/common.rs +++ b/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; diff --git a/src/librustc/metadata/csearch.rs b/src/librustc/metadata/csearch.rs index c7785ff4c8772..ed5783c8dba66 100644 --- a/src/librustc/metadata/csearch.rs +++ b/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) } diff --git a/src/librustc/metadata/decoder.rs b/src/librustc/metadata/decoder.rs index f5f9bd41c8b8e..dbbc17c018a2f 100644 --- a/src/librustc/metadata/decoder.rs +++ b/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 { diff --git a/src/librustc/metadata/encoder.rs b/src/librustc/metadata/encoder.rs index c4ba2373b9f57..08263eb8e6a03 100644 --- a/src/librustc/metadata/encoder.rs +++ b/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 { diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index add829074c4f0..60a04417e028f 100644 --- a/src/librustc/middle/ty.rs +++ b/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>>>>, - /// Maps a trait onto a list of *default* trait implementations - default_trait_impls: RefCell>, + /// A set of traits that have a default impl + traits_with_default_impls: RefCell>, /// 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 { || Rc::new(csearch::get_item_variances(&tcx.sess.cstore, item_id))) } -pub fn trait_default_impl(tcx: &ctxt, trait_def_id: DefId) -> Option { - 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(_) => {} diff --git a/src/librustc_typeck/coherence/overlap.rs b/src/librustc_typeck/coherence/overlap.rs index a6ecafb624131..466d00b348b94 100644 --- a/src/librustc_typeck/coherence/overlap.rs +++ b/src/librustc_typeck/coherence/overlap.rs @@ -20,10 +20,11 @@ use syntax::ast; use syntax::ast_util; use syntax::visit; use syntax::codemap::Span; +use util::nodemap::DefIdMap; use util::ppaux::Repr; pub fn check(tcx: &ty::ctxt) { - let mut overlap = OverlapChecker { tcx: tcx }; + let mut overlap = OverlapChecker { tcx: tcx, default_impls: DefIdMap() }; overlap.check_for_overlapping_impls(); // this secondary walk specifically checks for impls of defaulted @@ -33,6 +34,9 @@ pub fn check(tcx: &ty::ctxt) { struct OverlapChecker<'cx, 'tcx:'cx> { tcx: &'cx ty::ctxt<'tcx>, + + // maps from a trait def-id to an impl id + default_impls: DefIdMap, } impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> { @@ -134,24 +138,19 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OverlapChecker<'cx, 'tcx> { fn visit_item(&mut self, item: &'v ast::Item) { match item.node { ast::ItemDefaultImpl(_, _) => { + // look for another default impl; note that due to the + // general orphan/coherence rules, it must always be + // in this crate. let impl_def_id = ast_util::local_def(item.id); - match ty::impl_trait_ref(self.tcx, impl_def_id) { - Some(ref trait_ref) => { - match ty::trait_default_impl(self.tcx, trait_ref.def_id) { - Some(other_impl) if other_impl != impl_def_id => { - self.report_overlap_error(trait_ref.def_id, - other_impl, - impl_def_id); - } - Some(_) => {} - None => { - self.tcx.sess.bug( - &format!("no default implementation recorded for `{:?}`", - item)); - } - } + let trait_ref = ty::impl_trait_ref(self.tcx, impl_def_id).unwrap(); + let prev_default_impl = self.default_impls.insert(trait_ref.def_id, item.id); + match prev_default_impl { + Some(prev_id) => { + self.report_overlap_error(trait_ref.def_id, + impl_def_id, + ast_util::local_def(prev_id)); } - _ => {} + None => { } } } _ => {} diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 77e3b6ee64bb8..5591281263d05 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -979,7 +979,7 @@ fn convert_item(ccx: &CrateCtxt, it: &ast::Item) { None, None); - ty::record_default_trait_implementation(tcx, trait_ref.def_id, local_def(it.id)) + ty::record_trait_has_default_impl(tcx, trait_ref.def_id); } ast::ItemImpl(_, _, ref generics, From 2e216896e4778738b4ed01e427f711f3a0b2c44c Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 4 Mar 2015 15:23:17 -0500 Subject: [PATCH 2/4] Add stricter orphan rules for cross-crate impls of default traits. --- src/librustc_typeck/coherence/orphan.rs | 37 ++++++++++++++++++- ...lt-trait-impl-cross-crate-coherence-lib.rs | 17 +++++++++ ...efault-trait-impl-cross-crate-coherence.rs | 34 +++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs create mode 100644 src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs diff --git a/src/librustc_typeck/coherence/orphan.rs b/src/librustc_typeck/coherence/orphan.rs index 95dafccd866bf..fc49a555ad325 100644 --- a/src/librustc_typeck/coherence/orphan.rs +++ b/src/librustc_typeck/coherence/orphan.rs @@ -69,7 +69,8 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> { 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) => { @@ -92,6 +93,40 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> { } } } + + // Impls of a defaulted trait face additional rules. + 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(); + match self_ty.sty { + ty::ty_struct(self_def_id, _) | ty::ty_enum(self_def_id, _) => { + // The orphan check often rules this out, + // but not always. For example, the orphan + // check would accept `impl Send for + // Box`, but we want to + // forbid that. + if self_def_id.krate != ast::LOCAL_CRATE { + self.tcx.sess.span_err( + item.span, + "cross-crate traits with a default impl \ + can only be implemented for a struct/enum type \ + defined in the current crate"); + } + } + _ => { + self.tcx.sess.span_err( + item.span, + "cross-crate traits with a default impl \ + can only be implemented for a struct or enum type"); + } + } + } } ast::ItemDefaultImpl(..) => { // "Trait" impl diff --git a/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs b/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs new file mode 100644 index 0000000000000..579b9b0192555 --- /dev/null +++ b/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs @@ -0,0 +1,17 @@ +// 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. + +#![feature(optin_builtin_traits)] +#![crate_type = "rlib"] + +use std::marker::MarkerTrait; + +pub trait DefaultedTrait : MarkerTrait { } +impl DefaultedTrait for .. { } diff --git a/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs b/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs new file mode 100644 index 0000000000000..bcc01d1a0a72b --- /dev/null +++ b/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs @@ -0,0 +1,34 @@ +// 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. + +// aux-build:typeck-default-trait-impl-cross-crate-coherence-lib.rs + +// Test that we do not consider associated types to be sendable without +// some applicable trait bound (and we don't ICE). + +#![feature(optin_builtin_traits)] + +extern crate "typeck-default-trait-impl-cross-crate-coherence-lib" as lib; + +use lib::DefaultedTrait; + +struct A; +impl DefaultedTrait for (A,) { } +//~^ ERROR can only be implemented for a struct or enum type + +struct B; +impl !DefaultedTrait for (B,) { } +//~^ ERROR can only be implemented for a struct or enum type + +struct C; +impl DefaultedTrait for Box { } +//~^ ERROR can only be implemented for a struct or enum type + +fn main() { } From 4e789e03be332c73836aae018eb83a47bc82d3c9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 6 Mar 2015 15:55:30 -0500 Subject: [PATCH 3/4] Remove the coherence impls pass that was specialized to builtin bounds, since there are separate checks that apply to Copy (and Send uses the generic defaulted trait rules). Also prohibit `Sized` from being manually implemented for now. --- src/librustc_typeck/coherence/impls.rs | 47 -------- src/librustc_typeck/coherence/mod.rs | 2 - src/librustc_typeck/coherence/orphan.rs | 110 ++++++++++++++---- src/librustc_typeck/diagnostics.rs | 4 +- ...lt-trait-impl-cross-crate-coherence-lib.rs | 2 + src/test/compile-fail/coherence-impls-copy.rs | 39 +++++++ ...pls-builtin.rs => coherence-impls-send.rs} | 15 +-- .../compile-fail/coherence-impls-sized.rs | 33 ++++++ src/test/compile-fail/coherence-orphan.rs | 8 +- ...efault-trait-impl-cross-crate-coherence.rs | 12 +- ...typeck-default-trait-impl-outside-crate.rs | 2 +- 11 files changed, 184 insertions(+), 90 deletions(-) delete mode 100644 src/librustc_typeck/coherence/impls.rs create mode 100644 src/test/compile-fail/coherence-impls-copy.rs rename src/test/compile-fail/{coherence-impls-builtin.rs => coherence-impls-send.rs} (65%) create mode 100644 src/test/compile-fail/coherence-impls-sized.rs diff --git a/src/librustc_typeck/coherence/impls.rs b/src/librustc_typeck/coherence/impls.rs deleted file mode 100644 index e89c96b36e1a2..0000000000000 --- a/src/librustc_typeck/coherence/impls.rs +++ /dev/null @@ -1,47 +0,0 @@ -// 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. - -//! Implementations checker: builtin traits and default impls are allowed just -//! for structs and enums. - -use middle::ty; -use syntax::ast::{Item, ItemImpl}; -use syntax::ast; -use syntax::visit; - -pub fn check(tcx: &ty::ctxt) { - let mut impls = ImplsChecker { tcx: tcx }; - visit::walk_crate(&mut impls, tcx.map.krate()); -} - -struct ImplsChecker<'cx, 'tcx:'cx> { - tcx: &'cx ty::ctxt<'tcx> -} - -impl<'cx, 'tcx,'v> visit::Visitor<'v> for ImplsChecker<'cx, 'tcx> { - fn visit_item(&mut self, item: &'v ast::Item) { - match item.node { - ast::ItemImpl(_, _, _, Some(_), _, _) => { - let trait_ref = ty::impl_id_to_trait_ref(self.tcx, item.id); - if let Some(_) = self.tcx.lang_items.to_builtin_kind(trait_ref.def_id) { - match trait_ref.self_ty().sty { - ty::ty_struct(..) | ty::ty_enum(..) => {} - _ => { - span_err!(self.tcx.sess, item.span, E0209, - "builtin traits can only be \ - implemented on structs or enums"); - } - } - } - } - _ => {} - } - } -} diff --git a/src/librustc_typeck/coherence/mod.rs b/src/librustc_typeck/coherence/mod.rs index 9a8545f3dd515..a06dcbaf556bd 100644 --- a/src/librustc_typeck/coherence/mod.rs +++ b/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); diff --git a/src/librustc_typeck/coherence/orphan.rs b/src/librustc_typeck/coherence/orphan.rs index fc49a555ad325..0857203b777ac 100644 --- a/src/librustc_typeck/coherence/orphan.rs +++ b/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,6 +66,7 @@ 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; } } } @@ -81,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)) => { @@ -90,11 +95,44 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> { some local type (e.g. `MyStruct`); only traits defined in \ the current crate can be implemented for a type parameter", param_ty.user_string(self.tcx)); + return; } } } - // Impls of a defaulted trait face additional rules. + // 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(..) { + // one_foo::<(A,B)>(..) + // } + // fn one_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), @@ -104,29 +142,53 @@ impl<'cx, 'tcx,'v> visit::Visitor<'v> for OrphanChecker<'cx, 'tcx> { trait_def_id.krate != ast::LOCAL_CRATE { let self_ty = trait_ref.self_ty(); - match self_ty.sty { - ty::ty_struct(self_def_id, _) | ty::ty_enum(self_def_id, _) => { - // The orphan check often rules this out, - // but not always. For example, the orphan - // check would accept `impl Send for - // Box`, but we want to - // forbid that. - if self_def_id.krate != ast::LOCAL_CRATE { - self.tcx.sess.span_err( - item.span, - "cross-crate traits with a default impl \ + 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` or + // `unsafe impl !Send for Box`. + 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"); + defined in the current crate", + ty::item_path_str(self.tcx, trait_def_id))) } } _ => { - self.tcx.sess.span_err( - item.span, - "cross-crate traits with a default impl \ - can only be implemented for a struct or enum type"); + 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 @@ -135,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); } } diff --git a/src/librustc_typeck/diagnostics.rs b/src/librustc_typeck/diagnostics.rs index 3bd15fbc7dbea..03fa269ccf829 100644 --- a/src/librustc_typeck/diagnostics.rs +++ b/src/librustc_typeck/diagnostics.rs @@ -175,7 +175,9 @@ register_diagnostics! { E0250, // expected constant expr for array length E0318, // can't create default impls for traits outside their crates E0319, // trait impls for defaulted traits allowed just for structs/enums - E0320 // recursive overflow during dropck + E0320, // recursive overflow during dropck + E0321, // extended coherence rules for defaulted traits violated + E0322 // cannot implement Sized explicitly } __build_diagnostic_array! { DIAGNOSTICS } diff --git a/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs b/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs index 579b9b0192555..506e7a00c75bc 100644 --- a/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs +++ b/src/test/auxiliary/typeck-default-trait-impl-cross-crate-coherence-lib.rs @@ -15,3 +15,5 @@ use std::marker::MarkerTrait; pub trait DefaultedTrait : MarkerTrait { } impl DefaultedTrait for .. { } + +pub struct Something { t: T } diff --git a/src/test/compile-fail/coherence-impls-copy.rs b/src/test/compile-fail/coherence-impls-copy.rs new file mode 100644 index 0000000000000..3034be177ca68 --- /dev/null +++ b/src/test/compile-fail/coherence-impls-copy.rs @@ -0,0 +1,39 @@ +// 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. + +#![feature(optin_builtin_traits)] + +use std::marker::Copy; + +enum TestE { + A +} + +struct MyType; + +struct NotSync; +impl !Sync for NotSync {} + +impl Copy for TestE {} +impl Copy for MyType {} +impl Copy for (MyType, MyType) {} +//~^ ERROR E0206 + +impl Copy for &'static NotSync {} +//~^ ERROR E0206 + +impl Copy for [MyType] {} +//~^ ERROR E0206 + +impl Copy for &'static [NotSync] {} +//~^ ERROR E0206 + +fn main() { +} diff --git a/src/test/compile-fail/coherence-impls-builtin.rs b/src/test/compile-fail/coherence-impls-send.rs similarity index 65% rename from src/test/compile-fail/coherence-impls-builtin.rs rename to src/test/compile-fail/coherence-impls-send.rs index 3e132dcb11fa3..b05c1ff0f0b72 100644 --- a/src/test/compile-fail/coherence-impls-builtin.rs +++ b/src/test/compile-fail/coherence-impls-send.rs @@ -10,7 +10,7 @@ #![feature(optin_builtin_traits)] -use std::marker::Send; +use std::marker::Copy; enum TestE { A @@ -24,20 +24,17 @@ impl !Sync for NotSync {} unsafe impl Send for TestE {} unsafe impl Send for MyType {} unsafe impl Send for (MyType, MyType) {} -//~^ ERROR builtin traits can only be implemented on structs or enums +//~^ ERROR E0321 unsafe impl Send for &'static NotSync {} -//~^ ERROR builtin traits can only be implemented on structs or enums +//~^ ERROR E0321 unsafe impl Send for [MyType] {} -//~^ ERROR builtin traits can only be implemented on structs or enums +//~^ ERROR E0321 unsafe impl Send for &'static [NotSync] {} -//~^ ERROR builtin traits can only be implemented on structs or enums -//~^^ ERROR conflicting implementations for trait `core::marker::Send` - -fn is_send() {} +//~^ ERROR E0321 +//~| ERROR conflicting implementations fn main() { - is_send::<(MyType, TestE)>(); } diff --git a/src/test/compile-fail/coherence-impls-sized.rs b/src/test/compile-fail/coherence-impls-sized.rs new file mode 100644 index 0000000000000..a9a3ebaffb75a --- /dev/null +++ b/src/test/compile-fail/coherence-impls-sized.rs @@ -0,0 +1,33 @@ +// 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. + +#![feature(optin_builtin_traits)] + +use std::marker::Copy; + +enum TestE { + A +} + +struct MyType; + +struct NotSync; +impl !Sync for NotSync {} + +impl Sized for TestE {} //~ ERROR E0322 +impl Sized for MyType {} //~ ERROR E0322 +impl Sized for (MyType, MyType) {} //~ ERROR E0322 +impl Sized for &'static NotSync {} //~ ERROR E0322 +impl Sized for [MyType] {} //~ ERROR E0322 +//~^ ERROR E0277 +impl Sized for &'static [NotSync] {} //~ ERROR E0322 + +fn main() { +} diff --git a/src/test/compile-fail/coherence-orphan.rs b/src/test/compile-fail/coherence-orphan.rs index d7cd68e73c349..97dffec2dd9bd 100644 --- a/src/test/compile-fail/coherence-orphan.rs +++ b/src/test/compile-fail/coherence-orphan.rs @@ -19,13 +19,15 @@ use lib::TheTrait; struct TheType; -impl TheTrait for isize { } //~ ERROR E0117 +impl TheTrait for isize { } +//~^ ERROR E0117 impl TheTrait for isize { } impl TheTrait for TheType { } -impl !Send for Vec { } //~ ERROR E0117 -//~^ ERROR conflicting +impl !Send for Vec { } +//~^ ERROR E0117 +//~| ERROR E0119 fn main() { } diff --git a/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs b/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs index bcc01d1a0a72b..3a29bb9c2277c 100644 --- a/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs +++ b/src/test/compile-fail/typeck-default-trait-impl-cross-crate-coherence.rs @@ -20,15 +20,15 @@ extern crate "typeck-default-trait-impl-cross-crate-coherence-lib" as lib; use lib::DefaultedTrait; struct A; -impl DefaultedTrait for (A,) { } -//~^ ERROR can only be implemented for a struct or enum type +impl DefaultedTrait for (A,) { } //~ ERROR E0321 struct B; -impl !DefaultedTrait for (B,) { } -//~^ ERROR can only be implemented for a struct or enum type +impl !DefaultedTrait for (B,) { } //~ ERROR E0321 struct C; -impl DefaultedTrait for Box { } -//~^ ERROR can only be implemented for a struct or enum type +struct D(T); +impl DefaultedTrait for Box { } //~ ERROR E0321 +impl DefaultedTrait for lib::Something { } //~ ERROR E0321 +impl DefaultedTrait for D { } // OK fn main() { } diff --git a/src/test/compile-fail/typeck-default-trait-impl-outside-crate.rs b/src/test/compile-fail/typeck-default-trait-impl-outside-crate.rs index 10ba8c7416483..a345bd1b65c0e 100644 --- a/src/test/compile-fail/typeck-default-trait-impl-outside-crate.rs +++ b/src/test/compile-fail/typeck-default-trait-impl-outside-crate.rs @@ -13,6 +13,6 @@ #![feature(optin_builtin_traits)] impl Copy for .. {} -//~^ ERROR cannot create default implementations for traits outside the crate they're defined in; define a new trait instead. +//~^ ERROR E0318 fn main() {} From 17358d1d2152710feb911fe1c02fcaeba3de4b17 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 9 Mar 2015 14:39:31 -0400 Subject: [PATCH 4/4] Address nit by @flaper87 --- src/librustc_typeck/coherence/orphan.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/coherence/orphan.rs b/src/librustc_typeck/coherence/orphan.rs index 0857203b777ac..5dfe80cfcb213 100644 --- a/src/librustc_typeck/coherence/orphan.rs +++ b/src/librustc_typeck/coherence/orphan.rs @@ -155,7 +155,7 @@ impl<'cx, 'tcx> OrphanChecker<'cx, 'tcx> { // 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` or - // `unsafe impl !Send for Box`. + // `impl !Send for Box`. Some(self_def_id) => { if self_def_id.krate == ast::LOCAL_CRATE { None