Merge pull request #1549 from Manishearth/never_loop

New never loop lint

Author: oli-obk

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
+* New [`never_loop`] lint
 * New [`mut_from_ref`] lint
 
 ## 0.0.114 — 2017-02-08
@@ -384,6 +385,7 @@ All notable changes to this project will be documented in this file.
 [`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return
 [`needless_update`]: https://github.com/Manishearth/rust-clippy/wiki#needless_update
 [`neg_multiply`]: https://github.com/Manishearth/rust-clippy/wiki#neg_multiply
+[`never_loop`]: https://github.com/Manishearth/rust-clippy/wiki#never_loop
 [`new_ret_no_self`]: https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self
 [`new_without_default`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default
 [`new_without_default_derive`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive

README.md

@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 189 lints included in this crate:
+There are 190 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -291,6 +291,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)                                               | warn    | any loop with an unconditional `break` statement
 [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

clippy_lints/src/lib.rs

@@ -405,6 +405,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,

clippy_lints/src/loops.rs

@@ -286,6 +286,23 @@ 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`.
+///
+/// **Why is this bad?** This loop never loops, all it does is obfuscating the
+/// code.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// loop { ..; break; }
+/// ```
+declare_lint! {
+    pub NEVER_LOOP,
+    Warn,
+    "any loop with an unconditional `break` statement"
+}
+
 #[derive(Copy, Clone)]
 pub struct Pass;
 
@@ -303,7 +320,8 @@ impl LintPass for Pass {
                     EXPLICIT_COUNTER_LOOP,
                     EMPTY_LOOP,
                     WHILE_LET_ON_ITERATOR,
-                    FOR_KV_MAP)
+                    FOR_KV_MAP,
+                    NEVER_LOOP)
     }
 }
 
@@ -324,6 +342,9 @@ 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);
@@ -402,6 +423,51 @@ 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_stmt(stmt: &Stmt) -> bool {
+    match stmt.node {
+        StmtSemi(ref e, _) |
+        StmtExpr(ref e, _) => never_loop_expr(e),
+        StmtDecl(ref d, _) => never_loop_decl(d),
+    }
+}
+
+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 never_loop_expr(expr: &Expr) -> 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),
+        ExprAssign(ref e1, ref e2) |
+        ExprAssignOp(_, ref e1, ref e2) |
+        ExprIndex(ref e1, ref e2) => never_loop_expr(e1) || never_loop_expr(e2),
+        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)),
+        _ => false,
+    }
+}
+
 fn check_for_loop<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     pat: &'tcx Pat,

tests/ui/never_loop.rs

@@ -0,0 +1,34 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(never_loop)]
+#![allow(dead_code, unused)]
+
+fn main() {
+    loop {
+        println!("This is only ever printed once");
+        break;
+    }
+
+    let x = 1;
+    loop {
+        println!("This, too"); // but that's OK
+        if x == 1 {
+            break;
+        }
+    }
+
+    loop {
+        loop {
+            // another one
+            break;
+        }
+        break;
+    }
+
+    loop {
+        loop {
+            if x == 1 { return; }
+        }
+    }
+}

tests/ui/never_loop.stderr

@@ -0,0 +1,41 @@
+error: this loop never actually loops
+  --> $DIR/never_loop.rs:8:5
+   |
+8  |       loop {
+   |  _____^ starting here...
+9  | |         println!("This is only ever printed once");
+10 | |         break;
+11 | |     }
+   | |_____^ ...ending here
+   |
+note: lint level defined here
+  --> $DIR/never_loop.rs:4:9
+   |
+4  | #![deny(never_loop)]
+   |         ^^^^^^^^^^
+
+error: this loop never actually loops
+  --> $DIR/never_loop.rs:21:5
+   |
+21 |       loop {
+   |  _____^ starting here...
+22 | |         loop {
+23 | |             // another one
+24 | |             break;
+25 | |         }
+26 | |         break;
+27 | |     }
+   | |_____^ ...ending here
+
+error: this loop never actually loops
+  --> $DIR/never_loop.rs:22:9
+   |
+22 |           loop {
+   |  _________^ starting here...
+23 | |             // another one
+24 | |             break;
+25 | |         }
+   | |_________^ ...ending here
+
+error: aborting due to 3 previous errors
+

tests/ui/unused_labels.rs

@@ -1,7 +1,7 @@
 #![plugin(clippy)]
 #![feature(plugin)]
 
-#![allow(dead_code, items_after_statements)]
+#![allow(dead_code, items_after_statements, never_loop)]
 #![deny(unused_label)]
 
 fn unused_label() {

tests/ui/while_loop.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(while_let_loop, empty_loop, while_let_on_iterator)]
-#![allow(dead_code, unused, cyclomatic_complexity)]
+#![allow(dead_code, never_loop, unused, cyclomatic_complexity)]
 
 fn main() {
     let y = Some(true);