From 21df87f515b10ab4d6dac2275d09e679471c7d7c Mon Sep 17 00:00:00 2001 From: Aaron Turon Date: Fri, 26 Feb 2016 13:21:44 -0800 Subject: [PATCH] Forbid items with the same name being defined in overlapping inherent impl blocks. For example, the following is now correctly illegal: ```rust struct Foo; impl Foo { fn id() {} } impl Foo { fn id() {} } ``` "Overlapping" here is determined the same way it is for traits (and in fact shares the same code path): roughly, there must be some way of substituting any generic types to unify the impls, such that none of the `where` clauses are provably unsatisfiable under such a unification. Closes #22889 --- src/librustc/dep_graph/mod.rs | 1 + src/librustc/middle/infer/mod.rs | 8 +-- src/librustc/middle/traits/coherence.rs | 21 +++---- src/librustc/middle/ty/mod.rs | 16 ++--- src/librustc/middle/ty/structural_impls.rs | 3 +- src/librustc_typeck/coherence/mod.rs | 9 +++ src/librustc_typeck/coherence/overlap.rs | 72 +++++++++++++++++++--- src/librustc_typeck/collect.rs | 13 +--- src/librustc_typeck/diagnostics.rs | 15 +++++ 9 files changed, 113 insertions(+), 45 deletions(-) diff --git a/src/librustc/dep_graph/mod.rs b/src/librustc/dep_graph/mod.rs index 2fad161652f69..f4df6994e047b 100644 --- a/src/librustc/dep_graph/mod.rs +++ b/src/librustc/dep_graph/mod.rs @@ -58,6 +58,7 @@ pub enum DepNode { CoherenceCheckImpl(DefId), CoherenceOverlapCheck(DefId), CoherenceOverlapCheckSpecial(DefId), + CoherenceOverlapInherentCheck(DefId), CoherenceOrphanCheck(DefId), Variance, WfCheck(DefId), diff --git a/src/librustc/middle/infer/mod.rs b/src/librustc/middle/infer/mod.rs index 7cc7f599e14f9..3eca4624bc17d 100644 --- a/src/librustc/middle/infer/mod.rs +++ b/src/librustc/middle/infer/mod.rs @@ -482,14 +482,14 @@ pub fn mk_sub_poly_trait_refs<'a, 'tcx>(cx: &InferCtxt<'a, 'tcx>, pub fn mk_eq_impl_headers<'a, 'tcx>(cx: &InferCtxt<'a, 'tcx>, a_is_expected: bool, origin: TypeOrigin, - a: ty::ImplHeader<'tcx>, - b: ty::ImplHeader<'tcx>) + a: &ty::ImplHeader<'tcx>, + b: &ty::ImplHeader<'tcx>) -> UnitResult<'tcx> { debug!("mk_eq_impl_header({:?} = {:?})", a, b); match (a.trait_ref, b.trait_ref) { - (Some(a_ref), Some(b_ref)) => mk_eq_trait_refs(cx, a_is_expected, a_ref, b_ref), - (None, None) => mk_eqty(cx, a_is_expected, a.self_ty, b.self_ty), + (Some(a_ref), Some(b_ref)) => mk_eq_trait_refs(cx, a_is_expected, origin, a_ref, b_ref), + (None, None) => mk_eqty(cx, a_is_expected, origin, a.self_ty, b.self_ty), _ => cx.tcx.sess.bug("mk_eq_impl_headers given mismatched impl kinds"), } } diff --git a/src/librustc/middle/traits/coherence.rs b/src/librustc/middle/traits/coherence.rs index b59fb5c7e257c..6005d36ff4eb2 100644 --- a/src/librustc/middle/traits/coherence.rs +++ b/src/librustc/middle/traits/coherence.rs @@ -10,18 +10,15 @@ //! See `README.md` for high-level documentation -use super::{Normalized, SelectionContext}; -use super::{Obligation, ObligationCause, PredicateObligation}; -use super::project; -use super::util; +use super::{SelectionContext}; +use super::{Obligation, ObligationCause}; use middle::cstore::LOCAL_CRATE; use middle::def_id::DefId; -use middle::subst::{Subst, Substs, TypeSpace}; +use middle::subst::TypeSpace; use middle::ty::{self, Ty, TyCtxt}; -use middle::ty::error::TypeError; use middle::infer::{self, InferCtxt, TypeOrigin}; -use syntax::codemap::{DUMMY_SP, Span}; +use syntax::codemap::DUMMY_SP; #[derive(Copy, Clone)] struct InferIsLocal(bool); @@ -31,7 +28,7 @@ struct InferIsLocal(bool); pub fn overlapping_impls<'cx, 'tcx>(infcx: &InferCtxt<'cx, 'tcx>, impl1_def_id: DefId, impl2_def_id: DefId) - -> Option> + -> Option> { debug!("impl_can_satisfy(\ impl1_def_id={:?}, \ @@ -48,7 +45,7 @@ pub fn overlapping_impls<'cx, 'tcx>(infcx: &InferCtxt<'cx, 'tcx>, fn overlap<'cx, 'tcx>(selcx: &mut SelectionContext<'cx, 'tcx>, a_def_id: DefId, b_def_id: DefId) - -> Option> + -> Option> { debug!("overlap(a_def_id={:?}, b_def_id={:?})", a_def_id, @@ -64,8 +61,8 @@ fn overlap<'cx, 'tcx>(selcx: &mut SelectionContext<'cx, 'tcx>, if let Err(_) = infer::mk_eq_impl_headers(selcx.infcx(), true, TypeOrigin::Misc(DUMMY_SP), - a_impl_header, - b_impl_header) { + &a_impl_header, + &b_impl_header) { return None; } @@ -74,7 +71,7 @@ fn overlap<'cx, 'tcx>(selcx: &mut SelectionContext<'cx, 'tcx>, // Are any of the obligations unsatisfiable? If so, no overlap. let infcx = selcx.infcx(); let opt_failing_obligation = - a_impl_header.prediates + a_impl_header.predicates .iter() .chain(&b_impl_header.predicates) .map(|p| infcx.resolve_type_vars_if_possible(p)) diff --git a/src/librustc/middle/ty/mod.rs b/src/librustc/middle/ty/mod.rs index 2405f661ab3f1..b54b0b73ef292 100644 --- a/src/librustc/middle/ty/mod.rs +++ b/src/librustc/middle/ty/mod.rs @@ -164,9 +164,9 @@ pub struct ImplHeader<'tcx> { } impl<'tcx> ImplHeader<'tcx> { - pub fn with_fresh_ty_vars<'a,'tcx>(selcx: &mut traits::SelectionContext<'a,'tcx>, - impl_def_id: DefId) - -> ImplHeader<'tcx> + pub fn with_fresh_ty_vars<'a>(selcx: &mut traits::SelectionContext<'a, 'tcx>, + impl_def_id: DefId) + -> ImplHeader<'tcx> { let tcx = selcx.tcx(); let impl_generics = tcx.lookup_item_type(impl_def_id).generics; @@ -174,13 +174,13 @@ impl<'tcx> ImplHeader<'tcx> { let header = ImplHeader { impl_def_id: impl_def_id, - self_ty: tcx.lookup_item_type(impl_def_id), + self_ty: tcx.lookup_item_type(impl_def_id).ty, trait_ref: tcx.impl_trait_ref(impl_def_id), - predicates: tcx.lookup_predicates(impl_def_id), - }.subst(tcx, impl_substs); + predicates: tcx.lookup_predicates(impl_def_id).predicates.into_vec(), + }.subst(tcx, &impl_substs); - let Normalized { value: mut header, obligations: obligations } = - proect::normalize(selcx, ObligationCause::dummy(), &header); + let traits::Normalized { value: mut header, obligations } = + traits::normalize(selcx, traits::ObligationCause::dummy(), &header); header.predicates.extend(obligations.into_iter().map(|o| o.predicate)); header diff --git a/src/librustc/middle/ty/structural_impls.rs b/src/librustc/middle/ty/structural_impls.rs index 645e709f51235..57cfdd0d8b8ae 100644 --- a/src/librustc/middle/ty/structural_impls.rs +++ b/src/librustc/middle/ty/structural_impls.rs @@ -452,8 +452,7 @@ impl<'tcx> TypeFoldable<'tcx> for ty::ImplHeader<'tcx> { impl_def_id: self.impl_def_id, self_ty: self.self_ty.fold_with(folder), trait_ref: self.trait_ref.map(|t| t.fold_with(folder)), - predicates: self.predicates.into_iter().map(|p| p.fold_with(folder)).collect(), - polarity: self.polarity, + predicates: self.predicates.iter().map(|p| p.fold_with(folder)).collect(), } } diff --git a/src/librustc_typeck/coherence/mod.rs b/src/librustc_typeck/coherence/mod.rs index cad321c0b2374..9dc8d7ae943a1 100644 --- a/src/librustc_typeck/coherence/mod.rs +++ b/src/librustc_typeck/coherence/mod.rs @@ -35,7 +35,9 @@ use CrateCtxt; use middle::infer::{self, InferCtxt, TypeOrigin, new_infer_ctxt}; use std::cell::RefCell; use std::rc::Rc; +use syntax::ast; use syntax::codemap::Span; +use syntax::errors::DiagnosticBuilder; use util::nodemap::{DefIdMap, FnvHashMap}; use rustc::dep_graph::DepNode; use rustc::front::map as hir_map; @@ -519,6 +521,13 @@ fn enforce_trait_manually_implementable(tcx: &TyCtxt, sp: Span, trait_def_id: De err.emit(); } +// Factored out into helper because the error cannot be defined in multiple locations. +pub fn report_duplicate_item<'tcx>(tcx: &TyCtxt<'tcx>, sp: Span, name: ast::Name) + -> DiagnosticBuilder<'tcx> +{ + struct_span_err!(tcx.sess, sp, E0201, "duplicate definitions with name `{}`:", name) +} + pub fn check_coherence(crate_context: &CrateCtxt) { let _task = crate_context.tcx.dep_graph.in_task(DepNode::Coherence); let infcx = new_infer_ctxt(crate_context.tcx, &crate_context.tcx.tables, None); diff --git a/src/librustc_typeck/coherence/overlap.rs b/src/librustc_typeck/coherence/overlap.rs index 9ec42b6d3f859..80430076f197d 100644 --- a/src/librustc_typeck/coherence/overlap.rs +++ b/src/librustc_typeck/coherence/overlap.rs @@ -9,7 +9,8 @@ // except according to those terms. //! Overlap: No two impls for the same trait are implemented for the -//! same type. +//! same type. Likewise, no two inherent impls for a given type +//! constructor provide a method with the same name. use middle::cstore::{CrateStore, LOCAL_CRATE}; use middle::def_id::DefId; @@ -115,7 +116,6 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> { } } - fn check_if_impls_overlap(&self, impl1_def_id: DefId, impl2_def_id: DefId) @@ -128,8 +128,8 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> { impl2_def_id); let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, None); - if let Some(trait_ref) = traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) { - self.report_overlap_error(impl1_def_id, impl2_def_id, trait_ref); + if let Some(header) = traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) { + self.report_overlap_error(impl1_def_id, impl2_def_id, header.trait_ref.unwrap()); } } } @@ -150,13 +150,13 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> { }).unwrap_or(String::new()) }; - let mut err = struct_span_err!(self.tcx.sess, self.span_of_impl(impl1), E0119, + let mut err = struct_span_err!(self.tcx.sess, self.span_of_def_id(impl1), E0119, "conflicting implementations of trait `{}`{}:", trait_ref, self_type); if impl2.is_local() { - span_note!(&mut err, self.span_of_impl(impl2), + span_note!(&mut err, self.span_of_def_id(impl2), "conflicting implementation is here:"); } else { let cname = self.tcx.sess.cstore.crate_name(impl2.krate); @@ -165,10 +165,61 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> { err.emit(); } - fn span_of_impl(&self, impl_did: DefId) -> Span { - let node_id = self.tcx.map.as_local_node_id(impl_did).unwrap(); + fn span_of_def_id(&self, did: DefId) -> Span { + let node_id = self.tcx.map.as_local_node_id(did).unwrap(); self.tcx.map.span(node_id) } + + fn check_for_common_items_in_impls(&self, impl1: DefId, impl2: DefId) { + #[derive(Copy, Clone, PartialEq)] + enum Namespace { Type, Value } + + fn name_and_namespace(tcx: &TyCtxt, item: &ty::ImplOrTraitItemId) + -> (ast::Name, Namespace) + { + let name = tcx.impl_or_trait_item(item.def_id()).name(); + (name, match *item { + ty::TypeTraitItemId(..) => Namespace::Type, + ty::ConstTraitItemId(..) => Namespace::Value, + ty::MethodTraitItemId(..) => Namespace::Value, + }) + } + + let impl_items = self.tcx.impl_items.borrow(); + + for item1 in &impl_items[&impl1] { + let (name, namespace) = name_and_namespace(&self.tcx, item1); + + for item2 in &impl_items[&impl2] { + if (name, namespace) == name_and_namespace(&self.tcx, item2) { + let mut err = super::report_duplicate_item( + &self.tcx, self.span_of_def_id(item1.def_id()), name); + span_note!(&mut err, self.span_of_def_id(item2.def_id()), + "conflicting definition is here:"); + err.emit(); + } + } + } + } + + fn check_for_overlapping_inherent_impls(&self, ty_def_id: DefId) { + let _task = self.tcx.dep_graph.in_task(DepNode::CoherenceOverlapInherentCheck(ty_def_id)); + + let inherent_impls = self.tcx.inherent_impls.borrow(); + let impls = match inherent_impls.get(&ty_def_id) { + Some(impls) => impls, + None => return + }; + + for (i, &impl1_def_id) in impls.iter().enumerate() { + for &impl2_def_id in &impls[(i+1)..] { + let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, None); + if traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id).is_some() { + self.check_for_common_items_in_impls(impl1_def_id, impl2_def_id) + } + } + } + } } @@ -180,6 +231,11 @@ impl<'cx, 'tcx,'v> intravisit::Visitor<'v> for OverlapChecker<'cx, 'tcx> { self.check_for_overlapping_impls_of_trait(trait_def_id); } + hir::ItemEnum(..) | hir::ItemStruct(..) => { + let type_def_id = self.tcx.map.local_def_id(item.id); + self.check_for_overlapping_inherent_impls(type_def_id); + } + hir::ItemDefaultImpl(..) => { // look for another default impl; note that due to the // general orphan/coherence rules, it must always be diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 3ce03e245785a..b493b64a45fcf 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -63,6 +63,7 @@ use lint; use middle::def::Def; use middle::def_id::DefId; use constrained_type_params as ctp; +use coherence; use middle::lang_items::SizedTraitLangItem; use middle::resolve_lifetime; use middle::const_eval::{self, ConstVal}; @@ -750,17 +751,7 @@ fn convert_item(ccx: &CrateCtxt, it: &hir::Item) { _ => &mut seen_value_items, }; if !seen_items.insert(impl_item.name) { - let desc = match impl_item.node { - hir::ImplItemKind::Const(_, _) => "associated constant", - hir::ImplItemKind::Type(_) => "associated type", - hir::ImplItemKind::Method(ref sig, _) => - match sig.explicit_self.node { - hir::SelfStatic => "associated function", - _ => "method", - }, - }; - - span_err!(tcx.sess, impl_item.span, E0201, "duplicate {}", desc); + coherence::report_duplicate_item(tcx, impl_item.span, impl_item.name).emit(); } if let hir::ImplItemKind::Const(ref ty, _) = impl_item.node { diff --git a/src/librustc_typeck/diagnostics.rs b/src/librustc_typeck/diagnostics.rs index cfe76206b0290..14d04a5d109bb 100644 --- a/src/librustc_typeck/diagnostics.rs +++ b/src/librustc_typeck/diagnostics.rs @@ -2285,6 +2285,21 @@ impl Baz for Foo { type Quux = u32; } ``` + +Note, however, that items with the same name are allowed for inherent `impl` +blocks that don't overlap: + +``` +struct Foo(T); + +impl Foo { + fn bar(&self) -> bool { self.0 > 5 } +} + +impl Foo { + fn bar(&self) -> bool { self.0 } +} +``` "##, E0202: r##"