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

Argument type error improvements #100479

Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 13, 2022

Motivated by this interesting code snippet:

#[derive(Copy, Clone)]
struct Wrapper<T>(T);

fn foo(_: fn(i32), _: Wrapper<i32>) {}

fn f(_: u32) {}

fn main() {
    let w = Wrapper::<isize>(1isize);
    foo(f, w);
}

Which currently errors like:

error[E0308]: arguments to this function are incorrect
  --> src/main.rs:10:5
   |
10 |     foo(f, w);
   |     ^^^ -  - expected `i32`, found `isize`
   |         |
   |         expected `i32`, found `u32`
   |
   = note: expected fn pointer `fn(i32)`
                 found fn item `fn(u32) {f}`
   = note: expected struct `Wrapper<i32>`
              found struct `Wrapper<isize>`
note: function defined here
  --> src/main.rs:4:4
   |
4  | fn foo(_: fn(i32), _: Wrapper<i32>) {}
   |    ^^^ ----------  ---------------

Specifically, that double expected .. found .. which is very difficult to correlate to the types in the arguments. Also, the fact that "expected i32, found isize" and the other argument mismatch label don't even really explain what's going on here.

After this PR:

error[E0308]: arguments to this function are incorrect
  --> $DIR/two-mismatch-notes.rs:10:5
   |
LL |     foo(f, w);
   |     ^^^
   |
note: expected fn pointer, found fn item
  --> $DIR/two-mismatch-notes.rs:10:9
   |
LL |     foo(f, w);
   |         ^
   = note: expected fn pointer `fn(i32)`
                 found fn item `fn(u32) {f}`
note: expected struct `Wrapper`, found a different struct `Wrapper`
  --> $DIR/two-mismatch-notes.rs:10:12
   |
LL |     foo(f, w);
   |            ^
   = note: expected struct `Wrapper<i32>`
              found struct `Wrapper<isize>`
note: function defined here
  --> $DIR/two-mismatch-notes.rs:4:4
   |
LL | fn foo(_: fn(i32), _: Wrapper<i32>) {}
   |    ^^^ ----------  ---------------

error: aborting due to previous error

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

Yeah, it's a bit verbose, but much clearer IMO.


Open to discussions about how this could be further improved. Motivated by @jyn514's tweet here.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 13, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit, then r=me

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Aug 13, 2022

😍😍😍

This is a great minimization, thank you! Unfortunately I'm not actually sure I find the error message more clear. It no longer clearly calls out the smallest types that don't match. In this example it's reasonably clear, but if the types of the arguments more complicated, like in my original example, or if the changes in #100473 don't land, I think it will be harder to spot why the argument types are mismatched.

That said, I absolutely love that it now points to the specific argument, rather than the entire function call.

@jyn514
Copy link
Member

jyn514 commented Aug 13, 2022

I think what would be helpful is to keep the new structure, where there's a separate note and label for each mismatched argument, but to keep the original message for secondary label:

note: expected struct `Wrapper<isize>`, found struct `Wrapper<i32>`
  --> $DIR/two-mismatch-notes.rs:10:12
   |
LL |     foo(f, w);
   |            ^
   = note: expected i32
              found isize

@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 13, 2022

@jyn514 thanks for the feedback, I can probably just rebase out the commit that changes the mismatched types being reported

@compiler-errors
Copy link
Member Author

Now:

error[E0308]: arguments to this function are incorrect
  --> $DIR/two-mismatch-notes.rs:10:5
   |
LL |     foo(f, w);
   |     ^^^
   |
note: expected `i32`, found `u32`
  --> $DIR/two-mismatch-notes.rs:10:9
   |
LL |     foo(f, w);
   |         ^
   = note: expected fn pointer `fn(i32)`
                 found fn item `fn(u32) {f}`
note: expected `i32`, found `isize`
  --> $DIR/two-mismatch-notes.rs:10:12
   |
LL |     foo(f, w);
   |            ^
   = note: expected struct `Wrapper<i32>`
              found struct `Wrapper<isize>`
note: function defined here
  --> $DIR/two-mismatch-notes.rs:4:4
   |
LL | fn foo(_: fn(i32), _: Wrapper<i32>) {}
   |    ^^^ ----------  ---------------

@jyn514
Copy link
Member

jyn514 commented Aug 13, 2022

Amazing, thank you!! 😍

@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Aug 13, 2022

📌 Commit aa1a07f has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2022
@compiler-errors
Copy link
Member Author

@bors rollup this just changes diagnostics anyways

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2022
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#99646 (Only point out a single function parameter if we have a single arg incompatibility)
 - rust-lang#100299 (make `clean::Item::span` return `Option` instead of dummy span)
 - rust-lang#100335 (Rustdoc-Json: Add `Path` type for traits.)
 - rust-lang#100367 (Suggest the path separator when a dot is used on a trait)
 - rust-lang#100431 (Enum variant ctor inherits the stability of the enum variant)
 - rust-lang#100446 (Suggest removing a semicolon after impl/trait items)
 - rust-lang#100468 (Use an extensionless `x` script for non-Windows)
 - rust-lang#100479 (Argument type error improvements)

Failed merges:

 - rust-lang#100483 (Point to generic or arg if it's the self type of unsatisfied projection predicate)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b3e76aa into rust-lang:master Aug 14, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 14, 2022
@compiler-errors compiler-errors deleted the argument-type-error-improvements branch August 11, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants