Skip to content
Merged
Next Next commit
Add hints when intercrate ambiguity causes overlap.
  • Loading branch information
qnighy authored and nikomatsakis committed Sep 5, 2017
commit d49f27826388ae62467d2111f9e82a828c3e2ea8
15 changes: 12 additions & 3 deletions src/librustc/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<IntercrateAmbiguityCause>,
}

/// 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<ty::ImplHeader<'tcx>>
-> Option<OverlapResult<'tcx>>
{
debug!("impl_can_satisfy(\
impl1_def_id={:?}, \
Expand Down Expand Up @@ -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<ty::ImplHeader<'tcx>>
-> Option<OverlapResult<'tcx>>
{
debug!("overlap(a_def_id={:?}, b_def_id={:?})",
a_def_id,
Expand Down Expand Up @@ -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>,
Expand Down
5 changes: 2 additions & 3 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -39,6 +37,7 @@ pub use self::object_safety::ObjectSafetyViolation;
pub use self::object_safety::MethodViolationCode;
pub use self::on_unimplemented::{OnUnimplementedDirective, OnUnimplementedNote};
pub use self::select::{EvaluationCache, SelectionContext, SelectionCache};
pub use self::select::IntercrateAmbiguityCause;
pub use self::specialize::{OverlapError, specialization_graph, translate_substs};
pub use self::specialize::{SpecializesCache, find_associated_item};
pub use self::util::elaborate_predicates;
Expand Down
29 changes: 29 additions & 0 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@ pub struct SelectionContext<'cx, 'gcx: 'cx+'tcx, 'tcx: 'cx> {
intercrate: bool,

inferred_obligations: SnapshotVec<InferredObligationsSnapshotVecDelegate<'tcx>>,

intercrate_ambiguity_causes: Vec<IntercrateAmbiguityCause>,
}

#[derive(Clone)]
pub enum IntercrateAmbiguityCause {
DownstreamCrate(DefId),
UpstreamCrateUpdate(DefId),
}

// A stack that walks back up the stack frame.
Expand Down Expand Up @@ -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(),
}
}

Expand All @@ -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(),
}
}

Expand All @@ -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<R, F>(&mut self, f: F) -> R
Expand Down Expand Up @@ -757,6 +771,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 &&
Expand Down Expand Up @@ -1003,6 +1025,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);
}

Expand Down
16 changes: 16 additions & 0 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,6 +37,7 @@ pub struct OverlapError {
pub with_impl: DefId,
pub trait_desc: String,
pub self_desc: Option<String>,
pub intercrate_ambiguity_causes: Vec<IntercrateAmbiguityCause>,
}

/// Given a subst for the requested impl, translate it to a subst
Expand Down Expand Up @@ -337,6 +339,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 {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: use backticks, like:

may implement `{}`

tcx.item_path_str(def_id)));
}
&IntercrateAmbiguityCause::UpstreamCrateUpdate(def_id) => {
err.note(&format!("upstream crates may add new impl for {} \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

in future versions",
tcx.item_path_str(def_id)));
}
}
}

err.emit();
}
} else {
Expand Down
7 changes: 4 additions & 3 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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,
Expand All @@ -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))
Expand Down
45 changes: 32 additions & 13 deletions src/librustc_typeck/coherence/inherent_impls_overlap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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,
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: seems like we should extract this into some sort of helper; I feel like the same code was present elsewherein the PR, no?

match cause {
&IntercrateAmbiguityCause::DownstreamCrate(def_id) => {
err.note(&format!("downstream crates may implement {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit: use backticks around {}, like:

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();
}
}
}
Expand All @@ -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)
}
});
}
Expand Down