-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
E0060 and E0061 improvement #35983
E0060 and E0061 improvement #35983
Conversation
err.note(&format!("the following parameter type{} expected: {}", | ||
if expected_count == 1 {" was"} else {"s were"}, | ||
input_types.join(", "))); | ||
err.span_label(sp, &format!("the following parameter type{} expected: {}", |
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.
If we're going to make this a label, can we make it smaller? Maybe something like it simply:
"parameter type{} expected: {}"
Makes we wish we had a span_label for the definition site, so they could see where the requirements were coming from (if we don't have that already)
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.
What about my proposition of list? A type a line if more than one type and it makes a nice output, easy to read. What do you think about it?
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.
A list in the label or in a note? I think it would work better in the note. Do you have an heuristic in mind?
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.
Not really. I can try both and show the result in here if you want.
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.
Showing here sounds good
First test with list in the span message: ./x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/compile-fail/E0061.rs
error[E0061]: this function takes 2 parameters but 1 parameter was supplied
--> src/test/compile-fail/E0061.rs:14:5
|
14 | f(0);
| ^^^^ the following parameter types were expected:
* u16
* &str
error: aborting due to previous error Not very beautiful. |
And inside a note it gives: ./x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/compile-fail/E0061.rs
error[E0061]: this function takes 2 parameters but 1 parameter was supplied
--> src/test/compile-fail/E0061.rs:14:5
|
14 | f(0);
| ^^^^
|
= note: the following parameter types were expected:
* u16
* &str
error: aborting due to previous error Not very beautiful either... I think that the errors should keep the indent if there is a newline. If you want I can add it @jonathandturner. |
Maybe:
|
Another alternative (after chatting on IRC) might be:
I wish we could point at the function name in the definition, but iirc, that span isn't yet available. |
6b9d0cb
to
6716a60
Compare
Updated. |
Tidy failure:
|
6716a60
to
1eda14e
Compare
Updated. |
@@ -15,6 +15,6 @@ fn main() { | |||
}); | |||
//~^^ ERROR this function takes 2 parameters but 1 parameter was supplied | |||
//~| NOTE the following parameter types were expected | |||
//~| NOTE expected 2 parameters | |||
//~| NOTE _, _ |
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.
Sigh, this one got worse. I wish I'd fixed the type variable names also. Once they're fixed, this should look better
@bors r+ rollup |
📌 Commit 1eda14e has been approved by |
…handturner E0060 and E0061 improvement Fixes rust-lang#35290. r? @jonathandturner
This broke the non bonus part of #35214 Edit: moving the err.span_label out of the if chain fix it |
Fixes #35290.
r? @jonathandturner