Merge pull request #2119 from camsteffen/never_loop

Another never_loop fix

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,41 +491,35 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
     }
 }
 
-fn never_loop(block: &Block, id: NodeId) -> bool {
+struct NeverLoopState {
-    !contains_continue_block(block, Some(id)) && loop_exit_block(block, &mut vec![id])
+    breaks: HashSet<NodeId>,
+    continues: HashSet<NodeId>,
 }
 
-fn contains_continue_block(block: &Block, dest: Option<NodeId>) -> bool {
+fn never_loop_block(block: &Block, state: &mut NeverLoopState) -> bool {
-    block.stmts.iter().any(|e| contains_continue_stmt(e, dest)) ||
+    let stmts = block.stmts.iter().map(stmt_to_expr);
-        block.expr.as_ref().map_or(
+    let expr = once(block.expr.as_ref().map(|p| &**p));
-            false,
+    let mut iter = stmts.chain(expr).filter_map(|e| e);
-            |e| contains_continue_expr(e, dest),
+    never_loop_expr_seq(&mut iter, state)
-        )
 }
 
-fn contains_continue_stmt(stmt: &Stmt, dest: Option<NodeId>) -> bool {
+fn stmt_to_expr(stmt: &Stmt) -> Option<&Expr> {
     match stmt.node {
-        StmtSemi(ref e, _) |
+        StmtSemi(ref e, ..) |
-        StmtExpr(ref e, _) => contains_continue_expr(e, dest),
+        StmtExpr(ref e, ..) => Some(e),
-        StmtDecl(ref d, _) => contains_continue_decl(d, dest),
+        StmtDecl(ref d, ..) => decl_to_expr(d),
     }
 }
 
-fn contains_continue_decl(decl: &Decl, dest: Option<NodeId>) -> bool {
+fn decl_to_expr(decl: &Decl) -> Option<&Expr> {
     match decl.node {
-        DeclLocal(ref local) => {
+        DeclLocal(ref local) => local.init.as_ref().map(|p| &**p),
-            local.init.as_ref().map_or(
+        _ => None,
-                false,
-                |e| contains_continue_expr(e, dest),
-            )
-        },
-        _ => false,
     }
 }
 
-fn contains_continue_expr(expr: &Expr, dest: Option<NodeId>) -> bool {
+fn never_loop_expr(expr: &Expr, state: &mut NeverLoopState) -> bool {
     match expr.node {
-        ExprRet(Some(ref e)) |
         ExprBox(ref e) |
         ExprUnary(_, ref e) |
         ExprCast(ref e, _) |
@@ -527,100 +527,73 @@ fn contains_continue_expr(expr: &Expr, dest: Option<NodeId>) -> bool {
         ExprField(ref e, _) |
         ExprTupField(ref e, _) |
         ExprAddrOf(_, ref e) |
-        ExprRepeat(ref e, _) => contains_continue_expr(e, dest),
+        ExprRepeat(ref e, _) => never_loop_expr(e, state),
         ExprArray(ref es) |
         ExprMethodCall(_, _, ref es) |
-        ExprTup(ref es) => es.iter().any(|e| contains_continue_expr(e, dest)),
+        ExprTup(ref es) => never_loop_expr_seq(&mut es.iter(), state),
-        ExprCall(ref e, ref es) => {
+        ExprCall(ref e, ref es) => never_loop_expr_seq(&mut once(&**e).chain(es.iter()), state),
-            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)),
+        ExprIndex(ref e1, ref e2) => never_loop_expr_seq(&mut [&**e1, &**e2].iter().cloned(), state),
         ExprIf(ref e, ref e2, ref e3) => {
-            [e, e2].iter().chain(e3.as_ref().iter()).any(|e| {
+            let e1 = never_loop_expr(e, state);
-                contains_continue_expr(e, dest)
+            let e2 = never_loop_expr(e2, state);
-            })
+            match *e3 {
+                Some(ref e3) => {
+                    let e3 = never_loop_expr(e3, state);
+                    e1 && (e2 || e3)
+                },
+                None => e1,
+            }
         },
-        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) |
-        ExprLoop(ref block, ..) => contains_continue_block(block, dest),
-        ExprStruct(_, _, ref base) => {
-            base.as_ref().map_or(
-                false,
-                |e| contains_continue_expr(e, dest),
-            )
-        },
-        ExprAgain(d) => dest.map_or(true, |dest| d.target_id.opt_id().map_or(false, |id| id == dest)),
-        _ => false,
-    }
-}
-
-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 loop_exit_stmt(stmt: &Stmt, loops: &mut Vec<NodeId>) -> bool {
-    match stmt.node {
-        StmtSemi(ref e, _) |
-        StmtExpr(ref e, _) => loop_exit_expr(e, loops),
-        StmtDecl(ref d, _) => loop_exit_decl(d, loops),
-    }
-}
-
-fn loop_exit_decl(decl: &Decl, loops: &mut Vec<NodeId>) -> bool {
-    match decl.node {
-        DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| loop_exit_expr(e, loops)),
-        _ => false,
-    }
-}
-
-fn loop_exit_expr(expr: &Expr, loops: &mut Vec<NodeId>) -> 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, loops),
-        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)),
-        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),
         ExprLoop(ref b, _, _) => {
-            loops.push(expr.id);
+            let block_may_complete = never_loop_block(b, state);
-            let val = loop_exit_block(b, loops);
+            let has_break = state.breaks.remove(&expr.id);
-            loops.pop();
+            state.continues.remove(&expr.id);
-            val
+            block_may_complete || has_break
         },
         ExprWhile(ref e, ref b, _) => {
-            loops.push(expr.id);
+            let e = never_loop_expr(e, state);
-            let val = loop_exit_expr(e, loops) || loop_exit_block(b, loops);
+            let block_may_complete = never_loop_block(b, state);
-            loops.pop();
+            let has_break = state.breaks.remove(&expr.id);
-            val
+            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)),
+        ExprMatch(ref e, ref arms, _) => {
-        ExprBlock(ref b) => loop_exit_block(b, loops),
+            let e = never_loop_expr(e, state);
-        ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| loops.iter().skip(1).all(|&id2| id != id2)),
+            let arms = never_loop_expr_branch(&mut arms.iter().map(|a| &*a.body), state);
-        ExprBreak(d, _) => d.target_id.opt_id().map_or(false, |id| loops[0] == id),
+            e && arms
-        ExprRet(_) => true,
+        },
-        _ => false,
+        ExprBlock(ref b) => never_loop_block(b, state),
+        ExprAgain(d) => {
+            let id = d.target_id.opt_id().expect("target id can only be missing in the presence of compilation errors");
+            state.continues.insert(id);
+            false
+        },
+        ExprBreak(d, _) => {
+            let id = d.target_id.opt_id().expect("target id can only be missing in the presence of compilation errors");
+            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.rs

@@ -139,6 +139,19 @@ pub fn test13() {
     }
 }
 
+pub fn test14() {
+    let mut a = true;
+    'outer: while a { // never loops
+        while a {
+            if a {
+                a = false;
+                continue
+            }
+        }
+        break 'outer;
+    }
+}
+
 fn main() {
     test1();
     test2();
@@ -153,5 +166,6 @@ fn main() {
     test11(|| 0);
     test12(true, false);
     test13();
+    test14();
 }
 

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 | |     }
+    | |_____^
+