Error messages are odd for delegating traits

Opened by Simonas Kazlauskas at 2023-05-24 08:41:09

This is a more general version of https://github.com/rust-lang/rust/issues/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.

This is a more general version of https://github.com/rust-lang/rust/issues/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.
    
rustbot at 2020-10-25 11:26:05
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.
Alex Crichton at 2015-03-04 18:15:33
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.
Felix S Klock II at 2015-03-05 21:26:35
I can help mentor this.
Niko Matsakis at 2015-03-05 21:27:08
@nikomatsakis I want to have a go at this if the mentor offer still stands.
Azharul Islam at 2015-03-15 14:57:13
This problem still exists.

@nikomatsakis I'm taking a look into this. Any pointers on where I should look?
Scott Olson at 2015-09-25 02:43:40

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?

Ariel Ben-Yehuda at 2015-09-25 18:16:54

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.
Scott Olson at 2015-09-26 00:47:19
@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.
Niko Matsakis at 2015-09-30 19:06:52
@nikomatsakis I am really interested to work on this, needs your help Kindly Assign me this issue
Chirag Batra at 2016-04-11 09:22:41

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?

Eugene Bulkin at 2016-07-21 01:15:11

@nagisa what do you think of @eugene-bulkin's output above?
Brian Anderson at 2016-07-22 22:16:50
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…
Simonas Kazlauskas at 2016-07-23 11:36:47

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.

Simonas Kazlauskas at 2016-07-23 11:40:09

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.
Niko Matsakis at 2016-07-26 14:15:50

@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.

Sophia J. Turner at 2016-07-26 15:30:24

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`

Sophia J. Turner at 2016-07-26 16:08:21

@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.
Simonas Kazlauskas at 2016-07-26 19:44:11
@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.
Sophia J. Turner at 2016-07-26 19:48:20
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).
Niko Matsakis at 2016-07-27 19:59:20
triage: P-medium

Would be nice, but not P-high. We should write-up some mentoring instructions.
Niko Matsakis at 2017-08-24 21:00:50
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
Michael Hewson at 2017-11-27 05:07:49
First and foremost, I think the primary error message for the above case should be <del>"the trait bound [{integer}; 3]: std::iter::Iterator is not satisfied"</del>"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
Michael Hewson at 2017-11-27 05:17:14
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
Michael Hewson at 2019-01-15 01:28:39
@rustbot claim
Olivier FAURE at 2020-10-25 11:26:03
Update: I have a general idea of where the problem comes from, but I'm not sure what the best fix would be.
- The general problem is that select in compiler/rustc_trait_selection/src/traits/select/mod.rs generates new obligations in a way that assumes any possible match is the one we want. Ideally, we'd want a way to distinguish "optional dependencies" (eg "there exists a blanket impl that you might want to use"), and straightforward dependencies (eg "you're not using that trait if your type isn't Sized and that's final"). I'm not sure what that distinction would look like in the code.
- The selection errors are processed in report_selection_error in compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs. It might be possible to monkey-patch the issue by saying "in the special case that a dependency is a blanket impl on the desired trait, cut off the entire dependency chain". This feels like a hack, but it might be way simpler to implement.
Anybody available for some short mentoring on this?
Olivier FAURE at 2020-11-29 16:15:33