diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e3168b8cfe02..bbc7c1f42afa 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -618,7 +618,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements)); store.register_early_pass(|| Box::new(precedence::Precedence)); store.register_late_pass(|_| Box::new(needless_parens_on_range_literals::NeedlessParensOnRangeLiterals)); - store.register_early_pass(|| Box::new(needless_continue::NeedlessContinue)); + store.register_late_pass(|_| Box::new(needless_continue::NeedlessContinue)); store.register_early_pass(|| Box::new(redundant_else::RedundantElse)); store.register_late_pass(|_| Box::new(create_dir::CreateDir)); store.register_early_pass(|| Box::new(needless_arbitrary_self_type::NeedlessArbitrarySelfType)); diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index b5d6da478d8a..a064a5910ef9 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -871,7 +871,10 @@ impl<'tcx> LateLintPass<'tcx> for Loops { while_let_on_iterator::check(cx, expr); - if let Some(higher::While { condition, body, span }) = higher::While::hir(expr) { + if let Some(higher::While { + condition, body, span, .. + }) = higher::While::hir(expr) + { while_immutable_condition::check(cx, condition, body); while_float::check(cx, condition); missing_spin_loop::check(cx, condition, body); diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index b8601f77e249..55208ae708b9 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -1,9 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::source::{indent_of, snippet, snippet_block}; -use rustc_ast::{Block, Label, ast}; -use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; +use clippy_utils::higher; +use clippy_utils::source::{indent_of, snippet_block, snippet_with_context}; +use rustc_ast::Label; +use rustc_errors::Applicability; +use rustc_hir::{Block, Expr, ExprKind, LoopSource, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::declare_lint_pass; -use rustc_span::Span; +use rustc_span::{ExpnKind, Span}; declare_clippy_lint! { /// ### What it does @@ -127,9 +130,11 @@ declare_clippy_lint! { declare_lint_pass!(NeedlessContinue => [NEEDLESS_CONTINUE]); -impl EarlyLintPass for NeedlessContinue { - fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) { - if !expr.span.from_expansion() { +impl<'tcx> LateLintPass<'tcx> for NeedlessContinue { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + // We cannot use `from_expansion` because for loops, while loops and while let loops are desugared + // into `loop` expressions. + if !matches!(expr.span.ctxt().outer_expn_data().kind, ExpnKind::Macro(..)) { check_and_warn(cx, expr); } } @@ -188,19 +193,19 @@ 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<&Label>) -> bool { +fn needless_continue_in_else(else_expr: &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()), + ExprKind::Block(else_block, _) => is_first_block_stmt_continue(else_block, label), + ExprKind::Continue(l) => compare_labels(label, l.label.as_ref()), _ => false, } } -fn is_first_block_stmt_continue(block: &Block, label: Option<&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 { - compare_labels(label, l.as_ref()) + StmtKind::Semi(e) | StmtKind::Expr(e) => { + if let ExprKind::Continue(l) = e.kind { + compare_labels(label, l.label.as_ref()) } else { false } @@ -222,20 +227,34 @@ fn compare_labels(loop_label: Option<&Label>, continue_label: Option<&Label>) -> } /// If `expr` is a loop expression (while/while let/for/loop), calls `func` with -/// the AST object representing the loop block of `expr`. -fn with_loop_block(expr: &ast::Expr, mut func: F) +/// the HIR object representing the loop block of `expr`. +fn with_loop_block(expr: &Expr<'_>, mut func: F) where - F: FnMut(&Block, Option<&Label>), + F: FnMut(&Block<'_>, Option<&Label>), { - if let ast::ExprKind::While(_, loop_block, label) - | ast::ExprKind::ForLoop { - body: loop_block, - label, - .. + if let Some(higher::ForLoop { body, label, .. }) = higher::ForLoop::hir(expr) + && let ExprKind::Block(block, _) = &body.kind + { + func(block, label.as_ref()); + return; } - | ast::ExprKind::Loop(loop_block, label, ..) = &expr.kind + + if let Some(higher::While { body, label, .. }) = higher::While::hir(expr) + && let ExprKind::Block(block, _) = &body.kind { - func(loop_block, label.as_ref()); + func(block, label.as_ref()); + return; + } + + if let Some(higher::WhileLet { if_then, label, .. }) = higher::WhileLet::hir(expr) + && let ExprKind::Block(block, _) = &if_then.kind + { + func(block, label.as_ref()); + return; + } + + if let ExprKind::Loop(block, label, LoopSource::Loop, ..) = expr.kind { + func(block, label.as_ref()); } } @@ -247,17 +266,18 @@ where /// - The `if` condition expression, /// - The `then` block, and /// - The `else` expression. -fn with_if_expr(stmt: &ast::Stmt, mut func: F) +fn with_if_expr(expr: &Expr<'_>, mut func: F) where - F: FnMut(&ast::Expr, &ast::Expr, &Block, &ast::Expr), + F: FnMut(&Expr<'_>, &Expr<'_>, &Block<'_>, &Expr<'_>), { - match stmt.kind { - ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => { - if let ast::ExprKind::If(ref cond, ref if_block, Some(ref else_expr)) = e.kind { - func(e, cond, if_block, else_expr); - } - }, - _ => {}, + if let Some(higher::If { + cond, + then, + r#else: Some(r#else), + }) = higher::If::hir(expr) + && let ExprKind::Block(then, _) = then.kind + { + func(expr, cond, then, r#else); } } @@ -269,20 +289,21 @@ enum LintType { } /// Data we pass around for construction of help messages. -struct LintData<'a> { +#[derive(Debug)] +struct LintData<'hir> { /// The `if` expression encountered in the above loop. - if_expr: &'a ast::Expr, + if_expr: &'hir Expr<'hir>, /// The condition expression for the above `if`. - if_cond: &'a ast::Expr, + if_cond: &'hir Expr<'hir>, /// The `then` block of the `if` statement. - if_block: &'a Block, + if_block: &'hir Block<'hir>, /// 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, + else_expr: &'hir Expr<'hir>, /// The 0-based index of the `if` statement in the containing loop block. - stmt_idx: usize, + stmt_idx: Option, /// The statements of the loop block. - loop_block: &'a Block, + loop_block: &'hir Block<'hir>, } const MSG_REDUNDANT_CONTINUE_EXPRESSION: &str = "this `continue` expression is redundant"; @@ -299,7 +320,7 @@ const DROP_ELSE_BLOCK_MSG: &str = "consider dropping the `else` clause"; const DROP_CONTINUE_EXPRESSION_MSG: &str = "consider dropping the `continue` expression"; -fn emit_warning(cx: &EarlyContext<'_>, data: &LintData<'_>, header: &str, typ: LintType) { +fn emit_warning(cx: &LateContext<'_>, data: &LintData<'_>, header: &str, typ: LintType) { // snip is the whole *help* message that appears after the warning. // message is the warning message. // expr is the expression which the lint warning message refers to. @@ -325,8 +346,15 @@ fn emit_warning(cx: &EarlyContext<'_>, data: &LintData<'_>, header: &str, typ: L ); } -fn suggestion_snippet_for_continue_inside_if(cx: &EarlyContext<'_>, data: &LintData<'_>) -> String { - let cond_code = snippet(cx, data.if_cond.span, ".."); +fn suggestion_snippet_for_continue_inside_if(cx: &LateContext<'_>, data: &LintData<'_>) -> String { + let mut applicability = Applicability::MachineApplicable; + let (cond_code, _) = snippet_with_context( + cx, + data.if_cond.span, + data.if_expr.span.ctxt(), + "..", + &mut applicability, + ); let continue_code = snippet_block(cx, data.if_block.span, "..", Some(data.if_expr.span)); @@ -339,8 +367,15 @@ fn suggestion_snippet_for_continue_inside_if(cx: &EarlyContext<'_>, data: &LintD ) } -fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &LintData<'_>) -> String { - let cond_code = snippet(cx, data.if_cond.span, ".."); +fn suggestion_snippet_for_continue_inside_else(cx: &LateContext<'_>, data: &LintData<'_>) -> String { + let mut applicability = Applicability::MachineApplicable; + let (cond_code, _) = snippet_with_context( + cx, + data.if_cond.span, + data.if_expr.span.ctxt(), + "..", + &mut applicability, + ); // Region B let block_code = erode_from_back(&snippet_block(cx, data.if_block.span, "..", Some(data.if_expr.span))); @@ -352,18 +387,32 @@ fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &Lin let indent = span_of_first_expr_in_block(data.if_block) .and_then(|span| indent_of(cx, span)) .unwrap_or(0); - let to_annex = data.loop_block.stmts[data.stmt_idx + 1..] - .iter() - .map(|stmt| { - let span = cx.sess().source_map().stmt_span(stmt.span, data.loop_block.span); + let to_annex = if let Some(stmt_idx) = data.stmt_idx { + let mut lines = data.loop_block.stmts[stmt_idx + 1..] + .iter() + .map(|stmt| { + let span = cx.sess().source_map().stmt_span(stmt.span, data.loop_block.span); + let snip = snippet_block(cx, span, "..", None); + snip.lines() + .map(|line| format!("{}{line}", " ".repeat(indent))) + .collect::>() + .join("\n") + }) + .collect::>(); + if let Some(expr) = data.loop_block.expr { + let span = expr.span; let snip = snippet_block(cx, span, "..", None); - snip.lines() + let expr_lines = snip + .lines() .map(|line| format!("{}{line}", " ".repeat(indent))) .collect::>() - .join("\n") - }) - .collect::>() - .join("\n"); + .join("\n"); + lines.push(expr_lines); + } + lines.join("\n") + } else { + String::new() + }; let indent_if = indent_of(cx, data.if_expr.span).unwrap_or(0); format!( @@ -373,46 +422,53 @@ fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &Lin ) } -fn check_last_stmt_in_expr(inner_expr: &ast::Expr, func: &F) +fn check_last_stmt_in_expr(cx: &LateContext<'_>, inner_expr: &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); + match inner_expr.kind { + ExprKind::Continue(continue_label) => { + func(continue_label.label.as_ref(), inner_expr.span); }, - ast::ExprKind::If(_, then_block, else_block) => { - check_last_stmt_in_block(then_block, func); + ExprKind::If(_, then_block, else_block) if let ExprKind::Block(then_block, _) = then_block.kind => { + check_last_stmt_in_block(cx, then_block, func); if let Some(else_block) = else_block { - check_last_stmt_in_expr(else_block, func); + check_last_stmt_in_expr(cx, else_block, func); } }, - ast::ExprKind::Match(_, arms, _) => { + ExprKind::Match(_, arms, _) => { + let match_ty = cx.typeck_results().expr_ty(inner_expr); + if !match_ty.is_unit() && !match_ty.is_never() { + return; + } for arm in arms { - if let Some(expr) = &arm.body { - check_last_stmt_in_expr(expr, func); - } + check_last_stmt_in_expr(cx, arm.body, func); } }, - ast::ExprKind::Block(b, _) => { - check_last_stmt_in_block(b, func); + ExprKind::Block(b, _) => { + check_last_stmt_in_block(cx, b, func); }, _ => {}, } } -fn check_last_stmt_in_block(b: &Block, func: &F) +fn check_last_stmt_in_block(cx: &LateContext<'_>, b: &Block<'_>, func: &F) where F: Fn(Option<&Label>, Span), { + if let Some(expr) = b.expr { + check_last_stmt_in_expr(cx, expr, func); + return; + } + if let Some(last_stmt) = b.stmts.last() - && let ast::StmtKind::Expr(inner_expr) | ast::StmtKind::Semi(inner_expr) = &last_stmt.kind + && let StmtKind::Expr(inner_expr) | StmtKind::Semi(inner_expr) = last_stmt.kind { - check_last_stmt_in_expr(inner_expr, func); + check_last_stmt_in_expr(cx, inner_expr, func); } } -fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) { +fn check_and_warn(cx: &LateContext<'_>, expr: &Expr<'_>) { with_loop_block(expr, |loop_block, label| { let p = |continue_label: Option<&Label>, span: Span| { if compare_labels(label, continue_label) { @@ -427,16 +483,51 @@ fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) { } }; - let stmts = &loop_block.stmts; + let stmts = loop_block.stmts; for (i, stmt) in stmts.iter().enumerate() { let mut maybe_emitted_in_if = false; - with_if_expr(stmt, |if_expr, cond, then_block, else_expr| { + if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind { + with_if_expr(expr, |if_expr, cond, then_block, else_expr| { + let data = &LintData { + if_expr, + if_cond: cond, + if_block: then_block, + else_expr, + stmt_idx: Some(i), + loop_block, + }; + + maybe_emitted_in_if = true; + if needless_continue_in_else(else_expr, label) { + emit_warning( + cx, + data, + DROP_ELSE_BLOCK_AND_MERGE_MSG, + LintType::ContinueInsideElseBlock, + ); + } else if is_first_block_stmt_continue(then_block, label) { + emit_warning(cx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock); + } else { + maybe_emitted_in_if = false; + } + }); + } + + if i == stmts.len() - 1 && loop_block.expr.is_none() && !maybe_emitted_in_if { + check_last_stmt_in_block(cx, loop_block, &p); + } + } + + if let Some(expr) = loop_block.expr { + let mut maybe_emitted_in_if = false; + + with_if_expr(expr, |if_expr, cond, then_block, else_expr| { let data = &LintData { if_expr, if_cond: cond, if_block: then_block, else_expr, - stmt_idx: i, + stmt_idx: None, loop_block, }; @@ -455,8 +546,8 @@ fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) { } }); - if i == stmts.len() - 1 && !maybe_emitted_in_if { - check_last_stmt_in_block(loop_block, &p); + if !maybe_emitted_in_if { + check_last_stmt_in_block(cx, loop_block, &p); } } }); @@ -491,8 +582,12 @@ fn erode_from_back(s: &str) -> String { if ret.is_empty() { s.to_string() } else { ret } } -fn span_of_first_expr_in_block(block: &Block) -> Option { - block.stmts.first().map(|stmt| stmt.span) +fn span_of_first_expr_in_block(block: &Block<'_>) -> Option { + block + .stmts + .first() + .map(|stmt| stmt.span) + .or(block.expr.map(|expr| expr.span)) } #[cfg(test)] diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 95e6ecd40dec..9fe6d8c62d2a 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -14,6 +14,7 @@ use rustc_span::{Span, symbol}; /// The essential nodes of a desugared for loop as well as the entire span: /// `for pat in arg { body }` becomes `(pat, arg, body)`. Returns `(pat, arg, body, span)`. +#[derive(Debug)] pub struct ForLoop<'tcx> { /// `for` loop item pub pat: &'tcx Pat<'tcx>, @@ -318,6 +319,7 @@ pub struct While<'hir> { pub body: &'hir Expr<'hir>, /// Span of the loop header pub span: Span, + pub label: Option, } impl<'hir> While<'hir> { @@ -333,13 +335,18 @@ impl<'hir> While<'hir> { }), .. }, - _, + label, LoopSource::While, span, ) = expr.kind && !has_let_expr(condition) { - return Some(Self { condition, body, span }); + return Some(Self { + condition, + body, + span, + label, + }); } None } diff --git a/tests/ui/needless_continue.rs b/tests/ui/needless_continue.rs index e873db6dee14..88b7905e7fa8 100644 --- a/tests/ui/needless_continue.rs +++ b/tests/ui/needless_continue.rs @@ -244,3 +244,101 @@ mod issue_4077 { true } } + +#[allow(clippy::let_unit_value)] +mod issue14550 { + fn match_with_value(mut producer: impl Iterator>) -> Result { + let mut counter = 2; + loop { + match producer.next().unwrap() { + Ok(ok) => break Ok((ok + 1) as u32), + Err(12) => { + counter -= 1; + continue; + }, + err => err?, + }; + } + } + + fn inside_macro() { + macro_rules! mac { + ($e:expr => $($rest:tt);*) => { + loop { + match $e { + 1 => continue, + 2 => break, + n => println!("{n}"), + } + $($rest;)* + } + }; + } + + mac!(2 => ); + mac!(1 => {println!("foobar")}); + } + + mod partially_inside_macro { + macro_rules! select { + ( + $expr:expr, + $( $pat:pat => $then:expr ),* + ) => { + fn foo() { + loop { + match $expr { + $( + $pat => $then, + )* + } + } + } + }; + } + + select!(Some(1), + Some(1) => { + println!("one"); + continue; + }, + Some(2) => {}, + None => break, + _ => () + ); + + macro_rules! choose { + ( + $expr:expr, + $case:expr + ) => { + fn bar() { + loop { + match $expr { + $case => { + println!("matched"); + continue; + }, + _ => { + println!("not matched"); + break; + }, + } + } + } + }; + } + + choose!(todo!(), 5); + } +} + +fn issue15548() { + loop { + if todo!() { + } else { + //~^ needless_continue + continue; + } + } +} diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr index 878c1e731e32..7a65872c85cd 100644 --- a/tests/ui/needless_continue.stderr +++ b/tests/ui/needless_continue.stderr @@ -220,5 +220,21 @@ LL | | } do_something(); } -error: aborting due to 15 previous errors +error: this `else` block is redundant + --> tests/ui/needless_continue.rs:339:16 + | +LL | } else { + | ________________^ +LL | | +LL | | continue; +LL | | } + | |_________^ + | + = help: consider dropping the `else` clause and merging the code that follows (in the loop) with the `if` block + if todo!() { + // merged code follows: + + } + +error: aborting due to 16 previous errors