diff --git a/README.md b/README.md index c5b28812e94..b650c64eb21 100644 --- 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/lib.rs b/clippy_lints/src/lib.rs index e212aaeaf7b..fb0114f8f68 100644 --- 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 index 70e7e99a66d..c4a79203175 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -285,15 +285,13 @@ 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 @@ -301,8 +299,8 @@ declare_lint! { /// ``` 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)] @@ -344,7 +342,7 @@ 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) { + if never_loop(block, &expr.id) { span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"); } @@ -424,47 +422,100 @@ 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 never_loop_stmt(stmt: &Stmt) -> bool { +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 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), + 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)), 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) => 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)), + 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), + ExprMethodCall(_, _, ref es) => es.iter().any(|e| loop_exit_expr(e)), + ExprArray(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/never_loop.rs b/clippy_tests/examples/never_loop.rs index 90dae2134d5..5d0c7e3c206 100644 --- a/clippy_tests/examples/never_loop.rs +++ b/clippy_tests/examples/never_loop.rs @@ -2,33 +2,56 @@ #![plugin(clippy)] #![warn(never_loop)] -#![allow(dead_code, unused)] +#![allow(single_match, while_true)] -fn main() { +fn break_stmt() { loop { - println!("This is only ever printed once"); break; } +} - let x = 1; +fn conditional_break() { + let mut x = 5; loop { - println!("This, too"); // but that's OK + x -= 1; if x == 1 { - break; - } - } - - loop { - loop { - // another one - break; - } - break; - } - - loop { - loop { - if x == 1 { return; } + break } } } + +fn nested_loop() { + loop { + while true { + break + } + break + } +} + +fn if_false() { + let x = 1; + loop { + if x == 1 { + return + } + } +} + +fn match_false() { + let x = 1; + loop { + match x { + 1 => return, + _ => (), + } + } +} + +fn main() { + break_stmt(); + conditional_break(); + nested_loop(); + if_false(); + match_false(); +} diff --git a/clippy_tests/examples/never_loop.stderr b/clippy_tests/examples/never_loop.stderr index 28e172bbd01..4f2c0f60227 100644 --- a/clippy_tests/examples/never_loop.stderr +++ b/clippy_tests/examples/never_loop.stderr @@ -2,38 +2,25 @@ error: this loop never actually loops --> never_loop.rs:8:5 | 8 | / loop { -9 | | println!("This is only ever printed once"); -10 | | break; -11 | | } +9 | | break; +10 | | } | |_____^ | = note: `-D never-loop` implied by `-D warnings` error: this loop never actually loops - --> never_loop.rs:21:5 + --> never_loop.rs:24:5 | -21 | / loop { -22 | | loop { -23 | | // another one -24 | | break; -25 | | } -26 | | break; -27 | | } +24 | / loop { +25 | | while true { +26 | | break +27 | | } +28 | | break +29 | | } | |_____^ | = note: `-D never-loop` implied by `-D warnings` -error: this loop never actually loops - --> never_loop.rs:22:9 - | -22 | / loop { -23 | | // another one -24 | | break; -25 | | } - | |_________^ - | - = note: `-D never-loop` implied by `-D warnings` - error: aborting due to previous error(s) error: Could not compile `clippy_tests`.