Skip to content

Commit 54f88c3

Browse files
authored
[needless_continue]: lint if the last stmt in loop is continue recurisvely (#13891)
fixes: #4077 Continuation of #11546. r? @y21 if you don't mind? changelog: [`needless_continue`] lint if the last stmt in loop is `continue` recurisvely
2 parents 7a01033 + f18399f commit 54f88c3

7 files changed

+254
-37
lines changed

Diff for: clippy_lints/src/methods/unnecessary_to_owned.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty<
494494
for (_, node) in cx.tcx.hir().parent_iter(expr.hir_id) {
495495
match node {
496496
Node::Stmt(_) => return true,
497-
Node::Block(..) => continue,
497+
Node::Block(..) => {},
498498
Node::Item(item) => {
499499
if let ItemKind::Fn(_, _, body_id) = &item.kind
500500
&& let output_ty = return_ty(cx, item.owner_id)

Diff for: clippy_lints/src/needless_continue.rs

+114-23
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_and_help;
22
use clippy_utils::source::{indent_of, snippet, snippet_block};
3-
use rustc_ast::ast;
3+
use rustc_ast::{Block, Label, ast};
44
use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
55
use rustc_session::declare_lint_pass;
66
use rustc_span::Span;
@@ -11,6 +11,7 @@ declare_clippy_lint! {
1111
/// that contain a `continue` statement in either their main blocks or their
1212
/// `else`-blocks, when omitting the `else`-block possibly with some
1313
/// rearrangement of code can make the code easier to understand.
14+
/// The lint also checks if the last statement in the loop is a `continue`
1415
///
1516
/// ### Why is this bad?
1617
/// Having explicit `else` blocks for `if` statements
@@ -75,6 +76,49 @@ declare_clippy_lint! {
7576
/// # break;
7677
/// }
7778
/// ```
79+
///
80+
/// ```rust
81+
/// # use std::io::ErrorKind;
82+
///
83+
/// fn foo() -> ErrorKind { ErrorKind::NotFound }
84+
/// for _ in 0..10 {
85+
/// match foo() {
86+
/// ErrorKind::NotFound => {
87+
/// eprintln!("not found");
88+
/// continue
89+
/// }
90+
/// ErrorKind::TimedOut => {
91+
/// eprintln!("timeout");
92+
/// continue
93+
/// }
94+
/// _ => {
95+
/// eprintln!("other error");
96+
/// continue
97+
/// }
98+
/// }
99+
/// }
100+
/// ```
101+
/// Could be rewritten as
102+
///
103+
///
104+
/// ```rust
105+
/// # use std::io::ErrorKind;
106+
///
107+
/// fn foo() -> ErrorKind { ErrorKind::NotFound }
108+
/// for _ in 0..10 {
109+
/// match foo() {
110+
/// ErrorKind::NotFound => {
111+
/// eprintln!("not found");
112+
/// }
113+
/// ErrorKind::TimedOut => {
114+
/// eprintln!("timeout");
115+
/// }
116+
/// _ => {
117+
/// eprintln!("other error");
118+
/// }
119+
/// }
120+
/// }
121+
/// ```
78122
#[clippy::version = "pre 1.29.0"]
79123
pub NEEDLESS_CONTINUE,
80124
pedantic,
@@ -144,15 +188,15 @@ impl EarlyLintPass for NeedlessContinue {
144188
///
145189
/// - The expression is a `continue` node.
146190
/// - The expression node is a block with the first statement being a `continue`.
147-
fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool {
191+
fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&Label>) -> bool {
148192
match else_expr.kind {
149193
ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label),
150194
ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()),
151195
_ => false,
152196
}
153197
}
154198

155-
fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> bool {
199+
fn is_first_block_stmt_continue(block: &Block, label: Option<&Label>) -> bool {
156200
block.stmts.first().is_some_and(|stmt| match stmt.kind {
157201
ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => {
158202
if let ast::ExprKind::Continue(ref l) = e.kind {
@@ -166,7 +210,7 @@ fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>)
166210
}
167211

168212
/// If the `continue` has a label, check it matches the label of the loop.
169-
fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::Label>) -> bool {
213+
fn compare_labels(loop_label: Option<&Label>, continue_label: Option<&Label>) -> bool {
170214
match (loop_label, continue_label) {
171215
// `loop { continue; }` or `'a loop { continue; }`
172216
(_, None) => true,
@@ -181,7 +225,7 @@ fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::
181225
/// the AST object representing the loop block of `expr`.
182226
fn with_loop_block<F>(expr: &ast::Expr, mut func: F)
183227
where
184-
F: FnMut(&ast::Block, Option<&ast::Label>),
228+
F: FnMut(&Block, Option<&Label>),
185229
{
186230
if let ast::ExprKind::While(_, loop_block, label)
187231
| ast::ExprKind::ForLoop {
@@ -205,7 +249,7 @@ where
205249
/// - The `else` expression.
206250
fn with_if_expr<F>(stmt: &ast::Stmt, mut func: F)
207251
where
208-
F: FnMut(&ast::Expr, &ast::Expr, &ast::Block, &ast::Expr),
252+
F: FnMut(&ast::Expr, &ast::Expr, &Block, &ast::Expr),
209253
{
210254
match stmt.kind {
211255
ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => {
@@ -231,14 +275,14 @@ struct LintData<'a> {
231275
/// The condition expression for the above `if`.
232276
if_cond: &'a ast::Expr,
233277
/// The `then` block of the `if` statement.
234-
if_block: &'a ast::Block,
278+
if_block: &'a Block,
235279
/// The `else` block of the `if` statement.
236280
/// Note that we only work with `if` exprs that have an `else` branch.
237281
else_expr: &'a ast::Expr,
238282
/// The 0-based index of the `if` statement in the containing loop block.
239283
stmt_idx: usize,
240284
/// The statements of the loop block.
241-
loop_block: &'a ast::Block,
285+
loop_block: &'a Block,
242286
}
243287

244288
const MSG_REDUNDANT_CONTINUE_EXPRESSION: &str = "this `continue` expression is redundant";
@@ -329,24 +373,63 @@ fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &Lin
329373
)
330374
}
331375

332-
fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
333-
if let ast::ExprKind::Loop(loop_block, loop_label, ..) = &expr.kind
334-
&& let Some(last_stmt) = loop_block.stmts.last()
376+
fn check_last_stmt_in_expr<F>(inner_expr: &ast::Expr, func: &F)
377+
where
378+
F: Fn(Option<&Label>, Span),
379+
{
380+
match &inner_expr.kind {
381+
ast::ExprKind::Continue(continue_label) => {
382+
func(continue_label.as_ref(), inner_expr.span);
383+
},
384+
ast::ExprKind::If(_, then_block, else_block) => {
385+
check_last_stmt_in_block(then_block, func);
386+
if let Some(else_block) = else_block {
387+
check_last_stmt_in_expr(else_block, func);
388+
}
389+
},
390+
ast::ExprKind::Match(_, arms, _) => {
391+
for arm in arms {
392+
if let Some(expr) = &arm.body {
393+
check_last_stmt_in_expr(expr, func);
394+
}
395+
}
396+
},
397+
ast::ExprKind::Block(b, _) => {
398+
check_last_stmt_in_block(b, func);
399+
},
400+
_ => {},
401+
}
402+
}
403+
404+
fn check_last_stmt_in_block<F>(b: &Block, func: &F)
405+
where
406+
F: Fn(Option<&Label>, Span),
407+
{
408+
if let Some(last_stmt) = b.stmts.last()
335409
&& let ast::StmtKind::Expr(inner_expr) | ast::StmtKind::Semi(inner_expr) = &last_stmt.kind
336-
&& let ast::ExprKind::Continue(continue_label) = inner_expr.kind
337-
&& compare_labels(loop_label.as_ref(), continue_label.as_ref())
338410
{
339-
span_lint_and_help(
340-
cx,
341-
NEEDLESS_CONTINUE,
342-
last_stmt.span,
343-
MSG_REDUNDANT_CONTINUE_EXPRESSION,
344-
None,
345-
DROP_CONTINUE_EXPRESSION_MSG,
346-
);
411+
check_last_stmt_in_expr(inner_expr, func);
347412
}
413+
}
414+
415+
fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
348416
with_loop_block(expr, |loop_block, label| {
349-
for (i, stmt) in loop_block.stmts.iter().enumerate() {
417+
let p = |continue_label: Option<&Label>, span: Span| {
418+
if compare_labels(label, continue_label) {
419+
span_lint_and_help(
420+
cx,
421+
NEEDLESS_CONTINUE,
422+
span,
423+
MSG_REDUNDANT_CONTINUE_EXPRESSION,
424+
None,
425+
DROP_CONTINUE_EXPRESSION_MSG,
426+
);
427+
}
428+
};
429+
430+
let stmts = &loop_block.stmts;
431+
for (i, stmt) in stmts.iter().enumerate() {
432+
let mut maybe_emitted_in_if = false;
350433
with_if_expr(stmt, |if_expr, cond, then_block, else_expr| {
351434
let data = &LintData {
352435
if_expr,
@@ -356,6 +439,8 @@ fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
356439
stmt_idx: i,
357440
loop_block,
358441
};
442+
443+
maybe_emitted_in_if = true;
359444
if needless_continue_in_else(else_expr, label) {
360445
emit_warning(
361446
cx,
@@ -365,8 +450,14 @@ fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
365450
);
366451
} else if is_first_block_stmt_continue(then_block, label) {
367452
emit_warning(cx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock);
453+
} else {
454+
maybe_emitted_in_if = false;
368455
}
369456
});
457+
458+
if i == stmts.len() - 1 && !maybe_emitted_in_if {
459+
check_last_stmt_in_block(loop_block, &p);
460+
}
370461
}
371462
});
372463
}
@@ -400,7 +491,7 @@ fn erode_from_back(s: &str) -> String {
400491
if ret.is_empty() { s.to_string() } else { ret }
401492
}
402493

403-
fn span_of_first_expr_in_block(block: &ast::Block) -> Option<Span> {
494+
fn span_of_first_expr_in_block(block: &Block) -> Option<Span> {
404495
block.stmts.first().map(|stmt| stmt.span)
405496
}
406497

Diff for: clippy_lints/src/redundant_else.rs

-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ impl EarlyLintPass for RedundantElse {
6969
ExprKind::If(_, next_then, Some(next_els)) => {
7070
then = next_then;
7171
els = next_els;
72-
continue;
7372
},
7473
// else if without else
7574
ExprKind::If(..) => return,

Diff for: clippy_lints/src/transmute/transmute_undefined_repr.rs

-4
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,14 @@ pub(super) fn check<'tcx>(
3030
| (ReducedTy::UnorderedFields(from_sub_ty), ReducedTy::OrderedFields(Some(to_sub_ty))) => {
3131
from_ty = from_sub_ty;
3232
to_ty = to_sub_ty;
33-
continue;
3433
},
3534
(ReducedTy::OrderedFields(Some(from_sub_ty)), ReducedTy::Other(to_sub_ty)) if reduced_tys.to_fat_ptr => {
3635
from_ty = from_sub_ty;
3736
to_ty = to_sub_ty;
38-
continue;
3937
},
4038
(ReducedTy::Other(from_sub_ty), ReducedTy::OrderedFields(Some(to_sub_ty))) if reduced_tys.from_fat_ptr => {
4139
from_ty = from_sub_ty;
4240
to_ty = to_sub_ty;
43-
continue;
4441
},
4542

4643
// ptr <-> ptr
@@ -50,7 +47,6 @@ pub(super) fn check<'tcx>(
5047
{
5148
from_ty = from_sub_ty;
5249
to_ty = to_sub_ty;
53-
continue;
5450
},
5551

5652
// fat ptr <-> (*size, *size)

Diff for: tests/missing-test-files.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ fn explore_directory(dir: &Path) -> Vec<String> {
5959
missing_files.push(path.to_str().unwrap().to_string());
6060
}
6161
},
62-
_ => continue,
62+
_ => {},
6363
};
6464
}
6565
}

Diff for: tests/ui/needless_continue.rs

+65
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,14 @@ fn simple_loop4() {
8787
}
8888
}
8989

90+
fn simple_loop5() {
91+
loop {
92+
println!("bleh");
93+
{ continue }
94+
//~^ ERROR: this `continue` expression is redundant
95+
}
96+
}
97+
9098
mod issue_2329 {
9199
fn condition() -> bool {
92100
unimplemented!()
@@ -168,3 +176,60 @@ fn issue_13641() {
168176
}
169177
}
170178
}
179+
180+
mod issue_4077 {
181+
fn main() {
182+
'outer: loop {
183+
'inner: loop {
184+
do_something();
185+
if some_expr() {
186+
println!("bar-7");
187+
continue 'outer;
188+
} else if !some_expr() {
189+
println!("bar-8");
190+
continue 'inner;
191+
} else {
192+
println!("bar-9");
193+
continue 'inner;
194+
}
195+
}
196+
}
197+
198+
for _ in 0..10 {
199+
match "foo".parse::<i32>() {
200+
Ok(_) => do_something(),
201+
Err(_) => {
202+
println!("bar-10");
203+
continue;
204+
},
205+
}
206+
}
207+
208+
loop {
209+
if true {
210+
} else {
211+
// redundant `else`
212+
continue; // redundant `continue`
213+
}
214+
}
215+
216+
loop {
217+
if some_expr() {
218+
continue;
219+
} else {
220+
do_something();
221+
}
222+
}
223+
}
224+
225+
// The contents of these functions are irrelevant, the purpose of this file is
226+
// shown in main.
227+
228+
fn do_something() {
229+
std::process::exit(0);
230+
}
231+
232+
fn some_expr() -> bool {
233+
true
234+
}
235+
}

0 commit comments

Comments
 (0)