diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 00394a9e1e2..102b3ea2fff 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -175,19 +175,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) -> bool { +fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool { match else_expr.node { - ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block), - ast::ExprKind::Continue(_) => true, + ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label), + ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()), _ => false, } } -fn is_first_block_stmt_continue(block: &ast::Block) -> bool { +fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> bool { block.stmts.get(0).map_or(false, |stmt| match stmt.node { ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => { - if let ast::ExprKind::Continue(_) = e.node { - true + if let ast::ExprKind::Continue(ref l) = e.node { + compare_labels(label, l.as_ref()) } else { false } @@ -196,17 +196,29 @@ fn is_first_block_stmt_continue(block: &ast::Block) -> bool { }) } +/// 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 { + match (loop_label, continue_label) { + // `loop { continue; }` or `'a loop { continue; }` + (_, None) => true, + // `loop { continue 'a; }` + (None, _) => false, + // `'a loop { continue 'a; }` or `'a loop { continue 'b; }` + (Some(x), Some(y)) => x.ident == y.ident, + } +} + /// 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) where - F: FnMut(&ast::Block), + F: FnMut(&ast::Block, Option<&ast::Label>), { match expr.node { - ast::ExprKind::While(_, ref loop_block, _) - | ast::ExprKind::WhileLet(_, _, ref loop_block, _) - | ast::ExprKind::ForLoop(_, _, ref loop_block, _) - | ast::ExprKind::Loop(ref loop_block, _) => func(loop_block), + ast::ExprKind::While(_, ref loop_block, ref label) + | ast::ExprKind::WhileLet(_, _, ref loop_block, ref label) + | ast::ExprKind::ForLoop(_, _, ref loop_block, ref label) + | ast::ExprKind::Loop(ref loop_block, ref label) => func(loop_block, label.as_ref()), _ => {}, } } @@ -344,7 +356,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>( } fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) { - with_loop_block(expr, |loop_block| { + with_loop_block(expr, |loop_block, label| { for (i, stmt) in loop_block.stmts.iter().enumerate() { with_if_expr(stmt, |if_expr, cond, then_block, else_expr| { let data = &LintData { @@ -355,14 +367,14 @@ fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) { else_expr, block_stmts: &loop_block.stmts, }; - if needless_continue_in_else(else_expr) { + if needless_continue_in_else(else_expr, label) { emit_warning( ctx, data, DROP_ELSE_BLOCK_AND_MERGE_MSG, LintType::ContinueInsideElseBlock, ); - } else if is_first_block_stmt_continue(then_block) { + } else if is_first_block_stmt_continue(then_block, label) { emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock); } }); diff --git a/tests/ui/needless_continue.rs b/tests/ui/needless_continue.rs index 8bb1ba6edb5..5da95647f2c 100644 --- a/tests/ui/needless_continue.rs +++ b/tests/ui/needless_continue.rs @@ -1,3 +1,5 @@ +#![warn(clippy::needless_continue)] + macro_rules! zero { ($x:expr) => { $x == 0 @@ -10,7 +12,6 @@ macro_rules! nonzero { }; } -#[warn(clippy::needless_continue)] fn main() { let mut i = 1; while i < 10 { @@ -49,3 +50,66 @@ fn main() { println!("bleh"); } } + +mod issue_2329 { + fn condition() -> bool { + unimplemented!() + } + fn update_condition() {} + + // only the outer loop has a label + fn foo() { + 'outer: loop { + println!("Entry"); + while condition() { + update_condition(); + if condition() { + println!("foo-1"); + } else { + continue 'outer; // should not lint here + } + println!("foo-2"); + + update_condition(); + if condition() { + continue 'outer; // should not lint here + } else { + println!("foo-3"); + } + println!("foo-4"); + } + } + } + + // both loops have labels + fn bar() { + 'outer: loop { + println!("Entry"); + 'inner: while condition() { + update_condition(); + if condition() { + println!("bar-1"); + } else { + continue 'outer; // should not lint here + } + println!("bar-2"); + + update_condition(); + if condition() { + println!("bar-3"); + } else { + continue 'inner; // should lint here + } + println!("bar-4"); + + update_condition(); + if condition() { + continue; // should lint here + } else { + println!("bar-5"); + } + println!("bar-6"); + } + } + } +} diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr index 763eaf3a70e..340ae66dae4 100644 --- a/tests/ui/needless_continue.stderr +++ b/tests/ui/needless_continue.stderr @@ -1,6 +1,6 @@ error: This else block is redundant. - --> $DIR/needless_continue.rs:27:16 + --> $DIR/needless_continue.rs:28:16 | LL | } else { | ________________^ @@ -37,7 +37,7 @@ LL | | } error: There is no need for an explicit `else` block for this `if` expression - --> $DIR/needless_continue.rs:42:9 + --> $DIR/needless_continue.rs:43:9 | LL | / if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { LL | | continue; @@ -55,5 +55,47 @@ LL | | } println!("Jabber"); ... -error: aborting due to 2 previous errors +error: This else block is redundant. + + --> $DIR/needless_continue.rs:100:24 + | +LL | } else { + | ________________________^ +LL | | continue 'inner; // should lint here +LL | | } + | |_________________^ + | + = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: + if condition() { + println!("bar-3"); + // Merged code follows...println!("bar-4"); + update_condition(); + if condition() { + continue; // should lint here + } else { + println!("bar-5"); + } + println!("bar-6"); + } + + +error: There is no need for an explicit `else` block for this `if` expression + + --> $DIR/needless_continue.rs:106:17 + | +LL | / if condition() { +LL | | continue; // should lint here +LL | | } else { +LL | | println!("bar-5"); +LL | | } + | |_________________^ + | + = help: Consider dropping the else clause, and moving out the code in the else block, like so: + if condition() { + continue; + } + println!("bar-5"); + ... + +error: aborting due to 4 previous errors