Add hints when intercrate ambiguity causes overlap. #43426

qnighy · 2017-07-23T13:52:37Z

I'm going to tackle #23980.

Examples

Trait impl overlap caused by possible downstream impl

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

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

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

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

rust-highfive · 2017-07-23T13:52:40Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2017-07-23T14:24:25Z

r? @nikomatsakis or @aturon

nikomatsakis · 2017-07-23T17:08:48Z

This looks exciting! :)

nikomatsakis · 2017-07-23T17:08:57Z

...and I didn't mean to close that...

nikomatsakis · 2017-07-23T18:52:45Z

src/librustc/traits/specialize/mod.rs

+                for cause in &overlap.intercrate_ambiguity_causes {
+                    match cause {
+                        &IntercrateAmbiguityCause::DownstreamCrate(def_id) => {
+                            err.note(&format!("downstream crates may implement {}",


Nit: use backticks, like:

may implement `{}`

nikomatsakis · 2017-07-23T18:52:56Z

src/librustc/traits/specialize/mod.rs

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


nikomatsakis

I left one nit, but the most important thing that is missing is tests! It would be great to have new tests in src/test/ui/coherence (you'll have to create the directory) showing these various errors. You could for example move the cases from your PR description into tests. (You can read more about how UI tests work in the src/test/COMPILER_TESTS.md file.)

I'll have to think about the general heuristic a bit and whether these errors will be misleading, but it seems quite clever to me at first glance. Making a bunch of tests for various scenarios, however, is always a good way to show that it generates good errors!

nikomatsakis · 2017-07-23T18:53:42Z

src/librustc_typeck/coherence/inherent_impls_overlap.rs

+                    err.span_label(self.tcx.span_of_impl(item2).unwrap(),
+                                   format!("other definition for `{}`", name));
+
+                    for cause in &overlap.intercrate_ambiguity_causes {


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

nikomatsakis · 2017-07-23T18:54:04Z

src/librustc_typeck/coherence/inherent_impls_overlap.rs

+                    for cause in &overlap.intercrate_ambiguity_causes {
+                        match cause {
+                            &IntercrateAmbiguityCause::DownstreamCrate(def_id) => {
+                                err.note(&format!("downstream crates may implement {}",


same nit: use backticks around {}, like:

may implement `{}`

qnighy · 2017-07-24T07:33:32Z

One problem I found by myself after submitting this PR is that it can't handle the example mentioned in #36162:

trait A {}
trait B {}

trait Foo { }

impl<T: A> Foo for T { }
impl<T: B> Foo for T { }

As far as I understand, there are several causes of this problem:

assemble_candidates gives up early if Self is a typevar.
_: A and _: B each have an in-crate candidate because they are object-safe, whereas there are no in-crate solutions satisfying both of them.

qnighy · 2017-07-26T06:27:57Z

All changes done! For now I'm going to leave the problem I mentioned above...

nikomatsakis · 2017-07-27T17:21:17Z

src/test/compile-fail/coherence-overlap-issue-23516-inherent.rs

+impl<T:Sugar> Cake<T> { fn dummy(&self) { } }
+//~^ ERROR E0592
+//~| NOTE duplicate definitions for `dummy`
+//~| NOTE upstream crates may add new impl for `Sugar` in future versions


Hmm, I think if we captured a bit more of the trait-ref, we could say something like "upstream crates may implement Sugar for Box<_> in the future", which might be more helpful, what do you think?

nikomatsakis · 2017-07-27T17:21:50Z

src/test/compile-fail/coherence-overlap-upstream.rs

+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


similarly here

nikomatsakis · 2017-07-27T17:36:09Z

I agree we don't have to solve all problems -- what do you think of the suggestions above though?

qnighy · 2017-08-01T08:36:19Z

Improved message by printing trait-refs.

r? @nikomatsakis

nikomatsakis

Looks great! I left one more minor nit, just a small refactoring.

nikomatsakis · 2017-08-04T06:56:55Z

src/librustc/traits/select.rs

-    DownstreamCrate(DefId),
-    UpstreamCrateUpdate(DefId),
+    DownstreamCrate {
+        trait_desc: String,


Hmm, I wonder -- why pass a String and not a Ty<'tcx>? The latter has the advantage of being cheaper; but do we only generate these structs in error cases anyway? I guess it's fine then.

nikomatsakis · 2017-08-04T07:08:35Z

src/librustc/traits/select.rs

+                };
+                let cause = if
+                    trait_ref.def_id.krate != LOCAL_CRATE &&
+                    !self.tcx().has_attr(trait_ref.def_id, "fundamental") {


Nit: This feels a bit like a DRY violation -- this same logic appears in traits::trait_ref_is_knowable() -- maybe factor it out into a helper? Something like trait_ref_permits_any_impl?

alexcrichton · 2017-08-10T19:00:40Z

ping @qnighy, just want to make sure this is still on your radar!

arielb1 · 2017-08-22T22:05:35Z

I think this just needs that little nit to land. Are you going to close it @qnighy ?

carols10cents · 2017-08-28T12:31:06Z

Hi @qnighy , thanks for the PR! Since you haven't said anything recently though, we're going to close this-- please feel free to reopen when you have a chance to take care of the last nit.

nikomatsakis · 2017-08-28T16:23:06Z

Hmm, I hate to see this PR get closed for such a small nit. I could handle it myself, or we could probably even just land it as is, it's not that big a thing.

nikomatsakis · 2017-08-28T20:56:19Z

OK, I made the changes myself.

nikomatsakis · 2017-08-28T20:56:30Z

@bors r+

nikomatsakis · 2017-09-05T16:32:29Z

@bors r+

bors · 2017-09-05T16:32:30Z

📌 Commit 37c9a60 has been approved by nikomatsakis

bors · 2017-09-05T18:22:07Z

⌛ Testing commit 37c9a60 with merge c47f8ca1419933d131d422084718f3864c361d2f...

bors · 2017-09-05T18:22:29Z

⌛ Testing commit 37c9a60 with merge 51b907cdbe9cefb646c75bb4952468495218b056...

bors · 2017-09-05T19:04:12Z

💔 Test failed - status-travis

alexcrichton · 2017-09-05T21:07:06Z

@bors: retry

good ol' travis troubles

bors · 2017-09-05T21:30:18Z

⌛ Testing commit 37c9a60 with merge faf99d5c0b80fa68131f2d0fb8db9d968e345950...

bors · 2017-09-05T22:32:48Z

💔 Test failed - status-travis

kennytm · 2017-09-05T22:47:46Z

@bors retry

check x86_64-apple-darwin timed out, no output for 30 minutes.

bors · 2017-09-06T00:28:22Z

⌛ Testing commit 37c9a60 with merge f83d20e...

Add hints when intercrate ambiguity causes overlap. I'm going to tackle #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 ```

bors · 2017-09-06T03:12:02Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing f83d20e to master...

rust-highfive assigned eddyb Jul 23, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Jul 23, 2017

nikomatsakis closed this Jul 23, 2017

nikomatsakis reopened this Jul 23, 2017

nikomatsakis reviewed Jul 23, 2017

View reviewed changes

nikomatsakis requested changes Jul 23, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 24, 2017

nikomatsakis reviewed Jul 27, 2017

View reviewed changes

nikomatsakis approved these changes Jul 27, 2017

View reviewed changes

nikomatsakis requested changes Aug 4, 2017

View reviewed changes

carols10cents closed this Aug 28, 2017

carols10cents added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2017

nikomatsakis reopened this Aug 28, 2017

qnighy and others added 9 commits September 5, 2017 12:19

Add hints when intercrate ambiguity causes overlap.

d49f278

Slightly modify hint messages.

bebd622

Add tests for intercrate ambiguity hints.

a59cc32

Fix a very subtle mistake in a ui test.

043786d

Unify intercrate ambiguity emitters into a function.

84bfc33

Print more detailed trait-ref for intercrate ambiguity.

d153ff3

Add downstream tests for intercrate ambiguity hints.

4d503b0

Fix misdetection of upstream intercrate ambiguity.

099bb1b

factor out helper method

37c9a60

nikomatsakis force-pushed the intercrate-ambiguity-hints branch from 3d7cde6 to 37c9a60 Compare September 5, 2017 16:19

arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2017

bors merged commit 37c9a60 into rust-lang:master Sep 6, 2017

qnighy mentioned this pull request Jan 25, 2018

remove intercrate ambiguity hints #47738

Merged

taiki-e mentioned this pull request Sep 19, 2019

IntoStream for vec async-rs/async-std#213

Closed

Add hints when intercrate ambiguity causes overlap. #43426

Add hints when intercrate ambiguity causes overlap. #43426

Conversation

qnighy commented Jul 23, 2017

Examples

Trait impl overlap caused by possible downstream impl

Trait impl overlap caused by possible upstream update

Inherent impl overlap caused by possible downstream impl

Inherent impl overlap caused by possible upstream update

rust-highfive commented Jul 23, 2017

eddyb commented Jul 23, 2017

nikomatsakis commented Jul 23, 2017

nikomatsakis commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qnighy commented Jul 24, 2017

qnighy commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jul 27, 2017

qnighy commented Aug 1, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 10, 2017

arielb1 commented Aug 22, 2017

carols10cents commented Aug 28, 2017

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

kennytm commented Sep 5, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017