diff --git a/README.md b/README.md index c5b28812e944..b650c64eb218 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/attrs.rs b/clippy_lints/src/attrs.rs index 1cca82d97954..88f0ef0955c3 100644 --- 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 index b869948c324a..729c3eaf86e6 100644 --- 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 index e212aaeaf7b8..fb0114f8f683 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 eb4708498b6b..fc9bcbab73af 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -286,15 +286,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 @@ -302,8 +300,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)] @@ -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 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), + 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 index 66c42d50e19a..e752ffd2362c 100644 --- 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 index 90dae2134d55..981331a1d6bd 100644 --- a/clippy_tests/examples/never_loop.rs +++ b/clippy_tests/examples/never_loop.rs @@ -1,34 +1,118 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(single_match, unused_assignments, unused_variables)] -#![warn(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 +fn test1() { + let mut x = 0; + loop { // never_loop + x += 1; if x == 1 { - break; - } - } - - loop { - loop { - // another one - break; + return } break; } +} +fn test2() { + let mut x = 0; loop { - loop { - if x == 1 { return; } + x += 1; + if x == 1 { + break } } } + +fn test3() { + let mut x = 0; + loop { // never loops + x += 1; + break + } +} + +fn test4() { + let mut x = 1; + loop { + x += 1; + match x { + 5 => return, + _ => (), + } + } +} + +fn test5() { + let i = 0; + loop { // never loops + while i == 0 { // never loops + break + } + return + } +} + +fn test6() { + let mut x = 0; + 'outer: loop { // never loops + x += 1; + loop { // never loops + if x == 5 { break } + continue 'outer + } + return + } +} + +fn test7() { + let mut x = 0; + loop { + x += 1; + match x { + 1 => continue, + _ => (), + } + return + } +} + +fn test8() { + let mut x = 0; + loop { + x += 1; + match x { + 5 => return, + _ => continue, + } + } +} + +fn test9() { + let x = Some(1); + while let Some(y) = x { // never loops + return + } +} + +fn test10() { + for x in 0..10 { // never loops + match x { + 1 => break, + _ => return, + } + } +} + +fn main() { + test1(); + test2(); + test3(); + test4(); + test5(); + test6(); + test7(); + test8(); + test9(); + test10(); +} + diff --git a/clippy_tests/examples/never_loop.stderr b/clippy_tests/examples/never_loop.stderr index 28e172bbd01f..1ad1ca8de89c 100644 --- a/clippy_tests/examples/never_loop.stderr +++ b/clippy_tests/examples/never_loop.stderr @@ -1,39 +1,99 @@ error: this loop never actually loops - --> never_loop.rs:8:5 + --> never_loop.rs:7:5 | -8 | / loop { -9 | | println!("This is only ever printed once"); -10 | | break; -11 | | } +7 | / loop { // never_loop +8 | | x += 1; +9 | | if x == 1 { +10 | | return +11 | | } +12 | | break; +13 | | } | |_____^ | = note: `-D never-loop` implied by `-D warnings` error: this loop never actually loops - --> never_loop.rs:21:5 + --> never_loop.rs:28:5 | -21 | / loop { -22 | | loop { -23 | | // another one -24 | | break; -25 | | } -26 | | break; -27 | | } +28 | / loop { // never loops +29 | | x += 1; +30 | | break +31 | | } | |_____^ | = note: `-D never-loop` implied by `-D warnings` error: this loop never actually loops - --> never_loop.rs:22:9 + --> never_loop.rs:47:2 | -22 | / loop { -23 | | // another one -24 | | break; -25 | | } +47 | / loop { // never loops +48 | | while i == 0 { // never loops +49 | | break +50 | | } +51 | | return +52 | | } + | |__^ + | + = note: `-D never-loop` implied by `-D warnings` + +error: this loop never actually loops + --> never_loop.rs:48:9 + | +48 | / while i == 0 { // never loops +49 | | break +50 | | } | |_________^ | = note: `-D never-loop` implied by `-D warnings` +error: this loop never actually loops + --> never_loop.rs:57:5 + | +57 | / 'outer: loop { // never loops +58 | | x += 1; +59 | | loop { // never loops +60 | | if x == 5 { break } +... | +63 | | return +64 | | } + | |__^ + | + = note: `-D never-loop` implied by `-D warnings` + +error: this loop never actually loops + --> never_loop.rs:59:3 + | +59 | / loop { // never loops +60 | | if x == 5 { break } +61 | | continue 'outer +62 | | } + | |___^ + | + = note: `-D never-loop` implied by `-D warnings` + +error: this loop never actually loops + --> never_loop.rs:92:5 + | +92 | / while let Some(y) = x { // never loops +93 | | return +94 | | } + | |_____^ + | + = note: `-D never-loop` implied by `-D warnings` + +error: this loop never actually loops + --> never_loop.rs:98:5 + | +98 | / for x in 0..10 { // never loops +99 | | match x { +100 | | 1 => break, +101 | | _ => return, +102 | | } +103 | | } + | |_____^ + | + = note: `-D never-loop` implied by `-D warnings` + error: aborting due to previous error(s) error: Could not compile `clippy_tests`.