Suggest Ok(()) when encountering Result::<(), E>::Ok()
#44059
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
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.
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.
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.
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.
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.
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.
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 ().
| | ---------------- defined here | ||
| ... | ||
| 18 | bar(); | ||
| | ^^^^^ expected 1 parameter |
There was a problem hiding this comment.
Shouldn't this be suggesting as well?
There was a problem hiding this comment.
Yea. I haven't figured out yet what's going on...
aidanhs
added
the
S-waiting-on-author
|
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.
pair of parentheses, plural
Suggest `Ok(())` when encountering `Result::<(), E>::Ok()`
|
☀️ Test successful - status-appveyor, status-travis |
No description provided.