fix never_loop

Author: Cameron Steffen

clippy_lints/src/loops.rs

@@ -16,6 +16,7 @@ use rustc::ty::{self, Ty};
 use rustc::ty::subst::{Subst, Substs};
 use rustc_const_eval::ConstContext;
 use std::collections::{HashMap, HashSet};
+use std::iter::{Iterator, once};
 use syntax::ast;
 use syntax::codemap::Span;
 use utils::sugg;
@@ -378,7 +379,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         match expr.node {
             ExprWhile(_, ref block, _) |
             ExprLoop(ref block, _, _) => {
-                if never_loop(block, expr.id) {
+                let mut state = NeverLoopState {
+                    breaks: HashSet::new(),
+                    continues: HashSet::new(),
+                };
+                let may_complete = never_loop_block(block, &mut state);
+                if !may_complete && !state.continues.contains(&expr.id) {
                     span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
                 }
             },
@@ -485,31 +491,34 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
     }
 }
 
-fn never_loop(block: &Block, id: NodeId) -> bool {
-    !contains_continue_block(block, Some(id)) && loop_exit_block(block, &mut vec![id])
+struct NeverLoopState {
+    breaks: HashSet<NodeId>,
+    continues: HashSet<NodeId>,
 }
 
-fn loop_exit_block(block: &Block, loops: &mut Vec<NodeId>) -> bool {
-    block.stmts.iter().take_while(|s| !contains_continue_stmt(s, None)).any(|s| loop_exit_stmt(s, loops))
-        || block.expr.as_ref().map_or(false, |e| loop_exit_expr(e, loops))
+fn never_loop_block(block: &Block, state: &mut NeverLoopState) -> bool {
+    let stmts = block.stmts.iter().map(stmt_to_expr);
+    let expr = once(block.expr.as_ref().map(|p| &**p));
+    let mut iter = stmts.chain(expr).filter_map(|e| e);
+    never_loop_expr_seq(&mut iter, state)
 }
 
-fn loop_exit_stmt(stmt: &Stmt, loops: &mut Vec<NodeId>) -> bool {
+fn stmt_to_expr(stmt: &Stmt) -> Option<&Expr> {
     match stmt.node {
-        StmtSemi(ref e, _) |
-        StmtExpr(ref e, _) => loop_exit_expr(e, loops),
-        StmtDecl(ref d, _) => loop_exit_decl(d, loops),
+        StmtSemi(ref e, ..) |
+        StmtExpr(ref e, ..) => Some(e),
+        StmtDecl(ref d, ..) => decl_to_expr(d),
     }
 }
 
-fn loop_exit_decl(decl: &Decl, loops: &mut Vec<NodeId>) -> bool {
+fn decl_to_expr(decl: &Decl) -> Option<&Expr> {
     match decl.node {
-        DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| loop_exit_expr(e, loops)),
-        _ => false,
+        DeclLocal(ref local) => local.init.as_ref().map(|p| &**p),
+        _ => None,
     }
 }
 
-fn loop_exit_expr(expr: &Expr, loops: &mut Vec<NodeId>) -> bool {
+fn never_loop_expr(expr: &Expr, state: &mut NeverLoopState) -> bool {
     match expr.node {
         ExprBox(ref e) |
         ExprUnary(_, ref e) |
@@ -518,38 +527,73 @@ fn loop_exit_expr(expr: &Expr, loops: &mut Vec<NodeId>) -> bool {
         ExprField(ref e, _) |
         ExprTupField(ref e, _) |
         ExprAddrOf(_, ref e) |
-        ExprRepeat(ref e, _) => loop_exit_expr(e, loops),
+        ExprRepeat(ref e, _) => never_loop_expr(e, state),
         ExprArray(ref es) |
         ExprMethodCall(_, _, ref es) |
-        ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e, loops)),
-        ExprCall(ref e, ref es) => loop_exit_expr(e, loops) || es.iter().any(|e| loop_exit_expr(e, loops)),
+        ExprTup(ref es) => never_loop_expr_seq(&mut es.iter(), state),
+        ExprCall(ref e, ref es) => never_loop_expr_seq(&mut once(&**e).chain(es.iter()), state),
         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, loops)),
-        ExprIf(ref e, ref e2, ref e3) => loop_exit_expr(e, loops)
-            || e3.as_ref().map_or(false, |e3| loop_exit_expr(e3, loops)) && loop_exit_expr(e2, loops),
+        ExprIndex(ref e1, ref e2) => never_loop_expr_seq(&mut [&**e1, &**e2].iter().cloned(), state),
+        ExprIf(ref e, ref e2, ref e3) => {
+            let e1 = never_loop_expr(e, state);
+            let e2 = never_loop_expr(e2, state);
+            match *e3 {
+                Some(ref e3) => {
+                    let e3 = never_loop_expr(e3, state);
+                    e1 && (e2 || e3)
+                },
+                None => e1,
+            }
+        },
         ExprLoop(ref b, _, _) => {
-            loops.push(expr.id);
-            let val = loop_exit_block(b, loops);
-            loops.pop();
-            val
+            let block_may_complete = never_loop_block(b, state);
+            let has_break = state.breaks.remove(&expr.id);
+            state.continues.remove(&expr.id);
+            block_may_complete || has_break
         },
         ExprWhile(ref e, ref b, _) => {
-            loops.push(expr.id);
-            let val = loop_exit_expr(e, loops) || loop_exit_block(b, loops);
-            loops.pop();
-            val
+            let e = never_loop_expr(e, state);
+            let block_may_complete = never_loop_block(b, state);
+            let has_break = state.breaks.remove(&expr.id);
+            let has_continue = state.continues.remove(&expr.id);
+            e && (block_may_complete || has_break || has_continue)
         },
-        ExprMatch(ref e, ref arms, _) => loop_exit_expr(e, loops) || arms.iter().all(|a| loop_exit_expr(&a.body, loops)),
-        ExprBlock(ref b) => loop_exit_block(b, loops),
-        ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| loops.iter().skip(1).all(|&id2| id != id2)),
-        ExprBreak(d, _) => d.target_id.opt_id().map_or(false, |id| loops[0] == id),
-        ExprRet(_) => true,
-        _ => false,
+        ExprMatch(ref e, ref arms, _) => {
+            let e = never_loop_expr(e, state);
+            let arms = never_loop_expr_branch(&mut arms.iter().map(|a| &*a.body), state);
+            e && arms
+        },
+        ExprBlock(ref b) => never_loop_block(b, state),
+        ExprAgain(d) => {
+            let id = d.target_id.opt_id().expect("continue is missing target id");
+            state.continues.insert(id);
+            false
+        },
+        ExprBreak(d, _) => {
+            let id = d.target_id.opt_id().expect("break is missing target id");
+            state.breaks.insert(id);
+            false
+        },
+        ExprRet(ref e) => {
+            if let Some(ref e) = *e {
+                never_loop_expr(e, state);
+            }
+            false
+        },
+        _ => true,
     }
 }
 
+fn never_loop_expr_seq<'a, T: Iterator<Item=&'a Expr>>(es: &mut T, state: &mut NeverLoopState) -> bool {
+    es.map(|e| never_loop_expr(e, state)).fold(true, |a, b| a && b)
+}
+
+fn never_loop_expr_branch<'a, T: Iterator<Item=&'a Expr>>(e: &mut T, state: &mut NeverLoopState) -> bool {
+    e.map(|e| never_loop_expr(e, state)).fold(false, |a, b| a || b)
+}
+
 fn check_for_loop<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     pat: &'tcx Pat,

tests/ui/never_loop.stderr

@@ -68,3 +68,15 @@ error: this loop never actually loops
 103 | |     }
     | |_____^
 
+error: this loop never actually loops
+   --> $DIR/never_loop.rs:144:5
+    |
+144 | /     'outer: while a { // never loops
+145 | |         while a {
+146 | |             if a {
+147 | |                 a = false;
+...   |
+151 | |         break 'outer;
+152 | |     }
+    | |_____^
+