Print missing semicolon warning for type-inferred ints #29396
Conversation
We only printed the extra semicolon warning if the type of the last
statement was the same as the return type of the function.
If a semicolon was appended, then the code
fn foo() -> u16 {
1234;
}
Would not print the extra semicolon warning because without knowing
that 1234 is the return value, the inference engine would take 1234 to
be an i32.
This adds extra logic to recognize this case.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
|
| // inform the user of a semicolon in this case. | ||
| (&ty::TyInt(ast::TyI32),&ty::TyInt(..)) => true, | ||
| (&ty::TyInt(ast::TyI32),&ty::TyUint(..)) => true, | ||
| (a,b) => (a == b), |
|
@sanxiyn fixed. |
| // to be evaluated as i32 values. We still want to | ||
| // inform the user of a semicolon in this case. | ||
| (&ty::TyInt(ast::TyI32), &ty::TyInt(..)) => true, | ||
| (&ty::TyInt(ast::TyI32), &ty::TyUint(..)) => true, |
There was a problem hiding this comment.
Won’t… this make case like
fn foo() -> u16 {
1234i32;
}also print the note, even though i32 type is clearly intended?
There was a problem hiding this comment.
I didn't think about that. At the point in time liveliness is run, we don't have any other means of checking for a type-inferred int. Is this false positive worth it?
There was a problem hiding this comment.
Well, this makes a compiler suggestion invalid in some cases, which is worse than suggesting nothing IMO. An idea that has been floating in my head is to try and try to reinfer the last expression with expected return value to see how it goes. Not sure if possible at this stage, though.
|
@bors r- |
|
It's not worth the false-positive for such a niche diagnostic. |
We only printed the extra semicolon warning if the type of the last
statement was the same as the return type of the function.
If a semicolon was appended, then the code
Would not print the extra semicolon warning, because without knowing
that
1234is the return value, the inference engine would take1234tobe an
i32.This adds extra logic to recognize this case.
Fixes issue #29307.