Use a distinct error code for "if may be missing an else clause" #36915
Conversation
Introduce the possibility of assigning distinct error codes to the various origin types of E0308. Start by assigning E0317 for the "IfExpressionWithNoElse" case, and write a long diagnostic specific to this case. Fixes rust-lang#36596
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| fn as_error_code(&self) -> &'static str { | ||
| match self { | ||
| // FIXME: use distinct codes for each case | ||
| &TypeOrigin::Misc(_) => "E0308", |
There was a problem hiding this comment.
Do not use strings for error codes, please. This way you circumvent the tidy checks for these error codes. It would probably make more sense to do a similar thing to rustc_resolve.
You can avold duplicating error code for each case by doing something like this:
match *self {
TypeOrigin::IfExpressionWithNoElse(_) => struct_span_err!(..., E0317, ...),
_ => struct_span_err!(..., E0308, ...)
}
| @@ -219,6 +219,25 @@ pub enum TypeOrigin { | |||
| } | |||
|
|
|||
| impl TypeOrigin { | |||
| fn as_error_code(&self) -> &'static str { | |||
| match self { | |||
There was a problem hiding this comment.
nit: *self here instead of &Pattern.
| ```compile_fail,E0317 | ||
| fn main() { | ||
| let x = 5; | ||
| let a = if x == 5 { 1 }; |
There was a problem hiding this comment.
This example can be made more relatable if written as such:
if condition {
some_value
}
Missing semicolon after last expression in an if is likely the most common reason for this error by a large degree.
There was a problem hiding this comment.
That example produces the generic "mismatched types" variant of E0308 though, not "if may be missing an else clause".
There was a problem hiding this comment.
Does it? That’s weird. I would expect else-less if as a last expression in a function body/block to produce the “if may be missing an else clause” error instead.
There was a problem hiding this comment.
It seems to be dependent on the expected type in the expression's context:
fn foo() -> i32 {
if true {
5
}
}
produces:
error[E0308]: if may be missing an else clause
--> <anon>:2:5
|
2 | if true {
| ^ expected (), found i32
|
However, change i32 to () and it's:
error[E0308]: mismatched types
--> <anon>:3:9
|
3 | 5
| ^ expected (), found integral variable
|
There was a problem hiding this comment.
This makes sense to me -- even if you added an else, given that the expected type of this expression is (), the body would have to be changed. That is, the body and the fn signature are in conflict. We choose to blame the body. You could make a case the other way, but blaming the if seems wrong, since there is nothing you can change about the if itself (without changing something else) that would make this code compile.
Not that the compiler thinks this hard when selecting what to blame; it's semi-random. Just saying that in this case the span makes sense. It'd be nice if we could give a better error message though, explaining where the () came from.
I think that Elm does some nice tracking here. We should copy them. =)
|
Any additional feedback or changes needed here? I have some work ready that builds on this but I'd like to get this PR merged first. |
| let span = trace.origin.span(); | ||
| let failure_str = trace.origin.as_failure_str(); | ||
| let mut diag = match trace.origin { | ||
| // FIXME: use distinct codes for each case |
There was a problem hiding this comment.
I'd say let's just remove this FIXME, but if not, reword it to be more tentative, like "FIXME: Do we want to have distinct codes for other cases?"
|
@jfirebaugh looks good, sorry for the delay. |
|
@jfirebaugh er, sorry, I meant "looks good apart from that nit." Think you can tweak that? If so, let me know and I'll r+ |
|
@nikomatsakis Agreed; removed the FIXME. |
|
@bors r+ |
|
📌 Commit d07602b has been approved by |
Use a distinct error code for "if may be missing an else clause" Introduce the possibility of assigning distinct error codes to the various origin types of E0308. Start by assigning E0317 for the "IfExpressionWithNoElse" case, and write a long diagnostic specific to this case. Fixes #36596
Introduce the possibility of assigning distinct error codes to the various origin types of E0308. Start by assigning E0317 for the "IfExpressionWithNoElse" case, and write a long diagnostic specific to this case.
Fixes #36596