From c7168d306f3e95ce522ed8589ccb322a76e7a8d2 Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Sun, 23 Jul 2017 22:30:47 +0900 Subject: [PATCH 1/9] Add hints when intercrate ambiguity causes overlap. --- src/librustc/traits/coherence.rs | 15 +++++-- src/librustc/traits/mod.rs | 5 +-- src/librustc/traits/select.rs | 29 ++++++++++++ src/librustc/traits/specialize/mod.rs | 16 +++++++ .../traits/specialize/specialization_graph.rs | 7 +-- .../coherence/inherent_impls_overlap.rs | 45 +++++++++++++------ 6 files changed, 95 insertions(+), 22 deletions(-) diff --git a/src/librustc/traits/coherence.rs b/src/librustc/traits/coherence.rs index 34df447a11e15..dee81b8c4fa28 100644 --- a/src/librustc/traits/coherence.rs +++ b/src/librustc/traits/coherence.rs @@ -13,6 +13,7 @@ use hir::def_id::{DefId, LOCAL_CRATE}; use syntax_pos::DUMMY_SP; use traits::{self, Normalized, SelectionContext, Obligation, ObligationCause, Reveal}; +use traits::select::IntercrateAmbiguityCause; use ty::{self, Ty, TyCtxt}; use ty::subst::Subst; @@ -21,12 +22,17 @@ use infer::{InferCtxt, InferOk}; #[derive(Copy, Clone)] struct InferIsLocal(bool); +pub struct OverlapResult<'tcx> { + pub impl_header: ty::ImplHeader<'tcx>, + pub intercrate_ambiguity_causes: Vec, +} + /// If there are types that satisfy both impls, returns a suitably-freshened /// `ImplHeader` with those types substituted pub fn overlapping_impls<'cx, 'gcx, 'tcx>(infcx: &InferCtxt<'cx, 'gcx, 'tcx>, impl1_def_id: DefId, impl2_def_id: DefId) - -> Option> + -> Option> { debug!("impl_can_satisfy(\ impl1_def_id={:?}, \ @@ -65,7 +71,7 @@ fn with_fresh_ty_vars<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, ' fn overlap<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, 'tcx>, a_def_id: DefId, b_def_id: DefId) - -> Option> + -> Option> { debug!("overlap(a_def_id={:?}, b_def_id={:?})", a_def_id, @@ -113,7 +119,10 @@ fn overlap<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, 'tcx>, return None } - Some(selcx.infcx().resolve_type_vars_if_possible(&a_impl_header)) + Some(OverlapResult { + impl_header: selcx.infcx().resolve_type_vars_if_possible(&a_impl_header), + intercrate_ambiguity_causes: selcx.intercrate_ambiguity_causes().to_vec(), + }) } pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index e14203b34a180..1283c5178b86e 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -28,9 +28,7 @@ use std::rc::Rc; use syntax::ast; use syntax_pos::{Span, DUMMY_SP}; -pub use self::coherence::orphan_check; -pub use self::coherence::overlapping_impls; -pub use self::coherence::OrphanCheckErr; +pub use self::coherence::{orphan_check, overlapping_impls, OrphanCheckErr, OverlapResult}; pub use self::fulfill::{FulfillmentContext, RegionObligation}; pub use self::project::MismatchedProjectionTypes; pub use self::project::{normalize, normalize_projection_type, Normalized}; @@ -38,6 +36,7 @@ pub use self::project::{ProjectionCache, ProjectionCacheSnapshot, Reveal}; pub use self::object_safety::ObjectSafetyViolation; pub use self::object_safety::MethodViolationCode; pub use self::select::{EvaluationCache, SelectionContext, SelectionCache}; +pub use self::select::IntercrateAmbiguityCause; pub use self::specialize::{OverlapError, specialization_graph, specializes, translate_substs}; pub use self::specialize::{SpecializesCache, find_associated_item}; pub use self::util::elaborate_predicates; diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index c690bebed8c00..d08203ae6d44a 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -90,6 +90,14 @@ pub struct SelectionContext<'cx, 'gcx: 'cx+'tcx, 'tcx: 'cx> { intercrate: bool, inferred_obligations: SnapshotVec>, + + intercrate_ambiguity_causes: Vec, +} + +#[derive(Clone)] +pub enum IntercrateAmbiguityCause { + DownstreamCrate(DefId), + UpstreamCrateUpdate(DefId), } // A stack that walks back up the stack frame. @@ -380,6 +388,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { freshener: infcx.freshener(), intercrate: false, inferred_obligations: SnapshotVec::new(), + intercrate_ambiguity_causes: Vec::new(), } } @@ -389,6 +398,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { freshener: infcx.freshener(), intercrate: true, inferred_obligations: SnapshotVec::new(), + intercrate_ambiguity_causes: Vec::new(), } } @@ -404,6 +414,10 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { self.infcx } + pub fn intercrate_ambiguity_causes(&self) -> &[IntercrateAmbiguityCause] { + &self.intercrate_ambiguity_causes + } + /// Wraps the inference context's in_snapshot s.t. snapshot handling is only from the selection /// context's self. fn in_snapshot(&mut self, f: F) -> R @@ -751,6 +765,14 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { if unbound_input_types && self.intercrate { debug!("evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous", stack.fresh_trait_ref); + // Heuristics: show the diagnostics when there are no candidates in crate. + if let Ok(candidate_set) = self.assemble_candidates(stack) { + if !candidate_set.ambiguous && candidate_set.vec.is_empty() { + let did = stack.fresh_trait_ref.def_id(); + self.intercrate_ambiguity_causes.push( + IntercrateAmbiguityCause::DownstreamCrate(did)); + } + } return EvaluatedToAmbig; } if unbound_input_types && @@ -1005,6 +1027,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { if !self.is_knowable(stack) { debug!("coherence stage: not knowable"); + // Heuristics: show the diagnostics when there are no candidates in crate. + let candidate_set = self.assemble_candidates(stack)?; + if !candidate_set.ambiguous && candidate_set.vec.is_empty() { + let did = stack.obligation.predicate.def_id(); + self.intercrate_ambiguity_causes.push( + IntercrateAmbiguityCause::UpstreamCrateUpdate(did)); + } return Ok(None); } diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index 18734e2dbc3f1..5ae6fec000fd8 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -25,6 +25,7 @@ use hir::def_id::DefId; use infer::{InferCtxt, InferOk}; use ty::subst::{Subst, Substs}; use traits::{self, Reveal, ObligationCause}; +use traits::select::IntercrateAmbiguityCause; use ty::{self, TyCtxt, TypeFoldable}; use syntax_pos::DUMMY_SP; use std::rc::Rc; @@ -36,6 +37,7 @@ pub struct OverlapError { pub with_impl: DefId, pub trait_desc: String, pub self_desc: Option, + pub intercrate_ambiguity_causes: Vec, } /// Given a subst for the requested impl, translate it to a subst @@ -342,6 +344,20 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx } } + for cause in &overlap.intercrate_ambiguity_causes { + match cause { + &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { + err.note(&format!("downstream crates may implement {}", + tcx.item_path_str(def_id))); + } + &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { + err.note(&format!("upstream crates may add new impl for {} \ + in future versions", + tcx.item_path_str(def_id))); + } + } + } + err.emit(); } } else { diff --git a/src/librustc/traits/specialize/specialization_graph.rs b/src/librustc/traits/specialize/specialization_graph.rs index f80caeec460fa..e6ba13afbbb8a 100644 --- a/src/librustc/traits/specialize/specialization_graph.rs +++ b/src/librustc/traits/specialize/specialization_graph.rs @@ -113,7 +113,7 @@ impl<'a, 'gcx, 'tcx> Children { let overlap = traits::overlapping_impls(&infcx, possible_sibling, impl_def_id); - if let Some(impl_header) = overlap { + if let Some(overlap) = overlap { if tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) { return Ok((false, false)); } @@ -123,7 +123,7 @@ impl<'a, 'gcx, 'tcx> Children { if le == ge { // overlap, but no specialization; error out - let trait_ref = impl_header.trait_ref.unwrap(); + let trait_ref = overlap.impl_header.trait_ref.unwrap(); let self_ty = trait_ref.self_ty(); Err(OverlapError { with_impl: possible_sibling, @@ -135,7 +135,8 @@ impl<'a, 'gcx, 'tcx> Children { Some(self_ty.to_string()) } else { None - } + }, + intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes, }) } else { Ok((le, ge)) diff --git a/src/librustc_typeck/coherence/inherent_impls_overlap.rs b/src/librustc_typeck/coherence/inherent_impls_overlap.rs index 078ae34bc524c..07de54233041c 100644 --- a/src/librustc_typeck/coherence/inherent_impls_overlap.rs +++ b/src/librustc_typeck/coherence/inherent_impls_overlap.rs @@ -12,6 +12,7 @@ use rustc::hir::def_id::{CrateNum, DefId, LOCAL_CRATE}; use rustc::hir; use rustc::hir::itemlikevisit::ItemLikeVisitor; use rustc::traits; +use rustc::traits::IntercrateAmbiguityCause; use rustc::ty::{self, TyCtxt}; pub fn crate_inherent_impls_overlap_check<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, @@ -26,7 +27,8 @@ struct InherentOverlapChecker<'a, 'tcx: 'a> { } impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { - fn check_for_common_items_in_impls(&self, impl1: DefId, impl2: DefId) { + fn check_for_common_items_in_impls(&self, impl1: DefId, impl2: DefId, + overlap: traits::OverlapResult) { #[derive(Copy, Clone, PartialEq)] enum Namespace { Type, @@ -50,16 +52,32 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { for &item2 in &impl_items2[..] { if (name, namespace) == name_and_namespace(item2) { - struct_span_err!(self.tcx.sess, - self.tcx.span_of_impl(item1).unwrap(), - E0592, - "duplicate definitions with name `{}`", - name) - .span_label(self.tcx.span_of_impl(item1).unwrap(), - format!("duplicate definitions for `{}`", name)) - .span_label(self.tcx.span_of_impl(item2).unwrap(), - format!("other definition for `{}`", name)) - .emit(); + let mut err = struct_span_err!(self.tcx.sess, + self.tcx.span_of_impl(item1).unwrap(), + E0592, + "duplicate definitions with name `{}`", + name); + + err.span_label(self.tcx.span_of_impl(item1).unwrap(), + format!("duplicate definitions for `{}`", name)); + err.span_label(self.tcx.span_of_impl(item2).unwrap(), + format!("other definition for `{}`", name)); + + for cause in &overlap.intercrate_ambiguity_causes { + match cause { + &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { + err.note(&format!("downstream crates may implement {}", + self.tcx.item_path_str(def_id))); + } + &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { + err.note(&format!("upstream crates may add new impl for {} \ + in future versions", + self.tcx.item_path_str(def_id))); + } + } + } + + err.emit(); } } } @@ -71,8 +89,9 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { for (i, &impl1_def_id) in impls.iter().enumerate() { for &impl2_def_id in &impls[(i + 1)..] { self.tcx.infer_ctxt().enter(|infcx| { - 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) + if let Some(overlap) = + traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) { + self.check_for_common_items_in_impls(impl1_def_id, impl2_def_id, overlap) } }); } From eeb16a1e0c626b55b00f5970bb917d91a1ca081d Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 25 Jul 2017 12:17:51 +0900 Subject: [PATCH 2/9] Slightly modify hint messages. --- src/librustc/traits/specialize/mod.rs | 4 ++-- src/librustc_typeck/coherence/inherent_impls_overlap.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index 5ae6fec000fd8..c4b79003a1258 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -347,11 +347,11 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx for cause in &overlap.intercrate_ambiguity_causes { match cause { &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { - err.note(&format!("downstream crates may implement {}", + err.note(&format!("downstream crates may implement `{}`", tcx.item_path_str(def_id))); } &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { - err.note(&format!("upstream crates may add new impl for {} \ + err.note(&format!("upstream crates may add new impl for `{}` \ in future versions", tcx.item_path_str(def_id))); } diff --git a/src/librustc_typeck/coherence/inherent_impls_overlap.rs b/src/librustc_typeck/coherence/inherent_impls_overlap.rs index 07de54233041c..e733b5328d3ba 100644 --- a/src/librustc_typeck/coherence/inherent_impls_overlap.rs +++ b/src/librustc_typeck/coherence/inherent_impls_overlap.rs @@ -66,11 +66,11 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { for cause in &overlap.intercrate_ambiguity_causes { match cause { &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { - err.note(&format!("downstream crates may implement {}", + err.note(&format!("downstream crates may implement `{}`", self.tcx.item_path_str(def_id))); } &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { - err.note(&format!("upstream crates may add new impl for {} \ + err.note(&format!("upstream crates may add new impl for `{}` \ in future versions", self.tcx.item_path_str(def_id))); } From c18c1d5b7bca7356a216438c1847d2a625707415 Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 25 Jul 2017 15:46:26 +0900 Subject: [PATCH 3/9] Add tests for intercrate ambiguity hints. --- .../coherence-overlap-issue-23516-inherent.rs | 26 +++++++++++++++++ .../coherence-overlap-issue-23516.rs | 7 ++++- .../coherence-overlap-upstream-inherent.rs | 28 +++++++++++++++++++ .../coherence-overlap-upstream.rs | 28 +++++++++++++++++++ .../overlapping_inherent_impls.stderr | 1 + 5 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs create mode 100644 src/test/compile-fail/coherence-overlap-upstream-inherent.rs create mode 100644 src/test/compile-fail/coherence-overlap-upstream.rs diff --git a/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs new file mode 100644 index 0000000000000..dfdcc1bc7cb43 --- /dev/null +++ b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs @@ -0,0 +1,26 @@ +// 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. + +// Tests that we consider `Box: !Sugar` to be ambiguous, even +// though we see no impl of `Sugar` for `Box`. Therefore, an overlap +// error is reported for the following pair of impls (#23516). + +pub trait Sugar {} + +struct Cake(X); + +impl Cake { fn dummy(&self) { } } +//~^ ERROR E0592 +//~| NOTE duplicate definitions for `dummy` +//~| NOTE upstream crates may add new impl for `Sugar` in future versions +impl Cake> { fn dummy(&self) { } } +//~^ NOTE other definition for `dummy` + +fn main() { } diff --git a/src/test/compile-fail/coherence-overlap-issue-23516.rs b/src/test/compile-fail/coherence-overlap-issue-23516.rs index 51d7c3e8b4cb1..ffde4011ee4d8 100644 --- a/src/test/compile-fail/coherence-overlap-issue-23516.rs +++ b/src/test/compile-fail/coherence-overlap-issue-23516.rs @@ -15,5 +15,10 @@ pub trait Sugar { fn dummy(&self) { } } pub trait Sweet { fn dummy(&self) { } } impl Sweet for T { } -impl Sweet for Box { } //~ ERROR E0119 +//~^ NOTE first implementation here +impl Sweet for Box { } +//~^ ERROR E0119 +//~| NOTE conflicting implementation for `std::boxed::Box<_>` +//~| NOTE upstream crates may add new impl for `Sugar` in future versions + fn main() { } diff --git a/src/test/compile-fail/coherence-overlap-upstream-inherent.rs b/src/test/compile-fail/coherence-overlap-upstream-inherent.rs new file mode 100644 index 0000000000000..2ec51b1bbe33e --- /dev/null +++ b/src/test/compile-fail/coherence-overlap-upstream-inherent.rs @@ -0,0 +1,28 @@ +// 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. + +// Tests that we consider `i16: Remote` to be ambiguous, even +// though the upstream crate doesn't implement it for now. + +// aux-build:coherence_lib.rs + +extern crate coherence_lib; + +use coherence_lib::Remote; + +struct A(X); +impl A where T: Remote { fn dummy(&self) { } } +//~^ ERROR E0592 +//~| NOTE duplicate definitions for `dummy` +//~| NOTE upstream crates may add new impl for `coherence_lib::Remote` in future versions +impl A { fn dummy(&self) { } } +//~^ NOTE other definition for `dummy` + +fn main() {} diff --git a/src/test/compile-fail/coherence-overlap-upstream.rs b/src/test/compile-fail/coherence-overlap-upstream.rs new file mode 100644 index 0000000000000..81c22e0685052 --- /dev/null +++ b/src/test/compile-fail/coherence-overlap-upstream.rs @@ -0,0 +1,28 @@ +// 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. + +// Tests that we consider `i16: Remote` to be ambiguous, even +// though the upstream crate doesn't implement it for now. + +// aux-build:coherence_lib.rs + +extern crate coherence_lib; + +use coherence_lib::Remote; + +trait Foo {} +impl Foo for T where T: Remote {} +//~^ NOTE first implementation here +impl Foo for i16 {} +//~^ ERROR E0119 +//~| NOTE conflicting implementation for `i16` +//~| NOTE upstream crates may add new impl for `coherence_lib::Remote` in future versions + +fn main() {} diff --git a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr index de8a24cf33f44..186bccdd25407 100644 --- a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr +++ b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr @@ -24,6 +24,7 @@ error[E0592]: duplicate definitions with name `baz` ... 43 | fn baz(&self) {} | ---------------- other definition for `baz` + = note: upstream crates may add new impl for `std::marker::Copy` in future versions error: aborting due to 3 previous errors From 51e92bb00b03b2e14fef74478bbb02bbc6b554c4 Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 25 Jul 2017 16:50:49 +0900 Subject: [PATCH 4/9] Fix a very subtle mistake in a ui test. --- src/test/ui/codemap_tests/overlapping_inherent_impls.stderr | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr index 186bccdd25407..f6ca4f15f9ed8 100644 --- a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr +++ b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr @@ -24,6 +24,7 @@ error[E0592]: duplicate definitions with name `baz` ... 43 | fn baz(&self) {} | ---------------- other definition for `baz` + | = note: upstream crates may add new impl for `std::marker::Copy` in future versions error: aborting due to 3 previous errors From 2cc72866eadf7928aab42c4f2ec527d7dcd17679 Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 25 Jul 2017 16:52:36 +0900 Subject: [PATCH 5/9] Unify intercrate ambiguity emitters into a function. --- src/librustc/traits/select.rs | 20 +++++++++++++++++++ src/librustc/traits/specialize/mod.rs | 12 +---------- .../coherence/inherent_impls_overlap.rs | 13 +----------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index d08203ae6d44a..9aa95233e4718 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -100,6 +100,26 @@ pub enum IntercrateAmbiguityCause { UpstreamCrateUpdate(DefId), } +impl IntercrateAmbiguityCause { + /// Emits notes when the overlap is caused by complex intercrate ambiguities. + /// See #23980 for details. + pub fn add_intercrate_ambiguity_hint<'a, 'tcx>(&self, + tcx: TyCtxt<'a, 'tcx, 'tcx>, + err: &mut ::errors::DiagnosticBuilder) { + match self { + &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { + err.note(&format!("downstream crates may implement `{}`", + tcx.item_path_str(def_id))); + } + &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { + err.note(&format!("upstream crates may add new impl for `{}` \ + in future versions", + tcx.item_path_str(def_id))); + } + } + } +} + // A stack that walks back up the stack frame. struct TraitObligationStack<'prev, 'tcx: 'prev> { obligation: &'prev TraitObligation<'tcx>, diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index c4b79003a1258..88cbc03a4631d 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -345,17 +345,7 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx } for cause in &overlap.intercrate_ambiguity_causes { - match cause { - &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { - err.note(&format!("downstream crates may implement `{}`", - tcx.item_path_str(def_id))); - } - &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { - err.note(&format!("upstream crates may add new impl for `{}` \ - in future versions", - tcx.item_path_str(def_id))); - } - } + cause.add_intercrate_ambiguity_hint(tcx, &mut err); } err.emit(); diff --git a/src/librustc_typeck/coherence/inherent_impls_overlap.rs b/src/librustc_typeck/coherence/inherent_impls_overlap.rs index e733b5328d3ba..0fad16c958222 100644 --- a/src/librustc_typeck/coherence/inherent_impls_overlap.rs +++ b/src/librustc_typeck/coherence/inherent_impls_overlap.rs @@ -12,7 +12,6 @@ use rustc::hir::def_id::{CrateNum, DefId, LOCAL_CRATE}; use rustc::hir; use rustc::hir::itemlikevisit::ItemLikeVisitor; use rustc::traits; -use rustc::traits::IntercrateAmbiguityCause; use rustc::ty::{self, TyCtxt}; pub fn crate_inherent_impls_overlap_check<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, @@ -64,17 +63,7 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { format!("other definition for `{}`", name)); for cause in &overlap.intercrate_ambiguity_causes { - match cause { - &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { - err.note(&format!("downstream crates may implement `{}`", - self.tcx.item_path_str(def_id))); - } - &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { - err.note(&format!("upstream crates may add new impl for `{}` \ - in future versions", - self.tcx.item_path_str(def_id))); - } - } + cause.add_intercrate_ambiguity_hint(self.tcx, &mut err); } err.emit(); From 24abea97c7130ac2031f5105dae42e3eb0a867ab Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 1 Aug 2017 10:27:25 +0430 Subject: [PATCH 6/9] Print more detailed trait-ref for intercrate ambiguity. --- src/librustc/traits/select.rs | 59 ++++++++++++++----- src/librustc/traits/specialize/mod.rs | 2 +- .../coherence/inherent_impls_overlap.rs | 2 +- .../coherence-overlap-issue-23516-inherent.rs | 2 +- .../coherence-overlap-issue-23516.rs | 2 +- .../coherence-overlap-upstream-inherent.rs | 2 +- .../coherence-overlap-upstream.rs | 2 +- .../overlapping_inherent_impls.stderr | 2 +- 8 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 9aa95233e4718..6e51f04146e27 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -96,25 +96,36 @@ pub struct SelectionContext<'cx, 'gcx: 'cx+'tcx, 'tcx: 'cx> { #[derive(Clone)] pub enum IntercrateAmbiguityCause { - DownstreamCrate(DefId), - UpstreamCrateUpdate(DefId), + DownstreamCrate { + trait_desc: String, + self_desc: Option, + }, + UpstreamCrateUpdate { + trait_desc: String, + self_desc: Option, + }, } impl IntercrateAmbiguityCause { /// Emits notes when the overlap is caused by complex intercrate ambiguities. /// See #23980 for details. pub fn add_intercrate_ambiguity_hint<'a, 'tcx>(&self, - tcx: TyCtxt<'a, 'tcx, 'tcx>, err: &mut ::errors::DiagnosticBuilder) { match self { - &IntercrateAmbiguityCause::DownstreamCrate(def_id) => { - err.note(&format!("downstream crates may implement `{}`", - tcx.item_path_str(def_id))); - } - &IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => { - err.note(&format!("upstream crates may add new impl for `{}` \ + &IntercrateAmbiguityCause::DownstreamCrate { ref trait_desc, ref self_desc } => { + let self_desc = if let &Some(ref ty) = self_desc { + format!(" for type `{}`", ty) + } else { "".to_string() }; + err.note(&format!("downstream crates may implement trait `{}`{}", + trait_desc, self_desc)); + } + &IntercrateAmbiguityCause::UpstreamCrateUpdate { ref trait_desc, ref self_desc } => { + let self_desc = if let &Some(ref ty) = self_desc { + format!(" for type `{}`", ty) + } else { "".to_string() }; + err.note(&format!("upstream crates may add new impl of trait `{}`{} \ in future versions", - tcx.item_path_str(def_id))); + trait_desc, self_desc)); } } } @@ -788,9 +799,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // Heuristics: show the diagnostics when there are no candidates in crate. if let Ok(candidate_set) = self.assemble_candidates(stack) { if !candidate_set.ambiguous && candidate_set.vec.is_empty() { - let did = stack.fresh_trait_ref.def_id(); - self.intercrate_ambiguity_causes.push( - IntercrateAmbiguityCause::DownstreamCrate(did)); + let trait_ref = stack.obligation.predicate.skip_binder().trait_ref; + let self_ty = trait_ref.self_ty(); + let cause = IntercrateAmbiguityCause::DownstreamCrate { + trait_desc: trait_ref.to_string(), + self_desc: if self_ty.has_concrete_skeleton() { + Some(self_ty.to_string()) + } else { + None + }, + }; + self.intercrate_ambiguity_causes.push(cause); } } return EvaluatedToAmbig; @@ -1050,9 +1069,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { // Heuristics: show the diagnostics when there are no candidates in crate. let candidate_set = self.assemble_candidates(stack)?; if !candidate_set.ambiguous && candidate_set.vec.is_empty() { - let did = stack.obligation.predicate.def_id(); - self.intercrate_ambiguity_causes.push( - IntercrateAmbiguityCause::UpstreamCrateUpdate(did)); + let trait_ref = stack.obligation.predicate.skip_binder().trait_ref; + let self_ty = trait_ref.self_ty(); + let cause = IntercrateAmbiguityCause::UpstreamCrateUpdate { + trait_desc: trait_ref.to_string(), + self_desc: if self_ty.has_concrete_skeleton() { + Some(self_ty.to_string()) + } else { + None + }, + }; + self.intercrate_ambiguity_causes.push(cause); } return Ok(None); } diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index 88cbc03a4631d..7dbf37decd594 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -345,7 +345,7 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx } for cause in &overlap.intercrate_ambiguity_causes { - cause.add_intercrate_ambiguity_hint(tcx, &mut err); + cause.add_intercrate_ambiguity_hint(&mut err); } err.emit(); diff --git a/src/librustc_typeck/coherence/inherent_impls_overlap.rs b/src/librustc_typeck/coherence/inherent_impls_overlap.rs index 0fad16c958222..76dcfe36e4fcd 100644 --- a/src/librustc_typeck/coherence/inherent_impls_overlap.rs +++ b/src/librustc_typeck/coherence/inherent_impls_overlap.rs @@ -63,7 +63,7 @@ impl<'a, 'tcx> InherentOverlapChecker<'a, 'tcx> { format!("other definition for `{}`", name)); for cause in &overlap.intercrate_ambiguity_causes { - cause.add_intercrate_ambiguity_hint(self.tcx, &mut err); + cause.add_intercrate_ambiguity_hint(&mut err); } err.emit(); diff --git a/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs index dfdcc1bc7cb43..9b8ad51c5ff7d 100644 --- a/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs +++ b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs @@ -19,7 +19,7 @@ struct Cake(X); impl Cake { fn dummy(&self) { } } //~^ ERROR E0592 //~| NOTE duplicate definitions for `dummy` -//~| NOTE upstream crates may add new impl for `Sugar` in future versions +//~| NOTE upstream crates may add new impl of trait `Sugar` for type `std::boxed::Box<_>` impl Cake> { fn dummy(&self) { } } //~^ NOTE other definition for `dummy` diff --git a/src/test/compile-fail/coherence-overlap-issue-23516.rs b/src/test/compile-fail/coherence-overlap-issue-23516.rs index ffde4011ee4d8..950d1fe29bb91 100644 --- a/src/test/compile-fail/coherence-overlap-issue-23516.rs +++ b/src/test/compile-fail/coherence-overlap-issue-23516.rs @@ -19,6 +19,6 @@ impl Sweet for T { } impl Sweet for Box { } //~^ ERROR E0119 //~| NOTE conflicting implementation for `std::boxed::Box<_>` -//~| NOTE upstream crates may add new impl for `Sugar` in future versions +//~| NOTE upstream crates may add new impl of trait `Sugar` for type `std::boxed::Box<_>` fn main() { } diff --git a/src/test/compile-fail/coherence-overlap-upstream-inherent.rs b/src/test/compile-fail/coherence-overlap-upstream-inherent.rs index 2ec51b1bbe33e..1d0c63110cecd 100644 --- a/src/test/compile-fail/coherence-overlap-upstream-inherent.rs +++ b/src/test/compile-fail/coherence-overlap-upstream-inherent.rs @@ -21,7 +21,7 @@ struct A(X); impl A where T: Remote { fn dummy(&self) { } } //~^ ERROR E0592 //~| NOTE duplicate definitions for `dummy` -//~| NOTE upstream crates may add new impl for `coherence_lib::Remote` in future versions +//~| NOTE upstream crates may add new impl of trait `coherence_lib::Remote` for type `i16` impl A { fn dummy(&self) { } } //~^ NOTE other definition for `dummy` diff --git a/src/test/compile-fail/coherence-overlap-upstream.rs b/src/test/compile-fail/coherence-overlap-upstream.rs index 81c22e0685052..e978143a067c5 100644 --- a/src/test/compile-fail/coherence-overlap-upstream.rs +++ b/src/test/compile-fail/coherence-overlap-upstream.rs @@ -23,6 +23,6 @@ impl Foo for T where T: Remote {} impl Foo for i16 {} //~^ ERROR E0119 //~| NOTE conflicting implementation for `i16` -//~| NOTE upstream crates may add new impl for `coherence_lib::Remote` in future versions +//~| NOTE upstream crates may add new impl of trait `coherence_lib::Remote` for type `i16` fn main() {} diff --git a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr index f6ca4f15f9ed8..eaf42cde22f76 100644 --- a/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr +++ b/src/test/ui/codemap_tests/overlapping_inherent_impls.stderr @@ -25,7 +25,7 @@ error[E0592]: duplicate definitions with name `baz` 43 | fn baz(&self) {} | ---------------- other definition for `baz` | - = note: upstream crates may add new impl for `std::marker::Copy` in future versions + = note: upstream crates may add new impl of trait `std::marker::Copy` for type `std::vec::Vec<_>` in future versions error: aborting due to 3 previous errors From ca684a6476df7f5524fecedef19f3820925d977b Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 1 Aug 2017 11:23:02 +0430 Subject: [PATCH 7/9] Add downstream tests for intercrate ambiguity hints. --- .../coherence-overlap-downstream-inherent.rs | 32 +++++++++++++++++++ .../coherence-overlap-downstream.rs | 32 +++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 src/test/compile-fail/coherence-overlap-downstream-inherent.rs create mode 100644 src/test/compile-fail/coherence-overlap-downstream.rs diff --git a/src/test/compile-fail/coherence-overlap-downstream-inherent.rs b/src/test/compile-fail/coherence-overlap-downstream-inherent.rs new file mode 100644 index 0000000000000..66068b535556c --- /dev/null +++ b/src/test/compile-fail/coherence-overlap-downstream-inherent.rs @@ -0,0 +1,32 @@ +// Copyright 2017 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. + +// Tests that we consider `T: Sugar + Fruit` to be ambiguous, even +// though no impls are found. + +struct Sweet(X); +pub trait Sugar {} +pub trait Fruit {} +impl Sweet { fn dummy(&self) { } } +//~^ ERROR E0592 +//~| NOTE duplicate definitions for `dummy` +impl Sweet { fn dummy(&self) { } } +//~^ NOTE other definition for `dummy` + +trait Bar {} +struct A(T, X); +impl A where T: Bar { fn f(&self) {} } +//~^ ERROR E0592 +//~| NOTE duplicate definitions for `f` +//~| NOTE downstream crates may implement trait `Bar<_>` for type `i32` +impl A { fn f(&self) {} } +//~^ NOTE other definition for `f` + +fn main() {} diff --git a/src/test/compile-fail/coherence-overlap-downstream.rs b/src/test/compile-fail/coherence-overlap-downstream.rs new file mode 100644 index 0000000000000..1df02737dec58 --- /dev/null +++ b/src/test/compile-fail/coherence-overlap-downstream.rs @@ -0,0 +1,32 @@ +// Copyright 2017 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. + +// Tests that we consider `T: Sugar + Fruit` to be ambiguous, even +// though no impls are found. + +pub trait Sugar {} +pub trait Fruit {} +pub trait Sweet {} +impl Sweet for T { } +//~^ NOTE first implementation here +impl Sweet for T { } +//~^ ERROR E0119 +//~| NOTE conflicting implementation + +pub trait Foo {} +pub trait Bar {} +impl Foo for T where T: Bar {} +//~^ NOTE first implementation here +impl Foo for i32 {} +//~^ ERROR E0119 +//~| NOTE conflicting implementation for `i32` +//~| NOTE downstream crates may implement trait `Bar<_>` for type `i32` + +fn main() { } From 59cff34e94cc038a0e07a06b606142c1f01b0708 Mon Sep 17 00:00:00 2001 From: Masaki Hara Date: Tue, 1 Aug 2017 12:37:11 +0430 Subject: [PATCH 8/9] Fix misdetection of upstream intercrate ambiguity. --- src/librustc/traits/select.rs | 21 ++++++++++++------- .../coherence-overlap-issue-23516-inherent.rs | 2 +- .../coherence-overlap-issue-23516.rs | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 6e51f04146e27..a5a95209be035 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -31,7 +31,7 @@ use super::{VtableImplData, VtableObjectData, VtableBuiltinData, use super::util; use dep_graph::{DepNodeIndex, DepKind}; -use hir::def_id::DefId; +use hir::def_id::{DefId, LOCAL_CRATE}; use infer; use infer::{InferCtxt, InferOk, TypeFreshener}; use ty::subst::{Kind, Subst, Substs}; @@ -1071,13 +1071,18 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { if !candidate_set.ambiguous && candidate_set.vec.is_empty() { let trait_ref = stack.obligation.predicate.skip_binder().trait_ref; let self_ty = trait_ref.self_ty(); - let cause = IntercrateAmbiguityCause::UpstreamCrateUpdate { - trait_desc: trait_ref.to_string(), - self_desc: if self_ty.has_concrete_skeleton() { - Some(self_ty.to_string()) - } else { - None - }, + let trait_desc = trait_ref.to_string(); + let self_desc = if self_ty.has_concrete_skeleton() { + Some(self_ty.to_string()) + } else { + None + }; + let cause = if + trait_ref.def_id.krate != LOCAL_CRATE && + !self.tcx().has_attr(trait_ref.def_id, "fundamental") { + IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_desc, self_desc } + } else { + IntercrateAmbiguityCause::DownstreamCrate { trait_desc, self_desc } }; self.intercrate_ambiguity_causes.push(cause); } diff --git a/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs index 9b8ad51c5ff7d..355af60710a9b 100644 --- a/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs +++ b/src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs @@ -19,7 +19,7 @@ struct Cake(X); impl Cake { fn dummy(&self) { } } //~^ ERROR E0592 //~| NOTE duplicate definitions for `dummy` -//~| NOTE upstream crates may add new impl of trait `Sugar` for type `std::boxed::Box<_>` +//~| NOTE downstream crates may implement trait `Sugar` for type `std::boxed::Box<_>` impl Cake> { fn dummy(&self) { } } //~^ NOTE other definition for `dummy` diff --git a/src/test/compile-fail/coherence-overlap-issue-23516.rs b/src/test/compile-fail/coherence-overlap-issue-23516.rs index 950d1fe29bb91..ffef5bf10871a 100644 --- a/src/test/compile-fail/coherence-overlap-issue-23516.rs +++ b/src/test/compile-fail/coherence-overlap-issue-23516.rs @@ -19,6 +19,6 @@ impl Sweet for T { } impl Sweet for Box { } //~^ ERROR E0119 //~| NOTE conflicting implementation for `std::boxed::Box<_>` -//~| NOTE upstream crates may add new impl of trait `Sugar` for type `std::boxed::Box<_>` +//~| NOTE downstream crates may implement trait `Sugar` for type `std::boxed::Box<_>` fn main() { } From 5c287406c23076cc72e2f6d34355d34a5f263d7a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 28 Aug 2017 16:50:41 -0400 Subject: [PATCH 9/9] factor out helper method --- src/librustc/traits/coherence.rs | 10 ++++++---- src/librustc/traits/select.rs | 5 ++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/librustc/traits/coherence.rs b/src/librustc/traits/coherence.rs index dee81b8c4fa28..edb6bcf4f8ccc 100644 --- a/src/librustc/traits/coherence.rs +++ b/src/librustc/traits/coherence.rs @@ -140,10 +140,7 @@ pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, // if the trait is not marked fundamental, then it's always possible that // an ancestor crate will impl this in the future, if they haven't // already - if - trait_ref.def_id.krate != LOCAL_CRATE && - !tcx.has_attr(trait_ref.def_id, "fundamental") - { + if !trait_ref_is_local_or_fundamental(tcx, trait_ref) { debug!("trait_ref_is_knowable: trait is neither local nor fundamental"); return false; } @@ -157,6 +154,11 @@ pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(true)).is_err() } +pub fn trait_ref_is_local_or_fundamental<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, + trait_ref: &ty::TraitRef<'tcx>) { + trait_ref.def_id.krate == LOCAL_CRATE || tcx.has_attr(trait_ref.def_id, "fundamental") +} + pub enum OrphanCheckErr<'tcx> { NoLocalInputType, UncoveredTy(Ty<'tcx>), diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index a5a95209be035..9268ea302cbd5 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -1077,9 +1077,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { } else { None }; - let cause = if - trait_ref.def_id.krate != LOCAL_CRATE && - !self.tcx().has_attr(trait_ref.def_id, "fundamental") { + let cause = if !coherence::trait_ref_is_local_or_fundamental(self.tcx(), + trait_ref) { IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_desc, self_desc } } else { IntercrateAmbiguityCause::DownstreamCrate { trait_desc, self_desc }