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 refers to incorrect trait method as "fn pointer" #80929

Closed
euclio opened this issue Jan 12, 2021 · 7 comments · Fixed by #106131
Closed

Diagnostic refers to incorrect trait method as "fn pointer" #80929

euclio opened this issue Jan 12, 2021 · 7 comments · Fixed by #106131
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@euclio
Copy link
Contributor

euclio commented Jan 12, 2021

I tried this code (playground):

struct Foo;

trait Trait {
    fn foo(&mut self);
}

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

I expected to see this happen: Diagnostic mentions an expected method type and a provided method type.

Instead, this happened: Diagnostic mentions that it expected a "fn pointer" of a different type:

error[E0053]: method `foo` has an incompatible type for trait
 --> src/lib.rs:8:12
  |
4 |     fn foo(&mut self);
  |            --------- type in trait
...
8 |     fn foo(&self) {}
  |            ^^^^^ types differ in mutability
  |
  = note: expected fn pointer `fn(&mut Foo)`
             found fn pointer `fn(&Foo)`
help: consider change the type to match the mutability in trait
  |
8 |     fn foo(&mut self) {}
  |            ^^^^^^^^^

This was pretty confusing to me, because there are no function pointers in the code. It appears to be an implementation detail leaking through.

(There's also a typo in the help note which I plan to fix in a separate PR)

cc #66389

Meta

rustc --version --verbose:

1.51.0-nightly

(2021-01-10 c97f11af7bc4a6d3578f)
@euclio euclio added the C-bug Category: This is a bug. label Jan 12, 2021
@euclio euclio changed the title Diagnostic refers to Iincorrect trait method as "fn pointer" Diagnostic refers to incorrect trait method as "fn pointer" Jan 12, 2021
@camelid
Copy link
Member

camelid commented Jan 12, 2021

Seems like we should just skip saying "fn pointer". Something related that's confusing:

struct Foo {
    bar: fn() -> i32,
}

fn bar() {}

fn main() {
    Foo { bar };
}
error[E0308]: mismatched types
 --> src/main.rs:8:11
  |
8 |     Foo { bar };
  |           ^^^ expected `i32`, found `()`
  |
  = note: expected fn pointer `fn() -> i32`
                found fn item `fn() {bar}`

It makes it sound like they are different in ways other than their return type, when in fact making bar() return i32 fixes the problem.

@camelid camelid added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Jan 12, 2021
@JohnTitor JohnTitor added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2021
@zackmdavis
Copy link
Member

there are no function pointers in the code

I think the issue is that &self is a function pointer?—though we don't usually think of it that way.

@camelid
Copy link
Member

camelid commented Jan 16, 2021

It is? Even if it is, I don't think that's what the diagnostic is saying:

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

IIUC, it's saying that it expected a function that takes &mut self rather than &self.

@euclio
Copy link
Contributor Author

euclio commented Jan 16, 2021

I think the label does a good job of pointing out the actual problem, but I would expect the note to say something like

note: expected method signature `(&mut Foo)`
         found method signature `(&Foo)`

@zackmdavis
Copy link
Member

The "fn pointer" language is at

ty::FnPtr(_) => "fn pointer".into(),

but I'm not immediately sure how we'd inspect it to determine whether to say "method" or "function". (The inner value is a PolyFnSig, but I don't see how I can get the info we want out of its fields.)

@camelid
Copy link
Member

camelid commented Jan 17, 2021

I think saying

note: expected function `(&mut Foo)`
         found function `(&Foo)`

would be fine.

@zackmdavis zackmdavis self-assigned this Feb 20, 2021
zackmdavis added a commit to zackmdavis/rust that referenced this issue Feb 21, 2021
@zackmdavis zackmdavis removed their assignment Feb 21, 2021
@zackmdavis
Copy link
Member

zackmdavis commented Feb 21, 2021

I think my previous comment was wrong. As part of a Rust Awesome Mentors meeting, @MoreTacos and I tried that (changing the text to "function"), and overall, it made the error messages worse overall: a lot of inline errors in the UI tests became less clear when they said (e.g.) "expected function, found closure" as opposed to "expected fn pointer, found closure", and the change (surprisingly) didn't even affect the expected/found note that we were trying to change! It looks like the expected/found note is being generated in rustc_infer/src/infer/error_reporting/mod.rs:note_type_err, but it's not immediately obvious where it's getting the "fn pointer" language from!

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-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants