From a73edc09448ac5b8f9a8a08c8c654db315d54abc Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 31 May 2017 23:22:15 -0500 Subject: [PATCH] add tests and fixes --- clippy_lints/src/loops.rs | 28 ++++-- clippy_tests/examples/for_loop.stderr | 22 +++++ clippy_tests/examples/never_loop.rs | 123 ++++++++++++++++++------ clippy_tests/examples/never_loop.stderr | 95 +++++++++++++++--- 4 files changed, 218 insertions(+), 50 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c4a79203175..6a0e8b70fd6 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -330,6 +330,18 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { 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) @@ -342,9 +354,6 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { "empty `loop {}` detected. You may want to either use `panic!()` or add \ `std::thread::sleep(..);` to the loop body."); } - if never_loop(block, &expr.id) { - 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); @@ -456,14 +465,17 @@ fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool { ExprTupField(ref 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) => [e1, e2].iter().any(|e| contains_continue_expr(e, dest)), - ExprArray(ref es) | - ExprTup(ref es) | - 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)), + 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), @@ -501,8 +513,8 @@ fn loop_exit_expr(expr: &Expr) -> bool { 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) | + 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) | diff --git a/clippy_tests/examples/for_loop.stderr b/clippy_tests/examples/for_loop.stderr index 8ecdc6f5a26..bd4f1bc8ee5 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 5d0c7e3c206..981331a1d6b 100644 --- a/clippy_tests/examples/never_loop.rs +++ b/clippy_tests/examples/never_loop.rs @@ -1,57 +1,118 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(single_match, unused_assignments, unused_variables)] -#![warn(never_loop)] -#![allow(single_match, while_true)] - -fn break_stmt() { - loop { +fn test1() { + let mut x = 0; + loop { // never_loop + x += 1; + if x == 1 { + return + } break; } } -fn conditional_break() { - let mut x = 5; +fn test2() { + let mut x = 0; loop { - x -= 1; + x += 1; if x == 1 { break } } } -fn nested_loop() { - loop { - while true { - break - } +fn test3() { + let mut x = 0; + loop { // never loops + x += 1; break } } -fn if_false() { - let x = 1; - loop { - if x == 1 { - return - } - } -} - -fn match_false() { - let x = 1; +fn test4() { + let mut x = 1; loop { + x += 1; match x { - 1 => return, + 5 => return, _ => (), } } } -fn main() { - break_stmt(); - conditional_break(); - nested_loop(); - if_false(); - match_false(); +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 4f2c0f60227..1ad1ca8de89 100644 --- a/clippy_tests/examples/never_loop.stderr +++ b/clippy_tests/examples/never_loop.stderr @@ -1,26 +1,99 @@ error: this loop never actually loops - --> never_loop.rs:8:5 + --> never_loop.rs:7:5 | -8 | / loop { -9 | | break; -10 | | } +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:24:5 + --> never_loop.rs:28:5 | -24 | / loop { -25 | | while true { -26 | | break -27 | | } -28 | | break -29 | | } +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:47:2 + | +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`.