Skip to content
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

Add diagnostics to "while loop" and "for loop" that note that it is always determined that it might iterate zero times. #126510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions compiler/rustc_hir_typeck/src/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ use crate::FnCtxt;
use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::Expr;
use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
use rustc_infer::infer::relate::RelateResult;
use rustc_infer::infer::{Coercion, DefineOpaqueTypes, InferOk, InferResult};
Expand Down Expand Up @@ -1702,10 +1704,10 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {

if let Some(expr) = expression {
if let hir::ExprKind::Loop(
_,
loop_blk,
_,
loop_src @ (hir::LoopSource::While | hir::LoopSource::ForLoop),
_,
loop_span,
) = expr.kind
{
let loop_type = if loop_src == hir::LoopSource::While {
Expand All @@ -1715,6 +1717,33 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
};

err.note(format!("{loop_type} evaluate to unit type `()`"));

struct RetExprChecker {
contains_ret_expr: bool,
}
impl<'v> Visitor<'v> for RetExprChecker {
fn visit_expr(&mut self, ex: &'v Expr<'v>) {
if let hir::ExprKind::Ret(_) = ex.kind {
self.contains_ret_expr = true;
return;
}
intravisit::walk_expr(self, ex);
}
}
let mut visitor = RetExprChecker { contains_ret_expr: false };
intravisit::walk_block(&mut visitor, loop_blk);
if visitor.contains_ret_expr {
let loop_header = if loop_src == hir::LoopSource::While {
"loop condition expression or pattern"
} else {
"iterator expression"
};
err.span_note(
loop_span,
format!("It is determined that this might iterate zero times, regardless of the {loop_header}.")
);
err.span_help(loop_span, "If you are assuming that it will iterate at least once, consider using a `loop` expression instead.");
}
}

fcx.emit_coerce_suggestions(
Expand Down
70 changes: 70 additions & 0 deletions tests/ui/coercion/coerce-loop-issue-122561.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,16 @@ LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/coerce-loop-issue-122561.rs:4:5
|
LL | for i in 0.. {
| ^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:4:5
|
LL | for i in 0.. {
| ^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand All @@ -116,6 +126,16 @@ LL | | }
| |_____^ expected `String`, found `()`
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/coerce-loop-issue-122561.rs:11:5
|
LL | for i in 0..5 {
| ^^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:11:5
|
LL | for i in 0..5 {
| ^^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand All @@ -134,6 +154,16 @@ LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/coerce-loop-issue-122561.rs:18:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:18:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand Down Expand Up @@ -168,6 +198,16 @@ LL | fn for_single_line() -> bool { for i in 0.. { return false; } }
| expected `bool` because of return type
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/coerce-loop-issue-122561.rs:33:32
|
LL | fn for_single_line() -> bool { for i in 0.. { return false; } }
| ^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:33:32
|
LL | fn for_single_line() -> bool { for i in 0.. { return false; } }
| ^^^^^^^^^^^^
help: consider returning a value here
|
LL | fn for_single_line() -> bool { for i in 0.. { return false; } /* `bool` value */ }
Expand All @@ -186,6 +226,16 @@ LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `while` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the loop condition expression or pattern.
--> $DIR/coerce-loop-issue-122561.rs:48:5
|
LL | while true {
| ^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:48:5
|
LL | while true {
| ^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand All @@ -206,6 +256,16 @@ LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `while` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the loop condition expression or pattern.
--> $DIR/coerce-loop-issue-122561.rs:57:5
|
LL | while i < 3 {
| ^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:57:5
|
LL | while i < 3 {
| ^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand All @@ -224,6 +284,16 @@ LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `while` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the loop condition expression or pattern.
--> $DIR/coerce-loop-issue-122561.rs:65:5
|
LL | while false {
| ^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/coerce-loop-issue-122561.rs:65:5
|
LL | while false {
| ^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/for-loop-while/break-while-condition.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ LL | | }
= note: expected type `!`
found unit type `()`
= note: `while` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the loop condition expression or pattern.
--> $DIR/break-while-condition.rs:24:13
|
LL | while false {
| ^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/break-while-condition.rs:24:13
|
LL | while false {
| ^^^^^^^^^^^
help: consider adding a diverging expression here
|
LL ~ }
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/typeck/irrefutable-while-let-pattern.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
fn main() {}

fn foo() -> bool {
while let x = 0 {
//~^ 4:5: 7:6: mismatched types [E0308]
return true;
}
}
31 changes: 31 additions & 0 deletions tests/ui/typeck/irrefutable-while-let-pattern.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
error[E0308]: mismatched types
--> $DIR/irrefutable-while-let-pattern.rs:4:5
|
LL | fn foo() -> bool {
| ---- expected `bool` because of return type
LL | / while let x = 0 {
LL | |
LL | | return true;
LL | | }
| |_____^ expected `bool`, found `()`
|
= note: `while` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the loop condition expression or pattern.
--> $DIR/irrefutable-while-let-pattern.rs:4:5
|
LL | while let x = 0 {
| ^^^^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/irrefutable-while-let-pattern.rs:4:5
|
LL | while let x = 0 {
| ^^^^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
LL + /* `bool` value */
|

error: aborting due to 1 previous error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A number of issues here.

  • Error messages don't start with an upper-case letter, and don't end with a '.'.
  • There is a lot of repetition here. The note and the following help both print the loop condition. The two could be merged. I am also not sure if the condition needs to be printed, given that the loop is printed above anyway.
  • "It is determined" is too wordy, something like "this loop might iterate zero times" would be better.

But there are bigger concerns, which I will discuss below.


For more information about this error, try `rustc --explain E0308`.
3 changes: 1 addition & 2 deletions tests/ui/typeck/issue-100285.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ fn foo(n: i32) -> i32 {
} else {
return 5;
}

} //~ HELP consider returning a value here
}
}

fn main() {}
14 changes: 12 additions & 2 deletions tests/ui/typeck/issue-100285.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,25 @@ LL | | if n < 0 {
LL | | return i;
LL | | } else if n < 10 {
... |
LL | |
LL | | }
LL | | }
| |_____^ expected `i32`, found `()`
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/issue-100285.rs:2:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/issue-100285.rs:2:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
LL ~ /* `i32` value */
LL + /* `i32` value */
|

error: aborting due to 1 previous error
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/typeck/issue-98982.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
fn foo() -> i32 {
for i in 0..0 { //~ ERROR mismatched types [E0308]
return i;
} //~ HELP consider returning a value here
}
}

fn main() {}
12 changes: 11 additions & 1 deletion tests/ui/typeck/issue-98982.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,20 @@ LL | | }
| |_____^ expected `i32`, found `()`
|
= note: `for` loops evaluate to unit type `()`
note: It is determined that this might iterate zero times, regardless of the iterator expression.
--> $DIR/issue-98982.rs:2:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: If you are assuming that it will iterate at least once, consider using a `loop` expression instead.
--> $DIR/issue-98982.rs:2:5
|
LL | for i in 0..0 {
| ^^^^^^^^^^^^^
help: consider returning a value here
|
LL ~ }
LL ~ /* `i32` value */
LL + /* `i32` value */
|

error: aborting due to 1 previous error
Expand Down
Loading