Auto merge of #3871 - taiki-e:needless_continue, r=phansch

bors · bors · commit 75bfa2953330 · 2019-03-12T07:04:44.000Z
Fix `needless_continue` false positive If the `continue` has a label, check it matches the label of the loop. Fixes #2329
diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs
@@ -181,19 +181,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
             }
@@ -202,17 +202,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<F>(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()),
         _ => {},
     }
 }
@@ -350,7 +362,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 {
@@ -361,14 +373,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
@@ -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
@@ -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
 
