diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ca1336afaf..90f67e8aec7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 0a6a62e9185..9e4b9fba1b6 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 80a369513e2..13ad8b41f96 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c863b11e0b8..d7f952255fe 100644 --- a/clippy_lints/src/loops.rs +++ b/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, diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs new file mode 100644 index 00000000000..087f1a1e6af --- /dev/null +++ b/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; } + } + } +} diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr new file mode 100644 index 00000000000..ebb98a6cf14 --- /dev/null +++ b/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 + diff --git a/tests/ui/unused_labels.rs b/tests/ui/unused_labels.rs index ce718c4903a..1eb067e9684 100644 --- a/tests/ui/unused_labels.rs +++ b/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() { diff --git a/tests/ui/while_loop.rs b/tests/ui/while_loop.rs index 2b75e5b83e8..253aebe4444 100644 --- a/tests/ui/while_loop.rs +++ b/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);