Error messages are odd for delegating traits

[nagisa]

This is a more general version of #22478. (i.e. probably should be considered to supersede it)

trait A {
    fn a(self);
}

trait B {
    fn b(self);
}

impl<T: B> A for T {
    fn a(self) {
        B::b(self)
    }
}

struct C;

fn main() {
    A::a(C);
}

Compiling this will result in

t.rs:18:5: 18:9 error: the trait `B` is not implemented for the type `C` [E0277]
t.rs:18     A::a(C);
            ^~~~
error: aborting due to previous error

but impl<T: B> A for T, T = C and C does not implement B and therefore the impl in question should not even be taken into consideration, even if it is the only implementation of a trait.

[alexcrichton]

triage: I-nominated

nominating for 1.0 polish as this is quite a confusing error message that leaks into many places. For example if you pass something to a function requiring a FnMut and doesn't satisfy it the compiler will complain about a lack of Fn implementation.

[pnkfelix]

we could provide more of a trace in the error message.

anyway this is polish at best, not a 1.0 blocker. P-low. not on 1.0 milestone.

[nikomatsakis]

I can help mentor this.

[az7arul]

@nikomatsakis I want to have a go at this if the mentor offer still stands.

[solson]

This problem still exists.

@nikomatsakis I'm taking a look into this. Any pointers on where I should look?

[arielb1]

The new error message is

<anon>:18:5: 18:9 error: the trait `B` is not implemented for the type `C` [E0277]
<anon>:18     A::a(C);
              ^~~~
<anon>:18:5: 18:9 help: see the detailed explanation for E0277
<anon>:18:5: 18:9 note: required by `A::a` <-- NOTE THIS
error: aborting due to previous error
playpen: application terminated with error code 101

Maybe just improve the error explanation?

[solson]

Ah, I accidentally ran on stable and missed the up-to-date message.

It might be good to show why that is required by A::a. Technically A::a doesn't require that at the trait definition, only in the blanket impl.

[nikomatsakis]

@tsion sorry, I'm quite behind on notifications, but I agree that we could improve the message further. I'd still be happy to mentor -- easiest would be to ping me on IRC (nmatsakis). But the basic idea is that we track the cause of each trait obligation, and in this case we could probably use a more specific cause that would permit a more specific error message. Also, as @aturon noted on another issue recently, we might prefer to invert the print-out, so that we print out the root cause first, and the final error only later. That's a bit harder to do in the general case though I think, not without some greater reorganization of the code that @jroesch has been working on.

[chirag08]

@nikomatsakis I am really interested to work on this, needs your help
Kindly Assign me this issue

[eugene-bulkin]

The current error message is this:

error: the trait bound `C: B` is not satisfied [--explain E0277]
  --> <anon>:18:5
18 |>     A::a(C);
   |>     ^^^^
note: required because of the requirements on the impl of `A` for `C`
note: required by `A::a`

error: aborting due to previous error

That looks like it covers most of the bases, right?

[brson]

@nagisa what do you think of @eugene-bulkin's output above?

[nagisa]

The output is better now, I’m still confused as to why compiler would even begin considering the impl <T: B> A for T. While I have some intuition as to why the compiler would report error as it currently does, I think reporting

error[E0277]: the trait bound `C: A` is not satisfied
  --> <anon>:12:5
   |
12 |     A::a(C);
   |     ^^^^
   |
   = note: required by `A::a` 

is preferable in this particular case.

OTOH, reporting an error message the way as it is done currently automatically improves diagnostics involving marker traits like Sized…

[nagisa]

Perhaps something like

error[E0277]: the trait bound `C: A` is not satisfied
  --> <anon>:12:5
   |
12 |     A::a(C);
   |     ^^^^
   |
   = note: required by `A::a`
   = help: implementing `B` for `C` would satisfy `C: A` because of `impl<T: B> A for T`
   = ...

could be an option.

[nikomatsakis]

I have been wanting for some time to switch to more of a "backtrace" style error. I think we should lead with the top-level thing which is unsatisfied (in this case, C: A, as @nagisa noted) and then offer some note as to the impls that almost applied but didn't (that would be the ones on the backtrace). @nagisa's suggestion is perhaps roughly how I would go (modulo precise wording).

This seems relevant to the abstract model we define for errors, btw (cc @jonathandturner) in that it seems like a clear case where we want "attached notes" to define the backtrace. I would think that ideally those notes would be linked to the impl (like, citing the line/column of the impl when possible).

But bah I'm not sure exactly how I think it should look. I'd like to avoid having to phrase trait errors in a new (i.e., "implementing B for C would satisfy...") because i've found it's annoying and difficult to have to describe every error not just with one string ("trait bound not satisfied" or whatever) but with two, one striking a more conditional tone.

[jonathandturner]

@nikomatsakis

Just sketching it out. Maybe something like this?

error[E0277]: `C` does not implement required traits
  --> src/test/compile-fail/issue-24356.rs:30:9
   |
9  | impl<T: B> A for T {
   |      ----  - `C` must impl `A`
   |      |
   |      `C` must impl `B` 
...
18 |    A::a(C);
   |         ^ `C` does not satisfy constraints

Though we might want to play with the spans. At least seems promising.

[jonathandturner]

A better one...

error[E0277]: `C` does not implement required traits
  --> src/test/compile-fail/issue-24356.rs:30:9
   |
9  | impl<T: B> A for T {
   |      ----  note: implementing `B` can satisfy `A` 
...
18 |    A::a(C);
   |         ^ `C` does not impl `A`

[nagisa]

@jonathandturner remember that delegation chain could be inifinitely long¹, thus we should strive somewhat to ensure that multiple such suggestions can be presented in readable way.

¹: https://play.rust-lang.org/?gist=4a8db1abe419dab9a9fb4e52c4ebf4e3&version=stable&backtrace=0 is an example extended to have an extra trait in the chain.

[jonathandturner]

@nagisa Heuristics are fair game here. What we choose to add in addition to the main error is up to us, but we can opt to only show some of the available additional information.

[nikomatsakis]

Though we might want to play with the spans. At least seems promising.

Interesting. I have no idea what the common depths of the stacktrace are. (Also, commonly the impls will be in other files, but that's ok).

[nikomatsakis]

triage: P-medium

Would be nice, but not P-high. We should write-up some mentoring instructions.

[mikeyhew]

These error messages have always been confusing for me. The most common case is when something requires IntoIterator, but the main error message is complaining about Iterator not being implemented:

error[E0277]: the trait bound `[{integer}; 3]: std::iter::Iterator` is not satisfied
 --> src/main.rs:2:5
  |
2 |     for x in [1,2,3] {}
  |     ^^^^^^^^^^^^^^^^^^^ `[{integer}; 3]` is not an iterator; maybe try calling `.iter()` or a similar method
  |
  = help: the trait `std::iter::Iterator` is not implemented for `[{integer}; 3]`
  = note: required by `std::iter::IntoIterator::into_iter`

I was playing around with some custom_dst related stuff today, and this came back again because I had a blanket impl of DynSized for T: Sized, and all the error messages for when DynSized wasn't implemented were talking about how Sized wasn't implemented... not ideal.

Anyway, I would like to try to fix this, if someone could give me some mentoring notes I'll get started on it this week

[mikeyhew]

First and foremost, I think the primary error message for the above case should be "the trait bound [{integer}; 3]: std::iter::Iterator is not satisfied""the trait bound [{integer}; 3]: std::iter::IntoIterator is not satisfied". That is the actual trait bound that is necessary, so that is what the error should be about. Mentioning the blanket impl (or impls, once multiple blanket impls are allowed), and why it doesn't apply, would be helpful, but that should be a "help" note, not the main error message.

EDIT 2019-01-14: replaced Iterator with IntoIterator in error message

[mikeyhew]

Update: these error messages are still a problem and no progress has been made to make them better, except as a special case for IntoIterator where the error message is now T is not an iterator. This makes things harder for beginners to learn Rust, and it regularly trips up experienced programmers too. Here is a simple example:

trait Foo {}
fn requires_foo<T: Foo>() {}
    
trait Bar {}
impl<T: Bar> Foo for T {}

fn main() {
    // try to call `requires_foo` with a type that doesn't implement it
    requires_foo::<()>();
}

The resulting error message:

error[E0277]: the trait bound `(): Bar` is not satisfied
 --> src/main.rs:9:5
  |
9 |     requires_foo::<()>();
  |     ^^^^^^^^^^^^^^^^^^ the trait `Bar` is not implemented for `()`
  |
  = note: required because of the requirements on the impl of `Foo` for `()`
note: required by `requires_foo`

Compare this to the error message if we comment out the blanket impl of Foo for T where T: Bar:

error[E0277]: the trait bound `(): Foo` is not satisfied
 --> src/main.rs:9:5
  |
9 |     requires_foo::<()>();
  |     ^^^^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `()`
  |
note: required by `requires_foo`

The error message when the blanket impl is present complains that () does not implement Bar. But that's not the real problem, the problem is that () doesn't implement Foo. The compiler should report that as the error, just like it does when there is no blanket impl. And then it can mention the blanket impl of Foo for T where T: Bar, and explain why that doesn't work.

This most recently came up on Reddit here