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

Diagnostics talk about fn pointer when no pointers are in the source #67296

Closed
oli-obk opened this issue Dec 14, 2019 · 8 comments · Fixed by #106131
Closed

Diagnostics talk about fn pointer when no pointers are in the source #67296

oli-obk opened this issue Dec 14, 2019 · 8 comments · Fixed by #106131
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-papercut Diagnostics: An error or lint that needs small tweaks. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2019

Trait function API mismatches (where the impl differs from the declaration) report the correct error about the mismatch, but also give a more detailed note like

  = note: expected fn pointer `fn(&())`
             found fn pointer `fn(())`

This could cause some confusion as the user hasn't done anything with function pointers.

trait Foo {
    fn foo(&self);
}

impl Foo for () {
    fn foo(self) {}
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0053]: method `foo` has an incompatible type for trait
 --> src/lib.rs:6:12
  |
2 |     fn foo(&self);
  |            ----- type in trait
...
6 |     fn foo(self) {}
  |            ^^^^ expected `&()`, found `()`
  |
  = note: expected fn pointer `fn(&())`
             found fn pointer `fn(())`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0053`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

@oli-obk oli-obk added A-diagnostics Area: Messages for errors, warnings, and lints P-low Low priority D-papercut Diagnostics: An error or lint that needs small tweaks. labels Dec 14, 2019
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 14, 2019
@Centril Centril added the C-bug Category: This is a bug. label Dec 14, 2019
@Centril
Copy link
Contributor

Centril commented Dec 14, 2019

cc @estebank

@estebank
Copy link
Contributor

What are some appropriate wordings? fn signature? fn? fn type? Revert this one back to just type?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 14, 2019

Maybe without any descriptive word?

  = note: expected `fn(&())`
             found `fn(())`

Is there any particular reason we're displaying the note at all? In the examples I was able to coax out of the compiler the main message was the part that was helpful, the note was just redundant.

@estebank
Copy link
Contributor

This used to be "expected type" which was also confusing when showing things that weren't really something that people associated with types, so it's using the TyKind description now. We could remove it entirely, but I find that having a small level of redundancy in our wording can help introduce and teach concepts to new users. This is why when I saw this I didn't change this to something like "function", but I'm really happy you opened this conversation.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 15, 2019

This used to be "expected type"

Yea that was quite suboptimal

We could remove it entirely, but I find that having a small level of redundancy in our wording can help introduce and teach concepts to new users

Right, having different verbosity levels for different experience levels is a different discussion, so: more verbosity is better for now. What do you think about

  = note: expected function with signature `fn(&())`
             found function with signature `fn(())`

or does that get too long for complex signatures?

@estebank
Copy link
Contributor

It makes sense but seems a bit long, I wonder if we can simultaneously make it shorter and less "jargony" by doing something like (for fn pointers specifically):

  = note: expected function like `fn(&())`
             found function like `fn(())`

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 15, 2019

Oh yea, that's good

@compiler-errors
Copy link
Member

Original problem is fixed by #106131, not sure about the rest of the discussion but probably should be moved to a different issue.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Jan 9, 2023
Mention "signature" rather than "fn pointer" when impl/trait methods are incompatible

Fixes rust-lang#80929
Fixes rust-lang#67296
@bors bors closed this as completed in 6afd161 Jan 9, 2023
@compiler-errors compiler-errors self-assigned this Dec 11, 2023
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 C-bug Category: This is a bug. D-papercut Diagnostics: An error or lint that needs small tweaks. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants