-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Argument type error improvements #100479
Conversation
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this 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
😍😍😍 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. |
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:
|
@jyn514 thanks for the feedback, I can probably just rebase out the commit that changes the mismatched types being reported |
6acb1dd
to
aa1a07f
Compare
Now:
|
Amazing, thank you!! 😍 |
@bors r=lcnr |
@bors rollup this just changes diagnostics anyways |
…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
Motivated by this interesting code snippet:
Which currently errors like:
Specifically, that double
expected .. found ..
which is very difficult to correlate to the types in the arguments. Also, the fact that "expectedi32
, foundisize
" and the other argument mismatch label don't even really explain what's going on here.After this PR:
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.