From e582fcd75dda451c68c266d91d538f010f4a9b9f Mon Sep 17 00:00:00 2001 From: lengyijun Date: Wed, 20 Sep 2023 21:05:36 +0800 Subject: [PATCH] [`needless_continue`]: lint if the last stmt in for/while/loop is `continue`, recursively fixes: #4077 --- .../src/methods/unnecessary_to_owned.rs | 2 +- clippy_lints/src/needless_continue.rs | 122 ++++++++++++++---- clippy_lints/src/redundant_else.rs | 1 - .../src/transmute/transmute_undefined_repr.rs | 4 - tests/missing-test-files.rs | 2 +- tests/ui/needless_continue.rs | 39 ++++++ tests/ui/needless_continue.stderr | 38 +++++- 7 files changed, 172 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index d19064fd57e3..a52851c9a1ab 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -494,7 +494,7 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty< for (_, node) in cx.tcx.hir().parent_iter(expr.hir_id) { match node { Node::Stmt(_) => return true, - Node::Block(..) => continue, + Node::Block(..) => {}, Node::Item(item) => { if let ItemKind::Fn(_, _, body_id) = &item.kind && let output_ty = return_ty(cx, item.owner_id) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index a7cd29bb8e32..dd75e0f4e4bf 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::source::{indent_of, snippet, snippet_block}; -use rustc_ast::ast; +use rustc_ast::{Block, Label, ast}; use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; use rustc_session::declare_lint_pass; use rustc_span::Span; @@ -11,6 +11,7 @@ declare_clippy_lint! { /// that contain a `continue` statement in either their main blocks or their /// `else`-blocks, when omitting the `else`-block possibly with some /// rearrangement of code can make the code easier to understand. + /// The lint also checks if the last statement in the loop is a `continue` /// /// ### Why is this bad? /// Having explicit `else` blocks for `if` statements @@ -75,6 +76,45 @@ declare_clippy_lint! { /// # break; /// } /// ``` + /// + /// ```rust + /// fn foo() -> std::io::ErrorKind { std::io::ErrorKind::NotFound } + /// for _ in 0..10 { + /// match foo() { + /// std::io::ErrorKind::NotFound => { + /// eprintln!("not found"); + /// continue + /// } + /// std::io::ErrorKind::TimedOut => { + /// eprintln!("timeout"); + /// continue + /// } + /// _ => { + /// eprintln!("other error"); + /// continue + /// } + /// } + /// } + /// ``` + /// Could be rewritten as + /// + /// + /// ```rust + /// fn foo() -> std::io::ErrorKind { std::io::ErrorKind::NotFound } + /// for _ in 0..10 { + /// match foo() { + /// std::io::ErrorKind::NotFound => { + /// eprintln!("not found"); + /// } + /// std::io::ErrorKind::TimedOut => { + /// eprintln!("timeout"); + /// } + /// _ => { + /// eprintln!("other error"); + /// } + /// } + /// } + /// ``` #[clippy::version = "pre 1.29.0"] pub NEEDLESS_CONTINUE, pedantic, @@ -144,7 +184,7 @@ impl EarlyLintPass for NeedlessContinue { /// /// - The expression is a `continue` node. /// - The expression node is a block with the first statement being a `continue`. -fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool { +fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&Label>) -> bool { match else_expr.kind { ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label), ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()), @@ -152,7 +192,7 @@ fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) } } -fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> bool { +fn is_first_block_stmt_continue(block: &Block, label: Option<&Label>) -> bool { block.stmts.first().is_some_and(|stmt| match stmt.kind { ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => { if let ast::ExprKind::Continue(ref l) = e.kind { @@ -166,7 +206,7 @@ fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) } /// If the `continue` has a label, check it matches the label of the loop. -fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::Label>) -> bool { +fn compare_labels(loop_label: Option<&Label>, continue_label: Option<&Label>) -> bool { match (loop_label, continue_label) { // `loop { continue; }` or `'a loop { continue; }` (_, None) => true, @@ -181,7 +221,7 @@ fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast:: /// the AST object representing the loop block of `expr`. fn with_loop_block(expr: &ast::Expr, mut func: F) where - F: FnMut(&ast::Block, Option<&ast::Label>), + F: FnMut(&Block, Option<&Label>), { if let ast::ExprKind::While(_, loop_block, label) | ast::ExprKind::ForLoop { @@ -205,7 +245,7 @@ where /// - The `else` expression. fn with_if_expr(stmt: &ast::Stmt, mut func: F) where - F: FnMut(&ast::Expr, &ast::Expr, &ast::Block, &ast::Expr), + F: FnMut(&ast::Expr, &ast::Expr, &Block, &ast::Expr), { match stmt.kind { ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => { @@ -231,14 +271,14 @@ struct LintData<'a> { /// The condition expression for the above `if`. if_cond: &'a ast::Expr, /// The `then` block of the `if` statement. - if_block: &'a ast::Block, + if_block: &'a Block, /// The `else` block of the `if` statement. /// Note that we only work with `if` exprs that have an `else` branch. else_expr: &'a ast::Expr, /// The 0-based index of the `if` statement in the containing loop block. stmt_idx: usize, /// The statements of the loop block. - loop_block: &'a ast::Block, + loop_block: &'a Block, } const MSG_REDUNDANT_CONTINUE_EXPRESSION: &str = "this `continue` expression is redundant"; @@ -329,23 +369,61 @@ fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &Lin ) } -fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) { - if let ast::ExprKind::Loop(loop_block, loop_label, ..) = &expr.kind - && let Some(last_stmt) = loop_block.stmts.last() +fn check_last_stmt_in_expr(inner_expr: &ast::Expr, func: &F) +where + F: Fn(Option<&Label>, Span), +{ + match &inner_expr.kind { + ast::ExprKind::Continue(continue_label) => { + func(continue_label.as_ref(), inner_expr.span); + }, + ast::ExprKind::If(_, then_block, else_block) => { + check_last_stmt_in_block(then_block, func); + if let Some(else_block) = else_block { + check_last_stmt_in_expr(else_block, func); + } + }, + ast::ExprKind::Match(_, arms, _) => { + for arm in arms { + if let Some(expr) = &arm.body { + check_last_stmt_in_expr(expr, func); + } + } + }, + ast::ExprKind::Block(b, _) => { + check_last_stmt_in_block(b, func); + }, + _ => {}, + } +} + +fn check_last_stmt_in_block(b: &Block, func: &F) +where + F: Fn(Option<&Label>, Span), +{ + if let Some(last_stmt) = b.stmts.last() && let ast::StmtKind::Expr(inner_expr) | ast::StmtKind::Semi(inner_expr) = &last_stmt.kind - && let ast::ExprKind::Continue(continue_label) = inner_expr.kind - && compare_labels(loop_label.as_ref(), continue_label.as_ref()) { - span_lint_and_help( - cx, - NEEDLESS_CONTINUE, - last_stmt.span, - MSG_REDUNDANT_CONTINUE_EXPRESSION, - None, - DROP_CONTINUE_EXPRESSION_MSG, - ); + check_last_stmt_in_expr(inner_expr, func); } +} + +fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) { with_loop_block(expr, |loop_block, label| { + let p = |continue_label: Option<&Label>, span: Span| { + if compare_labels(label, continue_label) { + span_lint_and_help( + cx, + NEEDLESS_CONTINUE, + span, + MSG_REDUNDANT_CONTINUE_EXPRESSION, + None, + DROP_CONTINUE_EXPRESSION_MSG, + ); + } + }; + check_last_stmt_in_block(loop_block, &p); + for (i, stmt) in loop_block.stmts.iter().enumerate() { with_if_expr(stmt, |if_expr, cond, then_block, else_expr| { let data = &LintData { @@ -400,7 +478,7 @@ fn erode_from_back(s: &str) -> String { if ret.is_empty() { s.to_string() } else { ret } } -fn span_of_first_expr_in_block(block: &ast::Block) -> Option { +fn span_of_first_expr_in_block(block: &Block) -> Option { block.stmts.first().map(|stmt| stmt.span) } diff --git a/clippy_lints/src/redundant_else.rs b/clippy_lints/src/redundant_else.rs index 6a1d40334e75..a27f9b631143 100644 --- a/clippy_lints/src/redundant_else.rs +++ b/clippy_lints/src/redundant_else.rs @@ -69,7 +69,6 @@ impl EarlyLintPass for RedundantElse { ExprKind::If(_, next_then, Some(next_els)) => { then = next_then; els = next_els; - continue; }, // else if without else ExprKind::If(..) => return, diff --git a/clippy_lints/src/transmute/transmute_undefined_repr.rs b/clippy_lints/src/transmute/transmute_undefined_repr.rs index 48d65eb15d9a..26323af31228 100644 --- a/clippy_lints/src/transmute/transmute_undefined_repr.rs +++ b/clippy_lints/src/transmute/transmute_undefined_repr.rs @@ -30,17 +30,14 @@ pub(super) fn check<'tcx>( | (ReducedTy::UnorderedFields(from_sub_ty), ReducedTy::OrderedFields(Some(to_sub_ty))) => { from_ty = from_sub_ty; to_ty = to_sub_ty; - continue; }, (ReducedTy::OrderedFields(Some(from_sub_ty)), ReducedTy::Other(to_sub_ty)) if reduced_tys.to_fat_ptr => { from_ty = from_sub_ty; to_ty = to_sub_ty; - continue; }, (ReducedTy::Other(from_sub_ty), ReducedTy::OrderedFields(Some(to_sub_ty))) if reduced_tys.from_fat_ptr => { from_ty = from_sub_ty; to_ty = to_sub_ty; - continue; }, // ptr <-> ptr @@ -50,7 +47,6 @@ pub(super) fn check<'tcx>( { from_ty = from_sub_ty; to_ty = to_sub_ty; - continue; }, // fat ptr <-> (*size, *size) diff --git a/tests/missing-test-files.rs b/tests/missing-test-files.rs index a8225d037e82..64eba5e0888a 100644 --- a/tests/missing-test-files.rs +++ b/tests/missing-test-files.rs @@ -59,7 +59,7 @@ fn explore_directory(dir: &Path) -> Vec { missing_files.push(path.to_str().unwrap().to_string()); } }, - _ => continue, + _ => {}, }; } } diff --git a/tests/ui/needless_continue.rs b/tests/ui/needless_continue.rs index b6d8a8f61aeb..bc7233f3ba50 100644 --- a/tests/ui/needless_continue.rs +++ b/tests/ui/needless_continue.rs @@ -87,6 +87,14 @@ fn simple_loop4() { } } +fn simple_loop5() { + loop { + println!("bleh"); + { continue } + //~^ ERROR: this `continue` expression is redundant + } +} + mod issue_2329 { fn condition() -> bool { unimplemented!() @@ -168,3 +176,34 @@ fn issue_13641() { } } } + +mod issue_4077 { + fn main() { + 'outer: loop { + 'inner: loop { + do_something(); + if some_expr() { + println!("bar-7"); + continue 'outer; + } else if !some_expr() { + println!("bar-8"); + continue 'inner; + } else { + println!("bar-9"); + continue 'inner; + } + } + } + } + + // The contents of these functions are irrelevant, the purpose of this file is + // shown in main. + + fn do_something() { + std::process::exit(0); + } + + fn some_expr() -> bool { + true + } +} diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr index 0741ba692487..4ab443108b0f 100644 --- a/tests/ui/needless_continue.stderr +++ b/tests/ui/needless_continue.stderr @@ -63,7 +63,7 @@ error: this `continue` expression is redundant --> tests/ui/needless_continue.rs:60:9 | LL | continue; - | ^^^^^^^^^ + | ^^^^^^^^ | = help: consider dropping the `continue` expression @@ -71,7 +71,7 @@ error: this `continue` expression is redundant --> tests/ui/needless_continue.rs:68:9 | LL | continue; - | ^^^^^^^^^ + | ^^^^^^^^ | = help: consider dropping the `continue` expression @@ -91,8 +91,16 @@ LL | continue | = help: consider dropping the `continue` expression +error: this `continue` expression is redundant + --> tests/ui/needless_continue.rs:93:11 + | +LL | { continue } + | ^^^^^^^^ + | + = help: consider dropping the `continue` expression + error: this `else` block is redundant - --> tests/ui/needless_continue.rs:136:24 + --> tests/ui/needless_continue.rs:144:24 | LL | } else { | ________________________^ @@ -117,7 +125,7 @@ LL | | } } error: there is no need for an explicit `else` block for this `if` expression - --> tests/ui/needless_continue.rs:143:17 + --> tests/ui/needless_continue.rs:151:17 | LL | / if condition() { LL | | @@ -137,12 +145,28 @@ LL | | } } error: this `continue` expression is redundant - --> tests/ui/needless_continue.rs:166:13 + --> tests/ui/needless_continue.rs:174:13 | LL | continue 'b; - | ^^^^^^^^^^^^ + | ^^^^^^^^^^^ + | + = help: consider dropping the `continue` expression + +error: this `continue` expression is redundant + --> tests/ui/needless_continue.rs:190:21 + | +LL | continue 'inner; + | ^^^^^^^^^^^^^^^ + | + = help: consider dropping the `continue` expression + +error: this `continue` expression is redundant + --> tests/ui/needless_continue.rs:193:21 + | +LL | continue 'inner; + | ^^^^^^^^^^^^^^^ | = help: consider dropping the `continue` expression -error: aborting due to 9 previous errors +error: aborting due to 12 previous errors