Detect type mismatch due to loop that might never iterate #100094
Conversation
rustbot
added
the
T-compiler
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| @@ -1552,12 +1576,72 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> { | |||
| } | |||
|
|
|||
| err.emit_unless(unsized_return); | |||
| if ret_vec.len() > 0 { | |||
| if let Some(expr) = expression { | |||
| let mut addtion_err = fcx.tcx.sess.diagnostic().span_note_diag(expr.span, "the function expects a value to always be returned, but loops might run zero times"); | |||
There was a problem hiding this comment.
We shouldn't emit an additional note here -- instead, we should append this to err that is referenced above.
| if ret_vec.len() > 0 { | ||
| if let Some(expr) = expression { | ||
| let mut addtion_err = fcx.tcx.sess.diagnostic().span_note_diag(expr.span, "the function expects a value to always be returned, but loops might run zero times"); | ||
| for i in 0..ret_vec.len() { |
There was a problem hiding this comment.
This should be limited to something reasonable like 3, and if there are >3, then say something like "... and other returns" or something like that.
There was a problem hiding this comment.
what do you mean by this? Sounds like a new issue for me.
Can you give me more details that we should emit?
There was a problem hiding this comment.
He means something like:
note: the function expects a value to always be returned, but loops might run zero times
--> $DIR/issue-98982.rs:2:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^ this might have zero elements to iterate on
LL |
LL | return i;
| -------- if the loop doesn't execute, this value would never get returned
...
LL | return i;
| -------- if the loop doesn't execute, this value would never get returned
...
LL | return i;
| -------- if the loop doesn't execute, this value would never get returned
= note: if the loop doesn't execute, 5 other values would never get returned
There was a problem hiding this comment.
created #100285 and assigned to myself
| loop_span, | ||
| "this might have zero elements to iterate on".to_string(), | ||
| ); | ||
| addtion_err.help("return a value for the case when the loop has zero elements to iterate on, and consider changing the return type to account for that possibility"); |
There was a problem hiding this comment.
I think the original issue #98982 was asking to make this into a suggestion, i.e. have the compiler suggest wrapping the return values in Some(..) and then returning None
There was a problem hiding this comment.
I think we need better machinery for default values (damn! I just realized that we could opportunistically programatically get that from the Default impl of types!), I'm ok with a help for now.
| @@ -1481,6 +1481,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> { | |||
|
|
|||
| let mut err; | |||
| let mut unsized_return = false; | |||
| let mut ret_vec: Vec<hir::HirId> = Vec::new(); // collect return expression, record HirId to save memory | |||
There was a problem hiding this comment.
It's fine, hir::Expr should be cheap enough for this, and we are already emitting the error, the allocation will have to happen either way, we wouldn't be pessimizing the happy path.
| loop_span, | ||
| "this might have zero elements to iterate on".to_string(), | ||
| ); | ||
| addtion_err.help("return a value for the case when the loop has zero elements to iterate on, and consider changing the return type to account for that possibility"); |
There was a problem hiding this comment.
I think we need better machinery for default values (damn! I just realized that we could opportunistically programatically get that from the Default impl of types!), I'm ok with a help for now.
|
r? @estebank |
| "the function expects a value to always be returned, but loops might run zero times", | ||
| ); | ||
| err.help( | ||
| "return a value for the case when the loop has zero elements to iterate on, and \ |
There was a problem hiding this comment.
| "return a value for the case when the loop has zero elements to iterate on, and \ | |
| "return a value for the case when the loop has zero elements to iterate on, or \ |
estebank
changed the title
lyming2007
force-pushed
the
issue-98982
branch
2 times, most recently
from
a4f876d to
6c86640
Compare
when loop as tail expression for miss match type E0308 error, recursively get the return statement and add diagnostic information on it use rustc_hir::intravisit to collect the return expression modified: compiler/rustc_typeck/src/check/coercion.rs new file: src/test/ui/typeck/issue-98982.rs new file: src/test/ui/typeck/issue-98982.stderr
|
@bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#100094 (Detect type mismatch due to loop that might never iterate) - rust-lang#100132 (Use (actually) dummy place for let-else divergence) - rust-lang#100167 (Recover `require`, `include` instead of `use` in item) - rust-lang#100193 (Remove more Clean trait implementations) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…iters-2, r=estebank Remove suggestion about iteration count in coerce Fixes rust-lang#122561 The iteration count-centric suggestion was implemented in PR rust-lang#100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang#122679 (comment)) This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value. It should also fix rust-lang#100285 by simply making it redundant.
…iters-2, r=estebank Remove suggestion about iteration count in coerce Fixes rust-lang#122561 The iteration count-centric suggestion was implemented in PR rust-lang#100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang#122679 (comment)) This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value. It should also fix rust-lang#100285 by simply making it redundant.
When loop as tail expression causes a miss match type E0308 error, recursively get the return statement and add diagnostic information on it.