Skip to content

Commit

Permalink
Rollup merge of rust-lang#43426 - qnighy:intercrate-ambiguity-hints, …
Browse files Browse the repository at this point in the history
…r=nikomatsakis

Add hints when intercrate ambiguity causes overlap.

I'm going to tackle rust-lang#23980.

# Examples

## Trait impl overlap caused by possible downstream impl

```rust
trait Foo<X> {}
trait Bar<X> {}
impl<X, T> Foo<X> for T where T: Bar<X> {}
impl<X> Foo<X> for i32 {}

fn main() {}
```

```
error[E0119]: conflicting implementations of trait `Foo<_>` for type `i32`:
 --> test1.rs:4:1
  |
3 | impl<X, T> Foo<X> for T where T: Bar<X> {}
  | ------------------------------------------ first implementation here
4 | impl<X> Foo<X> for i32 {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `i32`
  |
  = note: downstream crates may implement Bar

error: aborting due to previous error
```

## Trait impl overlap caused by possible upstream update

```rust
trait Foo {}
impl<T> Foo for T where T: ::std::fmt::Octal {}
impl Foo for () {}

fn main() {}
```

```
error[E0119]: conflicting implementations of trait `Foo` for type `()`:
 --> test2.rs:3:1
  |
2 | impl<T> Foo for T where T: ::std::fmt::Octal {}
  | ----------------------------------------------- first implementation here
3 | impl Foo for () {}
  | ^^^^^^^^^^^^^^^^^^ conflicting implementation for `()`
  |
  = note: upstream crates may add new impl for std::fmt::Octal in future versions

error: aborting due to previous error
```

## Inherent impl overlap caused by possible downstream impl

```rust
trait Bar<X> {}

struct A<T, X>(T, X);
impl<X, T> A<T, X> where T: Bar<X> { fn f(&self) {} }
impl<X> A<i32, X> { fn f(&self) {} }

fn main() {}
```

```
error[E0592]: duplicate definitions with name `f`
 --> test3.rs:4:38
  |
4 | impl<X, T> A<T, X> where T: Bar<X> { fn f(&self) {} }
  |                                      ^^^^^^^^^^^^^^ duplicate definitions for `f`
5 | impl<X> A<i32, X> { fn f(&self) {} }
  |                     -------------- other definition for `f`
  |
  = note: downstream crates may implement Bar

error: aborting due to previous error
```

## Inherent impl overlap caused by possible upstream update

```rust
struct A<T>(T);

impl<T> A<T> where T: ::std::fmt::Octal { fn f(&self) {} }
impl A<()> { fn f(&self) {} }

fn main() {}
```

```
error[E0592]: duplicate definitions with name `f`
 --> test4.rs:3:43
  |
3 | impl<T> A<T> where T: ::std::fmt::Octal { fn f(&self) {} }
  |                                           ^^^^^^^^^^^^^^ duplicate definitions for `f`
4 | impl A<()> { fn f(&self) {} }
  |              -------------- other definition for `f`
  |
  = note: upstream crates may add new impl for std::fmt::Octal in future versions

error: aborting due to previous error
```
  • Loading branch information
frewsxcv authored Aug 29, 2017
2 parents 6f82dea + 5c28740 commit cd1a286
Show file tree
Hide file tree
Showing 13 changed files with 286 additions and 28 deletions.
25 changes: 18 additions & 7 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 All @@ -131,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;
}
Expand All @@ -148,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>),
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,16 +28,15 @@ 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};
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;
Expand Down
82 changes: 81 additions & 1 deletion src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use super::{VtableImplData, VtableObjectData, VtableBuiltinData, VtableGenerator
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};
Expand Down Expand Up @@ -91,6 +91,45 @@ 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 {
trait_desc: String,
self_desc: Option<String>,
},
UpstreamCrateUpdate {
trait_desc: String,
self_desc: Option<String>,
},
}

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,
err: &mut ::errors::DiagnosticBuilder) {
match self {
&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",
trait_desc, self_desc));
}
}
}
}

// A stack that walks back up the stack frame.
Expand Down Expand Up @@ -390,6 +429,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 @@ -399,6 +439,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 @@ -414,6 +455,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 @@ -767,6 +812,22 @@ 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 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;
}
if unbound_input_types &&
Expand Down Expand Up @@ -1021,6 +1082,25 @@ 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 trait_ref = stack.obligation.predicate.skip_binder().trait_ref;
let self_ty = trait_ref.self_ty();
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 !coherence::trait_ref_is_local_or_fundamental(self.tcx(),
trait_ref) {
IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_desc, self_desc }
} else {
IntercrateAmbiguityCause::DownstreamCrate { trait_desc, self_desc }
};
self.intercrate_ambiguity_causes.push(cause);
}
return Ok(None);
}

Expand Down
6 changes: 6 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 @@ -343,6 +345,10 @@ 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(&mut err);
}

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
34 changes: 21 additions & 13 deletions src/librustc_typeck/coherence/inherent_impls_overlap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,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 +51,22 @@ 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 {
cause.add_intercrate_ambiguity_hint(&mut err);
}

err.emit();
}
}
}
Expand All @@ -71,8 +78,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
32 changes: 32 additions & 0 deletions src/test/compile-fail/coherence-overlap-downstream-inherent.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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>(X);
pub trait Sugar {}
pub trait Fruit {}
impl<T:Sugar> Sweet<T> { fn dummy(&self) { } }
//~^ ERROR E0592
//~| NOTE duplicate definitions for `dummy`
impl<T:Fruit> Sweet<T> { fn dummy(&self) { } }
//~^ NOTE other definition for `dummy`

trait Bar<X> {}
struct A<T, X>(T, X);
impl<X, T> A<T, X> where T: Bar<X> { fn f(&self) {} }
//~^ ERROR E0592
//~| NOTE duplicate definitions for `f`
//~| NOTE downstream crates may implement trait `Bar<_>` for type `i32`
impl<X> A<i32, X> { fn f(&self) {} }
//~^ NOTE other definition for `f`

fn main() {}
32 changes: 32 additions & 0 deletions src/test/compile-fail/coherence-overlap-downstream.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<T:Sugar> Sweet for T { }
//~^ NOTE first implementation here
impl<T:Fruit> Sweet for T { }
//~^ ERROR E0119
//~| NOTE conflicting implementation

pub trait Foo<X> {}
pub trait Bar<X> {}
impl<X, T> Foo<X> for T where T: Bar<X> {}
//~^ NOTE first implementation here
impl<X> Foo<X> for i32 {}
//~^ ERROR E0119
//~| NOTE conflicting implementation for `i32`
//~| NOTE downstream crates may implement trait `Bar<_>` for type `i32`

fn main() { }
Loading

0 comments on commit cd1a286

Please sign in to comment.