-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Suggest Ok(())
when encountering Result::<(), E>::Ok()
#44059
Conversation
(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.
Looks nice. I left a few thoughts -- I think we could use to iterate once or twice on the exact message.
src/librustc_typeck/check/mod.rs
Outdated
sugg_span.hi = sugg_span.lo; | ||
err.span_suggestion( | ||
sugg_span, | ||
"expected one unit argument. try creating a new unit value", |
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.
this is a cute trick; is unit
the terminology we are using here? somehow this message feels a bit "jargon-y" to me, but I'm not sure how to say it better.
cc @rust-lang/docs
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.
Maybe word it like E0178?
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.
I confess, this message was also designed to trigger the 10 word limit for suggestions, otherwise you'd get "try creating a new unit value: ()
", which really doesn't help the target audience.
I will reword it in a shorter manner and add a way to exempt this suggestion from being shortened to a label
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.
How about?
help: expected the unit value `()`; try to insert one more pair of parenthesis?
|
15 | let _: Result<(), String> = Ok(());
| ^^
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.
I like this one better, but the ;
looks out of place.
maybe like this?
help: expected a unit value. Try to insert one more pair of parenthesis?
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.
I updated the message
src/librustc_typeck/check/mod.rs
Outdated
@@ -2423,7 +2423,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
|
|||
fn parameter_count_error<'tcx>(sess: &Session, sp: Span, expected_count: usize, | |||
arg_count: usize, error_code: &str, variadic: bool, | |||
def_span: Option<Span>) { | |||
def_span: Option<Span>, sugg: Option<String>) { |
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.
Nit: can you add a comment to this method and explain what some of the arguments are? Or perhaps just give this a clearer name, like suggest_unit_argument
? (In that case, why not make it a bool?)
Alternatively, we could make this more general, but right now the added labels only make sense with ()
.
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.
done
| ---------------- defined here | ||
... | ||
18 | bar(); | ||
| ^^^^^ expected 1 parameter |
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.
Shouldn't this be suggesting as well?
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.
Yea. I haven't figured out yet what's going on...
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.
fixed
Seems good to me now. @bors r+ |
📌 Commit 0b72497 has been approved by |
sugg_span.hi = sugg_span.lo; | ||
err.span_suggestion( | ||
sugg_span, | ||
"expected the unit value `()`. You can create one with a pair of parenthesis", |
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.
pair of parentheses, plural
Suggest `Ok(())` when encountering `Result::<(), E>::Ok()`
☀️ Test successful - status-appveyor, status-travis |
No description provided.