Merge pull request #1804 from camsteffen/never_loop

oli-obk · web-flow · commit b1e9c1b7e760 · 2017-06-12T08:08:19.000+02:00
fix never_loop
diff --git a/README.md b/README.md
@@ -295,7 +295,7 @@ name
 [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return)                                     | warn    | using a return statement like `return expr;` where an expression would suffice
 [needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update)                                     | warn    | using `Foo { ..base }` when there are no missing fields
 [neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply)                                           | warn    | multiplying integers with -1
-[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop)                                               | allow   | any loop with an unconditional `break` or `return` statement
+[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop)                                               | warn    | any loop that will always `break` or `return`
 [new_ret_no_self](https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self)                                     | warn    | not returning `Self` in a `new` method
 [new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default)                             | warn    | `fn new() -> Self` method without `Default` implementation
 [new_without_default_derive](https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive)               | warn    | `fn new() -> Self` without `#[derive]`able `Default` implementation
diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs
@@ -184,16 +184,15 @@ fn is_relevant_trait(tcx: TyCtxt, item: &TraitItem) -> bool {
 }
 
 fn is_relevant_block(tcx: TyCtxt, tables: &ty::TypeckTables, block: &Block) -> bool {
-    for stmt in &block.stmts {
+    if let Some(stmt) = block.stmts.first() {
         match stmt.node {
-            StmtDecl(_, _) => return true,
+            StmtDecl(_, _) => true,
             StmtExpr(ref expr, _) |
-            StmtSemi(ref expr, _) => {
-                return is_relevant_expr(tcx, tables, expr);
-            },
+            StmtSemi(ref expr, _) => is_relevant_expr(tcx, tables, expr),
         }
+    } else {
+        block.expr.as_ref().map_or(false, |e| is_relevant_expr(tcx, tables, e))
     }
-    block.expr.as_ref().map_or(false, |e| is_relevant_expr(tcx, tables, e))
 }
 
 fn is_relevant_expr(tcx: TyCtxt, tables: &ty::TypeckTables, expr: &Expr) -> bool {
diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs
@@ -200,7 +200,7 @@ fn check_doc(cx: &EarlyContext, valid_idents: &[String], docs: &[(String, Span)]
         type Item = (bool, char);
 
         fn next(&mut self) -> Option<(bool, char)> {
-            while self.line < self.docs.len() {
+            if self.line < self.docs.len() {
                 if self.reset {
                     self.line += 1;
                     self.reset = false;
@@ -215,18 +215,18 @@ fn check_doc(cx: &EarlyContext, valid_idents: &[String], docs: &[(String, Span)]
                     self.pos += c.len_utf8();
                     let new_line = self.new_line;
                     self.new_line = c == '\n' || (self.new_line && c.is_whitespace());
-                    return Some((new_line, c));
+                    Some((new_line, c))
                 } else if self.line == self.docs.len() - 1 {
-                    return None;
+                    None
                 } else {
                     self.new_line = true;
                     self.reset = true;
                     self.pos += 1;
-                    return Some((true, '\n'));
+                    Some((true, '\n'))
                 }
+            } else {
+                None
             }
-
-            None
         }
     }
 
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -324,7 +324,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         enum_variants::STUTTER,
         if_not_else::IF_NOT_ELSE,
         items_after_statements::ITEMS_AFTER_STATEMENTS,
-        loops::NEVER_LOOP,
         matches::SINGLE_MATCH_ELSE,
         mem_forget::MEM_FORGET,
         methods::FILTER_MAP,
@@ -420,6 +419,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         loops::FOR_LOOP_OVER_RESULT,
         loops::ITER_NEXT_LOOP,
         loops::NEEDLESS_RANGE_LOOP,
+        loops::NEVER_LOOP,
         loops::REVERSE_RANGE_LOOP,
         loops::UNUSED_COLLECT,
         loops::WHILE_LET_LOOP,
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
@@ -286,24 +286,22 @@ declare_lint! {
     "looping on a map using `iter` when `keys` or `values` would do"
 }
 
-/// **What it does:** Checks for loops that contain an unconditional `break`
-/// or `return`.
+/// **What it does:** Checks for loops that will always `break`, `return` or
+/// `continue` an outer loop.
 ///
 /// **Why is this bad?** This loop never loops, all it does is obfuscating the
 /// code.
 ///
-/// **Known problems:** Ignores `continue` statements in the loop that create
-/// nontrivial control flow. Therefore set to `Allow` by default.
-/// See https://github.com/Manishearth/rust-clippy/issues/1586
+/// **Known problems:** None
 ///
 /// **Example:**
 /// ```rust
 /// loop { ..; break; }
 /// ```
 declare_lint! {
     pub NEVER_LOOP,
-    Allow,
-    "any loop with an unconditional `break` or `return` statement"
+    Warn,
+    "any loop that will always `break` or `return`"
 }
 
 #[derive(Copy, Clone)]
@@ -333,6 +331,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         if let Some((pat, arg, body)) = higher::for_loop(expr) {
             check_for_loop(cx, pat, arg, body, expr);
         }
+
+        // check for never_loop
+        match expr.node {
+            ExprWhile(_, ref block, _) |
+            ExprLoop(ref block, _, _) => {
+                if never_loop(block, &expr.id) {
+                    span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
+                }
+            },
+            _ => (),
+        }
+
         // check for `loop { if let {} else break }` that could be `while let`
         // (also matches an explicit "match" instead of "if let")
         // (even if the "match" or "if let" is used for declaration)
@@ -345,9 +355,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                           "empty `loop {}` detected. You may want to either use `panic!()` or add \
                            `std::thread::sleep(..);` to the loop body.");
             }
-            if never_loop_block(block) {
-                span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
-            }
 
             // extract the expression from the first statement (if any) in a block
             let inner_stmt_expr = extract_expr_from_first_stmt(block);
@@ -425,47 +432,103 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
     }
 }
 
-fn never_loop_block(block: &Block) -> bool {
-    block.stmts.iter().any(never_loop_stmt) || block.expr.as_ref().map_or(false, |e| never_loop_expr(e))
+fn never_loop(block: &Block, id: &NodeId) -> bool {
+    !contains_continue_block(block, id) && loop_exit_block(block)
+}
+
+fn contains_continue_block(block: &Block, dest: &NodeId) -> bool {
+    block.stmts.iter().any(|e| contains_continue_stmt(e, dest))
+        || block.expr.as_ref().map_or(false, |e| contains_continue_expr(e, dest))
 }
 
-fn never_loop_stmt(stmt: &Stmt) -> bool {
+fn contains_continue_stmt(stmt: &Stmt, dest: &NodeId) -> bool {
     match stmt.node {
         StmtSemi(ref e, _) |
-        StmtExpr(ref e, _) => never_loop_expr(e),
-        StmtDecl(ref d, _) => never_loop_decl(d),
+        StmtExpr(ref e, _) => contains_continue_expr(e, dest),
+        StmtDecl(ref d, _) => contains_continue_decl(d, dest),
     }
 }
 
-fn never_loop_decl(decl: &Decl) -> bool {
-    if let DeclLocal(ref local) = decl.node {
-        local.init.as_ref().map_or(false, |e| never_loop_expr(e))
-    } else {
-        false
+fn contains_continue_decl(decl: &Decl, dest: &NodeId) -> bool {
+    match decl.node {
+        DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
+        _ => false
     }
 }
 
-fn never_loop_expr(expr: &Expr) -> bool {
+fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool {
     match expr.node {
-        ExprBreak(..) | ExprRet(..) => true,
         ExprBox(ref e) |
         ExprUnary(_, ref e) |
-        ExprBinary(_, ref e, _) | // because short-circuiting
         ExprCast(ref e, _) |
         ExprType(ref e, _) |
         ExprField(ref e, _) |
         ExprTupField(ref e, _) |
-        ExprRepeat(ref e, _) |
-        ExprAddrOf(_, ref e) => never_loop_expr(e),
+        ExprAddrOf(_, ref e) |
+        ExprRepeat(ref e, _) => contains_continue_expr(e, dest),
+        ExprArray(ref es) |
+        ExprMethodCall(_, _, ref es) |
+        ExprTup(ref es) => es.iter().any(|e| contains_continue_expr(e, dest)),
+        ExprCall(ref e, ref es) => contains_continue_expr(e, dest) || es.iter().any(|e| contains_continue_expr(e, dest)),
+        ExprBinary(_, ref e1, ref e2) |
         ExprAssign(ref e1, ref e2) |
         ExprAssignOp(_, ref e1, ref e2) |
-        ExprIndex(ref e1, ref e2) => never_loop_expr(e1) || never_loop_expr(e2),
+        ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| contains_continue_expr(e, dest)),
+        ExprIf(ref e, ref e2, ref e3) => [e, e2].iter().chain(e3.as_ref().iter()).any(|e| contains_continue_expr(e, dest)),
+        ExprWhile(ref e, ref b, _) => contains_continue_expr(e, dest) || contains_continue_block(b, dest),
+        ExprMatch(ref e, ref arms, _) => contains_continue_expr(e, dest) || arms.iter().any(|a| contains_continue_expr(&a.body, dest)),
+        ExprBlock(ref block) => contains_continue_block(block, dest),
+        ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
+        ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| id == *dest),
+        _ => false,
+    }
+}
+
+fn loop_exit_block(block: &Block) -> bool {
+    block.stmts.iter().any(|e| loop_exit_stmt(e))
+        || block.expr.as_ref().map_or(false, |e| loop_exit_expr(e))
+}
+
+fn loop_exit_stmt(stmt: &Stmt) -> bool {
+    match stmt.node {
+        StmtSemi(ref e, _) |
+        StmtExpr(ref e, _) => loop_exit_expr(e),
+        StmtDecl(ref d, _) => loop_exit_decl(d),
+    }
+}
+
+fn loop_exit_decl(decl: &Decl) -> bool {
+    match decl.node {
+        DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| loop_exit_expr(e)),
+        _ => false
+    }
+}
+
+fn loop_exit_expr(expr: &Expr) -> bool {
+    match expr.node {
+        ExprBox(ref e) |
+        ExprUnary(_, ref e) |
+        ExprCast(ref e, _) |
+        ExprType(ref e, _) |
+        ExprField(ref e, _) |
+        ExprTupField(ref e, _) |
+        ExprAddrOf(_, ref e) |
+        ExprRepeat(ref e, _) => loop_exit_expr(e),
         ExprArray(ref es) |
-        ExprTup(ref es) |
-        ExprMethodCall(_, _, ref es) => es.iter().any(|e| never_loop_expr(e)),
-        ExprCall(ref e, ref es) => never_loop_expr(e) || es.iter().any(|e| never_loop_expr(e)),
-        ExprBlock(ref block) => never_loop_block(block),
-        ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| never_loop_expr(e)),
+        ExprMethodCall(_, _, ref es) |
+        ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e)),
+        ExprCall(ref e, ref es) => loop_exit_expr(e) || es.iter().any(|e| loop_exit_expr(e)),
+        ExprBinary(_, ref e1, ref e2) |
+        ExprAssign(ref e1, ref e2) |
+        ExprAssignOp(_, ref e1, ref e2) |
+        ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| loop_exit_expr(e)),
+        ExprIf(ref e, ref e2, ref e3) => loop_exit_expr(e) || e3.as_ref().map_or(false, |e| loop_exit_expr(e)) && loop_exit_expr(e2),
+        ExprWhile(ref e, ref b, _) => loop_exit_expr(e) || loop_exit_block(b),
+        ExprMatch(ref e, ref arms, _) => loop_exit_expr(e) || arms.iter().all(|a| loop_exit_expr(&a.body)),
+        ExprBlock(ref b) => loop_exit_block(b),
+        ExprBreak(_, _) |
+        ExprAgain(_) |
+        ExprRet(_) => true,
         _ => false,
     }
 }
diff --git a/clippy_tests/examples/for_loop.stderr b/clippy_tests/examples/for_loop.stderr
@@ -53,6 +53,28 @@ error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`
    = note: `-D for-loop-over-result` implied by `-D warnings`
    = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")`
 
+error: this loop never actually loops
+  --> for_loop.rs:52:5
+   |
+52 | /     while let Some(x) = option {
+53 | |         println!("{}", x);
+54 | |         break;
+55 | |     }
+   | |_____^
+   |
+   = note: `-D never-loop` implied by `-D warnings`
+
+error: this loop never actually loops
+  --> for_loop.rs:58:5
+   |
+58 | /     while let Ok(x) = result {
+59 | |         println!("{}", x);
+60 | |         break;
+61 | |     }
+   | |_____^
+   |
+   = note: `-D never-loop` implied by `-D warnings`
+
 error: the loop variable `i` is only used to index `vec`.
   --> for_loop.rs:84:5
    |
diff --git a/clippy_tests/examples/never_loop.rs b/clippy_tests/examples/never_loop.rs
diff --git a/clippy_tests/examples/never_loop.stderr b/clippy_tests/examples/never_loop.stderr

Original file line number	Diff line number	Diff line change
`@@ -184,16 +184,15 @@ fn is_relevant_trait(tcx: TyCtxt, item: &TraitItem) -> bool {`
`184`	`184`	`}`
`185`	`185`
`186`	`186`	`fn is_relevant_block(tcx: TyCtxt, tables: &ty::TypeckTables, block: &Block) -> bool {`
`187`		`- for stmt in &block.stmts {`
	`187`	`+ if let Some(stmt) = block.stmts.first() {`
`188`	`188`	`match stmt.node {`
`189`		`- StmtDecl(_, _) => return true,`
	`189`	`+ StmtDecl(_, _) => true,`
`190`	`190`	`StmtExpr(ref expr, _) \|`
`191`		`- StmtSemi(ref expr, _) => {`
`192`		`- return is_relevant_expr(tcx, tables, expr);`
`193`		`- },`
	`191`	`+ StmtSemi(ref expr, _) => is_relevant_expr(tcx, tables, expr),`
`194`	`192`	`}`
	`193`	`+ } else {`
	`194`	`+ block.expr.as_ref().map_or(false, \|e\| is_relevant_expr(tcx, tables, e))`
`195`	`195`	`}`
`196`		`- block.expr.as_ref().map_or(false, \|e\| is_relevant_expr(tcx, tables, e))`
`197`	`196`	`}`
`198`	`197`
`199`	`198`	`fn is_relevant_expr(tcx: TyCtxt, tables: &ty::TypeckTables, expr: &Expr) -> bool {`