Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[diagnostic::on_unimplemented] hint not shown for associated type mismatch #125231

Open
TimNN opened this issue May 17, 2024 · 5 comments
Open
Labels
A-diagnostics Area: Messages for errors, warnings, and lints F-on_unimplemented Error messages that can be tackled with `#[rustc_on_unimplemented]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@TimNN
Copy link
Contributor

TimNN commented May 17, 2024

Code

#[diagnostic::on_unimplemented(message = "Very important message!")]
trait TheImportantOne {}

trait ImplementationDetail {
    type Restriction;
}

impl<T: ImplementationDetail<Restriction = ()>> TheImportantOne for T {}

// Comment out this `impl` to show the expected error message.
impl ImplementationDetail for u8 {
    type Restriction = u8;
}

fn verify<T: TheImportantOne>() {}

pub fn main() {
    verify::<u8>();
}

Current output

error[E0271]: type mismatch resolving `<u8 as ImplementationDetail>::Restriction == ()`
  --> src/main.rs:18:14
   |
18 |     verify::<u8>();
   |              ^^ type mismatch resolving `<u8 as ImplementationDetail>::Restriction == ()`
   |
note: expected this to be `()`
  --> src/main.rs:12:24
   |
12 |     type Restriction = u8;
   |                        ^^
note: required for `u8` to implement `TheImportantOne`
  --> src/main.rs:8:49
   |
8  | impl<T: ImplementationDetail<Restriction = ()>> TheImportantOne for T {}
   |                              ----------------   ^^^^^^^^^^^^^^^     ^
   |                              |
   |                              unsatisfied trait bound introduced here
note: required by a bound in `verify`
  --> src/main.rs:15:14
   |
15 | fn verify<T: TheImportantOne>() {}
   |              ^^^^^^^^^^^^^^^ required by this bound in `verify`

For more information about this error, try `rustc --explain E0271`.

Desired output

The "Very important message!" should be shown somewhere in the output (similar to what is shown if impl ImplementationDetail for u8 is missing entirely).

Rationale and extra context

The fundamental error is required for `u8` to implement `TheImportantOne` in both cases (impl ImplementationDetail for u8 completely missing and impl ImplementationDetail for u8 having the wrong Restriction type). As such, I would expect the on_unimplemented message to display in both cases.

Other cases

No response

Rust Version

Playground nightly (`1.80.0-nightly (2024-05-16 8c127df75fde3d5ad8ef)`) and stable (`1.78.0`).

Anything else?

No response

@TimNN TimNN added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 17, 2024
@fmease fmease added the F-on_unimplemented Error messages that can be tackled with `#[rustc_on_unimplemented]` label May 18, 2024
@weiznich
Copy link
Contributor

I believe that this is expected behavior. #[diagnostic::on_unimplemented()] only applies to error messages that report that a trait is not implemented. The error message in question does report that types are not equal, which is a fundamentally different message. This can be potentially solved by:

As a more general note: Anything in the #[diagnostic] namespace is considered to be a hint for the compiler, which means the compiler is free to ignore that hint for any reason.

@TimNN
Copy link
Contributor Author

TimNN commented May 18, 2024

I though that this might be expected, though it still seems inconsistent to me. It becomes more obvious when you introduce another layer of indirection:

Code
#[diagnostic::on_unimplemented(message = "Very important message!")]
trait TheImportantOne {}

trait Intermediate {}

trait ImplementationDetail {
    type Restriction;
}

impl<T: Intermediate> TheImportantOne for T {}

impl<T: ImplementationDetail<Restriction = ()>> Intermediate for T {}

// Comment out this `impl` to show the expected error message.
impl ImplementationDetail for u8 {
    type Restriction = u8;
}

fn verify<T: TheImportantOne>() {}

pub fn main() {
    verify::<u8>();
}

Without the impl ImplementationDetail for u8, reading the errors from the bottom, we have:

  • note: required by a bound in `verify`
  • note: required for `u8` to implement `TheImportantOne`
  • note: required for `u8` to implement `Intermediate`
  • error[E0277]: Very important message!
    • with a label of Rust's own error the trait `ImplementationDetail` is not implemented for `u8`, which is required by `u8: TheImportantOne`

With the impl ImplementationDetail for u8, again reading from the bottom

  • note: required by a bound in `verify`
  • note: required for `u8` to implement `TheImportantOne`
  • note: required for `u8` to implement `Intermediate`
  • note: expected this to be `()`
  • error[E0271]: type mismatch resolving `<u8 as ImplementationDetail>::Restriction == ()`

To me, it feels like the final error message / code should not matter. As soon we get to the note: required for `u8` to implement `TheImportantOne` part, I would expect the on_unimplemented message to be shown, because that's the highest-level problem.

And I could resolve that highest-level problem by simply doing impl TheImportantOne for u8 {} directly. That the compiler is smart enough to figure out alternative ways to resolve the problem (e.g. impl Intermediate for u8 {} or changing the <u8 as ImplementationDetail>::Restriction type) is great, but IMO doesn't change that the original problem is note: required for `u8` to implement `TheImportantOne` (which IMO should trigger the on_unimplemented).

I guess the current behavior makes sense if on_unimplemented is defined to only apply if the "main reported" error is E0277, but as described above, that feel unintuitive to me.


Anyway, you can look at this as a bug, and enhancement request, or just some feedback, so feel free to close as "working as intended" if you think that's what it is.

@estebank
Copy link
Contributor

It makes sense as a feature request. We never had this for #[rustc_on_unimplemented] so we didn't think about it for #[diagnostic::on_unimplemented]. We'd have to first figure out what the actual fallout of changing the behavior would be, as the notes might be specific for E0277 and be inappropriate for E0271. We should add #[diagnostic::on_type_error], but it also feels like it would be different to this. We might want to add either a new attribute for these or a filtering mechanism to on_unimplemented so that a note that only matters in E0277 is only shown there.

@weiznich
Copy link
Contributor

I would argue that the "correct" behaviour is highly dependent on your use case. Is it more correct to suggest to implement TheImportantTrait or is it more correct to follow the generic implementation. That's ultimately something that only the author of the crate could answer.

In my personal opinion the correct solution for the former way would be to use the upcoming #[do_ not_recommend] attribute to hide the generic impl from the error message. After that you would get a "TheImportatTrait not implemented for u8" error, which could be customised via the on_unimplemented attribute. For the later case it might be more meaningful to have something that customises the error message for type mismatch errors.

@weiznich
Copy link
Contributor

I've opened #125717 to address this via the #[do_not_recommend] approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints F-on_unimplemented Error messages that can be tackled with `#[rustc_on_unimplemented]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants