Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit 1bc7da2

Browse files
taiki-etaiki-e
committed
Fix needless_continue false positive
1 parent 1cdac4a commit 1bc7da2

File tree

3 files changed

+136
-18
lines changed

3 files changed

+136
-18
lines changed

‎clippy_lints/src/needless_continue.rs

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -175,19 +175,19 @@ impl EarlyLintPass for NeedlessContinue {
175175
/// - The expression is a `continue` node.
176176
/// - The expression node is a block with the first statement being a
177177
/// `continue`.
178-
fn needless_continue_in_else(else_expr: &ast::Expr) -> bool {
178+
fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool {
179179
match else_expr.node {
180-
ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block),
181-
ast::ExprKind::Continue(_) => true,
180+
ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label),
181+
ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()),
182182
_ => false,
183183
}
184184
}
185185

186-
fn is_first_block_stmt_continue(block: &ast::Block) -> bool {
186+
fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> bool {
187187
block.stmts.get(0).map_or(false, |stmt| match stmt.node {
188188
ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => {
189-
if let ast::ExprKind::Continue(_) = e.node {
190-
true
189+
if let ast::ExprKind::Continue(ref l) = e.node {
190+
compare_labels(label, l.as_ref())
191191
} else {
192192
false
193193
}
@@ -196,17 +196,29 @@ fn is_first_block_stmt_continue(block: &ast::Block) -> bool {
196196
})
197197
}
198198

199+
/// If the `continue` has a label, check it matches the label of the loop.
200+
fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::Label>) -> bool {
201+
match (loop_label, continue_label) {
202+
// `loop { continue; }` or `'a loop { continue; }`
203+
(_, None) => true,
204+
// `loop { continue 'a; }`
205+
(None, _) => false,
206+
// `'a loop { continue 'a; }` or `'a loop { continue 'b; }`
207+
(Some(x), Some(y)) => x.ident == y.ident,
208+
}
209+
}
210+
199211
/// If `expr` is a loop expression (while/while let/for/loop), calls `func` with
200212
/// the AST object representing the loop block of `expr`.
201213
fn with_loop_block<F>(expr: &ast::Expr, mut func: F)
202214
where
203-
F: FnMut(&ast::Block),
215+
F: FnMut(&ast::Block, Option<&ast::Label>),
204216
{
205217
match expr.node {
206-
ast::ExprKind::While(_, ref loop_block, _)
207-
| ast::ExprKind::WhileLet(_, _, ref loop_block, _)
208-
| ast::ExprKind::ForLoop(_, _, ref loop_block, _)
209-
| ast::ExprKind::Loop(ref loop_block, _) => func(loop_block),
218+
ast::ExprKind::While(_, ref loop_block, ref label)
219+
| ast::ExprKind::WhileLet(_, _, ref loop_block, ref label)
220+
| ast::ExprKind::ForLoop(_, _, ref loop_block, ref label)
221+
| ast::ExprKind::Loop(ref loop_block, ref label) => func(loop_block, label.as_ref()),
210222
_ => {},
211223
}
212224
}
@@ -344,7 +356,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>(
344356
}
345357

346358
fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) {
347-
with_loop_block(expr, |loop_block| {
359+
with_loop_block(expr, |loop_block, label| {
348360
for (i, stmt) in loop_block.stmts.iter().enumerate() {
349361
with_if_expr(stmt, |if_expr, cond, then_block, else_expr| {
350362
let data = &LintData {
@@ -355,14 +367,14 @@ fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) {
355367
else_expr,
356368
block_stmts: &loop_block.stmts,
357369
};
358-
if needless_continue_in_else(else_expr) {
370+
if needless_continue_in_else(else_expr, label) {
359371
emit_warning(
360372
ctx,
361373
data,
362374
DROP_ELSE_BLOCK_AND_MERGE_MSG,
363375
LintType::ContinueInsideElseBlock,
364376
);
365-
} else if is_first_block_stmt_continue(then_block) {
377+
} else if is_first_block_stmt_continue(then_block, label) {
366378
emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock);
367379
}
368380
});

‎tests/ui/needless_continue.rs

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#![warn(clippy::needless_continue)]
2+
13
macro_rules! zero {
24
($x:expr) => {
35
$x == 0
@@ -10,7 +12,6 @@ macro_rules! nonzero {
1012
};
1113
}
1214

13-
#[warn(clippy::needless_continue)]
1415
fn main() {
1516
let mut i = 1;
1617
while i < 10 {
@@ -49,3 +50,66 @@ fn main() {
4950
println!("bleh");
5051
}
5152
}
53+
54+
mod issue_2329 {
55+
fn condition() -> bool {
56+
unimplemented!()
57+
}
58+
fn update_condition() {}
59+
60+
// only the outer loop has a label
61+
fn foo() {
62+
'outer: loop {
63+
println!("Entry");
64+
while condition() {
65+
update_condition();
66+
if condition() {
67+
println!("foo-1");
68+
} else {
69+
continue 'outer; // should not lint here
70+
}
71+
println!("foo-2");
72+
73+
update_condition();
74+
if condition() {
75+
continue 'outer; // should not lint here
76+
} else {
77+
println!("foo-3");
78+
}
79+
println!("foo-4");
80+
}
81+
}
82+
}
83+
84+
// both loops have labels
85+
fn bar() {
86+
'outer: loop {
87+
println!("Entry");
88+
'inner: while condition() {
89+
update_condition();
90+
if condition() {
91+
println!("bar-1");
92+
} else {
93+
continue 'outer; // should not lint here
94+
}
95+
println!("bar-2");
96+
97+
update_condition();
98+
if condition() {
99+
println!("bar-3");
100+
} else {
101+
continue 'inner; // should lint here
102+
}
103+
println!("bar-4");
104+
105+
update_condition();
106+
if condition() {
107+
continue; // should lint here
108+
} else {
109+
println!("bar-5");
110+
}
111+
println!("bar-6");
112+
}
113+
}
114+
}
115+
}

‎tests/ui/needless_continue.stderr

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
error: This else block is redundant.
22

3-
--> $DIR/needless_continue.rs:27:16
3+
--> $DIR/needless_continue.rs:28:16
44
|
55
LL | } else {
66
| ________________^
@@ -37,7 +37,7 @@ LL | | }
3737

3838
error: There is no need for an explicit `else` block for this `if` expression
3939

40-
--> $DIR/needless_continue.rs:42:9
40+
--> $DIR/needless_continue.rs:43:9
4141
|
4242
LL | / if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 {
4343
LL | | continue;
@@ -55,5 +55,47 @@ LL | | }
5555
println!("Jabber");
5656
...
5757

58-
error: aborting due to 2 previous errors
58+
error: This else block is redundant.
59+
60+
--> $DIR/needless_continue.rs:100:24
61+
|
62+
LL | } else {
63+
| ________________________^
64+
LL | | continue 'inner; // should lint here
65+
LL | | }
66+
| |_________________^
67+
|
68+
= help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so:
69+
if condition() {
70+
println!("bar-3");
71+
// Merged code follows...println!("bar-4");
72+
update_condition();
73+
if condition() {
74+
continue; // should lint here
75+
} else {
76+
println!("bar-5");
77+
}
78+
println!("bar-6");
79+
}
80+
81+
82+
error: There is no need for an explicit `else` block for this `if` expression
83+
84+
--> $DIR/needless_continue.rs:106:17
85+
|
86+
LL | / if condition() {
87+
LL | | continue; // should lint here
88+
LL | | } else {
89+
LL | | println!("bar-5");
90+
LL | | }
91+
| |_________________^
92+
|
93+
= help: Consider dropping the else clause, and moving out the code in the else block, like so:
94+
if condition() {
95+
continue;
96+
}
97+
println!("bar-5");
98+
...
99+
100+
error: aborting due to 4 previous errors
59101

0 commit comments

Comments
 (0)