From d63aeceaa1f4f175ad63456e691395912460ef41 Mon Sep 17 00:00:00 2001
From: Yotam Ofek <yotam.ofek@gmail.com>
Date: Mon, 19 Sep 2022 10:02:17 +0000
Subject: [PATCH] [`never_loop`]: Fix FP with let..else statements.
---
clippy_lints/src/loops/never_loop.rs | 31 +++++++++++++++++---------
tests/ui/never_loop.rs | 27 +++++++++++++++++++++++
tests/ui/never_loop.stderr | 33 +++++++++++++++++++---------
3 files changed, 71 insertions(+), 20 deletions(-)
diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
index 116e589cad6..2386f4f4f4e 100644
--- a/clippy_lints/src/loops/never_loop.rs
+++ b/clippy_lints/src/loops/never_loop.rs
@@ -42,6 +42,7 @@ pub(super) fn check(
}
}
+#[derive(Copy, Clone)]
enum NeverLoopResult {
// A break/return always get triggered but not necessarily for the main loop.
AlwaysBreak,
@@ -51,8 +52,8 @@ enum NeverLoopResult {
}
#[must_use]
-fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
- match *arg {
+fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
+ match arg {
NeverLoopResult::AlwaysBreak | NeverLoopResult::Otherwise => NeverLoopResult::Otherwise,
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
}
@@ -92,19 +93,29 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
}
fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult {
- let mut iter = block.stmts.iter().filter_map(stmt_to_expr).chain(block.expr);
+ let mut iter = block
+ .stmts
+ .iter()
+ .filter_map(stmt_to_expr)
+ .chain(block.expr.map(|expr| (expr, None)));
never_loop_expr_seq(&mut iter, main_loop_id)
}
-fn never_loop_expr_seq<'a, T: Iterator<Item = &'a Expr<'a>>>(es: &mut T, main_loop_id: HirId) -> NeverLoopResult {
- es.map(|e| never_loop_expr(e, main_loop_id))
- .fold(NeverLoopResult::Otherwise, combine_seq)
+fn never_loop_expr_seq<'a, T: Iterator<Item = (&'a Expr<'a>, Option<&'a Block<'a>>)>>(
+ es: &mut T,
+ main_loop_id: HirId,
+) -> NeverLoopResult {
+ es.map(|(e, els)| {
+ let e = never_loop_expr(e, main_loop_id);
+ els.map_or(e, |els| combine_branches(e, never_loop_block(els, main_loop_id)))
+ })
+ .fold(NeverLoopResult::Otherwise, combine_seq)
}
-fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'tcx Block<'tcx>>)> {
match stmt.kind {
- StmtKind::Semi(e, ..) | StmtKind::Expr(e, ..) => Some(e),
- StmtKind::Local(local) => local.init,
+ StmtKind::Semi(e, ..) | StmtKind::Expr(e, ..) => Some((e, None)),
+ StmtKind::Local(local) => local.init.map(|init| (init, local.els)),
StmtKind::Item(..) => None,
}
}
@@ -139,7 +150,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), main_loop_id),
ExprKind::Loop(b, _, _, _) => {
// Break can come from the inner loop so remove them.
- absorb_break(&never_loop_block(b, main_loop_id))
+ absorb_break(never_loop_block(b, main_loop_id))
},
ExprKind::If(e, e2, e3) => {
let e1 = never_loop_expr(e, main_loop_id);
diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
index 0a21589dd0d..fc60b440766 100644
--- a/tests/ui/never_loop.rs
+++ b/tests/ui/never_loop.rs
@@ -1,3 +1,4 @@
+#![feature(let_else)]
#![allow(
clippy::single_match,
unused_assignments,
@@ -203,6 +204,32 @@ pub fn test17() {
};
}
+// Issue #9356: `continue` in else branch of let..else
+pub fn test18() {
+ let x = Some(0);
+ let y = 0;
+ // might loop
+ let _ = loop {
+ let Some(x) = x else {
+ if y > 0 {
+ continue;
+ } else {
+ return;
+ }
+ };
+
+ break x;
+ };
+ // never loops
+ let _ = loop {
+ let Some(x) = x else {
+ return;
+ };
+
+ break x;
+ };
+}
+
fn main() {
test1();
test2();
diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
index f49b23924ef..1709e7512eb 100644
--- a/tests/ui/never_loop.stderr
+++ b/tests/ui/never_loop.stderr
@@ -1,5 +1,5 @@
error: this loop never actually loops
- --> $DIR/never_loop.rs:10:5
+ --> $DIR/never_loop.rs:11:5
|
LL | / loop {
LL | | // clippy::never_loop
@@ -13,7 +13,7 @@ LL | | }
= note: `#[deny(clippy::never_loop)]` on by default
error: this loop never actually loops
- --> $DIR/never_loop.rs:32:5
+ --> $DIR/never_loop.rs:33:5
|
LL | / loop {
LL | | // never loops
@@ -23,7 +23,7 @@ LL | | }
| |_____^
error: this loop never actually loops
- --> $DIR/never_loop.rs:52:5
+ --> $DIR/never_loop.rs:53:5
|
LL | / loop {
LL | | // never loops
@@ -35,7 +35,7 @@ LL | | }
| |_____^
error: this loop never actually loops
- --> $DIR/never_loop.rs:54:9
+ --> $DIR/never_loop.rs:55:9
|
LL | / while i == 0 {
LL | | // never loops
@@ -44,7 +44,7 @@ LL | | }
| |_________^
error: this loop never actually loops
- --> $DIR/never_loop.rs:66:9
+ --> $DIR/never_loop.rs:67:9
|
LL | / loop {
LL | | // never loops
@@ -56,7 +56,7 @@ LL | | }
| |_________^
error: this loop never actually loops
- --> $DIR/never_loop.rs:102:5
+ --> $DIR/never_loop.rs:103:5
|
LL | / while let Some(y) = x {
LL | | // never loops
@@ -65,7 +65,7 @@ LL | | }
| |_____^
error: this loop never actually loops
- --> $DIR/never_loop.rs:109:5
+ --> $DIR/never_loop.rs:110:5
|
LL | / for x in 0..10 {
LL | | // never loops
@@ -82,7 +82,7 @@ LL | if let Some(x) = (0..10).next() {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: this loop never actually loops
- --> $DIR/never_loop.rs:157:5
+ --> $DIR/never_loop.rs:158:5
|
LL | / 'outer: while a {
LL | | // never loops
@@ -94,12 +94,25 @@ LL | | }
| |_____^
error: this loop never actually loops
- --> $DIR/never_loop.rs:172:9
+ --> $DIR/never_loop.rs:173:9
|
LL | / while false {
LL | | break 'label;
LL | | }
| |_________^
-error: aborting due to 9 previous errors
+error: this loop never actually loops
+ --> $DIR/never_loop.rs:224:13
+ |
+LL | let _ = loop {
+ | _____________^
+LL | | let Some(x) = x else {
+LL | | return;
+LL | | };
+LL | |
+LL | | break x;
+LL | | };
+ | |_____^
+
+error: aborting due to 10 previous errors