WIP: Find binding or assignment within outer loop

clippy_lints/src/lib.rs

@@ -253,7 +253,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box shadow::Pass);
     reg.register_late_lint_pass(box types::LetPass);
     reg.register_late_lint_pass(box types::UnitCmp);
-    reg.register_late_lint_pass(box loops::Pass::default());
+    reg.register_late_lint_pass(box loops::Pass);
     reg.register_late_lint_pass(box lifetimes::LifetimePass);
     reg.register_late_lint_pass(box entry::HashMapLint);
     reg.register_late_lint_pass(box ranges::StepByZero);

clippy_lints/src/loops.rs

@@ -304,10 +304,8 @@ declare_lint! {
     "any loop that will always `break` or `return`"
 }
 
-#[derive(Copy, Clone, Default)]
-pub struct Pass {
-    loop_count: usize,
-}
+#[derive(Copy, Clone)]
+pub struct Pass;
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
@@ -329,15 +327,6 @@ impl LintPass for Pass {
 }
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
-    fn check_expr_post(&mut self, _: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
-        match expr.node {
-            ExprWhile(..) | ExprLoop(..) => {
-                self.loop_count -= 1;
-            },
-            _ => (),
-        }
-    }
-
     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);
@@ -347,7 +336,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         match expr.node {
             ExprWhile(_, ref block, _) |
             ExprLoop(ref block, _, _) => {
-                self.loop_count += 1;
                 if never_loop(block, &expr.id) {
                     span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
                 }
@@ -410,10 +398,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                     &ExprMethodCall(ref method_path, _, ref method_args)) = (pat, &match_expr.node) {
                 let iter_expr = &method_args[0];
                 let lhs_constructor = last_path_segment(qpath);
-                if self.loop_count < 2 && method_path.name == "next" &&
-                   match_trait_method(cx, match_expr, &paths::ITERATOR) &&
+                if method_path.name == "next" && match_trait_method(cx, match_expr, &paths::ITERATOR) &&
                    lhs_constructor.name == "Some" && !is_refutable(cx, &pat_args[0]) &&
-                   !is_iterator_used_after_while_let(cx, iter_expr) {
+                   !is_iterator_used_after_while_let(cx, iter_expr) && !is_nested(cx, expr, &method_args[0]) {
                     let iterator = snippet(cx, method_args[0].span, "_");
                     let loop_var = snippet(cx, pat_args[0].span, "_");
                     span_lint_and_sugg(cx,
@@ -939,6 +926,15 @@ fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool {
     }
 }
 
+fn match_var(expr: &Expr, var: Name) -> bool {
+    if let ExprPath(QPath::Resolved(None, ref path)) = expr.node {
+        if path.segments.len() == 1 && path.segments[0].name == var {
+            return true
+        }
+    }
+    false
+}
+
 struct UsedVisitor {
     var: ast::Name, // var to look for
     used: bool, // has the var been used otherwise?
@@ -946,15 +942,13 @@ struct UsedVisitor {
 
 impl<'tcx> Visitor<'tcx> for UsedVisitor {
     fn visit_expr(&mut self, expr: &'tcx Expr) {
-        if let ExprPath(QPath::Resolved(None, ref path)) = expr.node {
-            if path.segments.len() == 1 && path.segments[0].name == self.var {
-                self.used = true;
-                return;
-            }
+        if match_var(expr, self.var) {
+            self.used = true;
+            return;
         }
-
         walk_expr(self, expr);
     }
+
     fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
         NestedVisitorMap::None
     }
@@ -1328,3 +1322,110 @@ fn is_conditional(expr: &Expr) -> bool {
         _ => false,
     }
 }
+
+fn is_nested(cx: &LateContext, match_expr: &Expr, iter_expr: &Expr) -> bool {
+    if_let_chain! {[
+        let Some(loop_block) = get_enclosing_block(cx, match_expr.id),
+        let Some(map::Node::NodeExpr(loop_expr)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(loop_block.id)),
+        let Some(scope) = get_enclosing_block(cx, loop_expr.id)
+    ], {
+        return is_loop_nested(cx, scope, loop_expr.id, iter_expr)
+    }}
+    false
+}
+
+fn is_loop_nested(cx: &LateContext, scope: &Block, expr_id: NodeId, iter_expr: &Expr) -> bool {
+    let mut b = scope;
+    let mut e = expr_id;
+    if let Some(name) = path_name(iter_expr) {
+        loop {
+            if b.stmts.iter().take_while(|stmt| !is_expr_stmt(stmt, e)).any(|stmt|
+                    is_binding_or_assignment(stmt, name)) {
+                return false;
+            }
+            if let Some(map::Node::NodeExpr(outer)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(scope.id)) {
+                if let ExprLoop(..) = outer.node {
+                    return true;
+                }
+                e = outer.id;
+                if let Some(eb) = get_enclosing_block(cx, e) {
+                    b = eb;
+                } else {
+                    return false;
+                }
+            } else {
+                return false;
+            }
+        }
+    }
+    true
+}
+
+fn path_name(e: &Expr) -> Option<Name> {
+    if let ExprPath(QPath::Resolved(_, ref path)) = e.node {
+        let segments = &path.segments;
+        if segments.len() == 1 {
+            return Some(segments[0].name);
+        }
+    };
+    None
+}
+
+fn is_binding_or_assignment(stmt: &Stmt, name: Name) -> bool {
+    match stmt.node {
+        StmtExpr(ref e, _) | StmtSemi(ref e, _) => contains_assignment(e, name),
+        StmtDecl(ref decl, _) => is_binding(decl, name)
+    }
+}
+
+struct AssignmentVisitor {
+    var: ast::Name, // var to look for
+    assigned: bool, // has the var been assigned?
+}
+
+impl<'tcx> Visitor<'tcx> for AssignmentVisitor {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        match expr.node {
+            ExprAssign(ref path, _) |
+            ExprAssignOp(_, ref path, _) => if match_var(path, self.var) {
+                self.assigned = true;
+            }
+            ExprLoop(..) |
+            ExprIf(..) |
+            ExprWhile(..) => (),
+            _ => walk_expr(self, expr)
+        }
+    }
+
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::None
+    }
+}
+
+fn contains_assignment(e: &Expr, name: Name) -> bool {
+    let mut av = AssignmentVisitor { var: name, assigned: false };
+    walk_expr(&mut av, e);
+    av.assigned
+}
+
+fn is_binding(decl: &Decl, name: Name) -> bool {
+    match decl.node {
+        DeclLocal(ref local) => {
+            !local.pat.walk(&mut |p: &Pat| {
+                if let PatKind::Binding(_, _, span_name, _) = p.node {
+                    name == span_name.node
+                } else {
+                    false
+                }
+            })
+        },
+        _ => false
+    }
+}
+
+fn is_expr_stmt(stmt: &Stmt, expr_id: NodeId) -> bool {
+    match stmt.node {
+        StmtExpr(ref e, _) | StmtSemi(ref e, _) => e.id == expr_id,
+        _ => false
+    }
+}

tests/ui/while_loop.rs

@@ -174,7 +174,13 @@ fn refutable() {
 
     let mut y = a.iter();
     for _ in 0..2 {
-        while let Some(v) = y.next() {
+        while let Some(v) = y.next() { // y is reused, don't lint
+        }
+    }
+
+    loop {
+        let mut y = a.iter();
+        while let Some(v) = y.next() { // use a for loop here
         }
     }
 }

tests/ui/while_loop.stderr

@@ -103,5 +103,12 @@ error: empty `loop {}` detected. You may want to either use `panic!()` or add `s
     |
     = note: `-D empty-loop` implied by `-D warnings`
 
-error: aborting due to 10 previous errors
+error: this loop could be written as a `for` loop
+   --> while_loop.rs:177:9
+    |
+177 | /         while let Some(v) = y.next() {
+178 | |         }
+    | |_________^ help: try: `for v in y { .. }`
+
+error: aborting due to 11 previous errors