Skip the mutation in while body case for closures

Author: flip1995

clippy_lints/src/loops.rs

@@ -347,7 +347,9 @@ declare_lint! {
 /// **Why is this bad?** If the condition is unchanged, entering the body of the loop
 /// will lead to an infinite loop.
 ///
-/// **Known problems:** None
+/// **Known problems:** If the `while`-loop is in a closure, the check for mutation of the
+/// condition variables in the body can cause false negatives. For example when only `Upvar` `a` is
+/// in the condition and only `Upvar` `b` gets mutated in the body, the lint will not trigger.
 ///
 /// **Example:**
 /// ```rust
@@ -2150,17 +2152,6 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b
         return;
     }
 
-    if mut_var_visitor.ids.is_empty() {
-        span_lint(
-            cx,
-            WHILE_IMMUTABLE_CONDITION,
-            cond.span,
-            "all variables in condition are immutable. This either leads to an infinite or to a never running loop.",
-        );
-        return;
-    }
-
-
     let mut delegate = MutVarsDelegate {
         used_mutably: mut_var_visitor.ids,
         skip: false,
@@ -2169,6 +2160,9 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b
     let region_scope_tree = &cx.tcx.region_scope_tree(def_id);
     ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).walk_expr(expr);
 
+    if delegate.skip {
+        return;
+    }
     if !delegate.used_mutably.iter().any(|(_, v)| *v) {
         span_lint(
             cx,
@@ -2195,9 +2189,13 @@ impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> {
             if let ExprPath(ref qpath) = ex.node;
             if let QPath::Resolved(None, _) = *qpath;
             let def = self.cx.tables.qpath_def(qpath, ex.hir_id);
-            if let Def::Local(node_id) = def;
             then {
-                self.ids.insert(node_id, false);
+                match def {
+                    Def::Local(node_id) | Def::Upvar(node_id, ..) => {
+                        self.ids.insert(node_id, false);
+                    },
+                    _ => {},
+                }
             }
         }
     }
@@ -2206,10 +2204,7 @@ impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> {
 impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
     fn visit_expr(&mut self, ex: &'tcx Expr) {
         match ex.node {
-            ExprPath(_) => if let Some(node_id) = check_for_mutability(self.cx, ex) {
-                self.ids.insert(node_id, false);
-            },
-
+            ExprPath(_) => self.insert_def_id(ex),
             // If there is any fuction/method call… we just stop analysis
             ExprCall(..) | ExprMethodCall(..) => self.skip = true,
 
@@ -2236,7 +2231,12 @@ impl<'tcx> MutVarsDelegate {
                 if let Some(used) = self.used_mutably.get_mut(&id) {
                     *used = true;
                 },
-            Categorization::Upvar(_) => skip = true,
+            Categorization::Upvar(_) => {
+                //FIXME: This causes false negatives. We can't get the `NodeId` from
+                //`Categorization::Upvar(_)`. So we search for any `Upvar`s in the
+                //`while`-body, not just the ones in the condition.
+                self.skip = true
+            },
             Categorization::Deref(ref cmt, _) => self.update(&cmt.cat, sp),
             _ => {}
         }

tests/ui/infinite_loop.stderr

@@ -1,57 +1,57 @@
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:10:11
+  --> $DIR/infinite_loop.rs:14:11
    |
-10 |     while y < 10 {
+14 |     while y < 10 {
    |           ^^^^^^
    |
    = note: `-D while-immutable-condition` implied by `-D warnings`
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:15:11
+  --> $DIR/infinite_loop.rs:19:11
    |
-15 |     while y < 10 && x < 3 {
+19 |     while y < 10 && x < 3 {
    |           ^^^^^^^^^^^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:22:11
+  --> $DIR/infinite_loop.rs:26:11
    |
-22 |     while !cond {
+26 |     while !cond {
    |           ^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:52:11
+  --> $DIR/infinite_loop.rs:64:11
    |
-52 |     while i < 3 {
+64 |     while i < 3 {
    |           ^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:57:11
+  --> $DIR/infinite_loop.rs:69:11
    |
-57 |     while i < 3 && j > 0 {
+69 |     while i < 3 && j > 0 {
    |           ^^^^^^^^^^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:61:11
+  --> $DIR/infinite_loop.rs:73:11
    |
-61 |     while i < 3 {
+73 |     while i < 3 {
    |           ^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:76:11
+  --> $DIR/infinite_loop.rs:88:11
    |
-76 |     while i < 3 {
+88 |     while i < 3 {
    |           ^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-  --> $DIR/infinite_loop.rs:81:11
+  --> $DIR/infinite_loop.rs:93:11
    |
-81 |     while i < 3 {
+93 |     while i < 3 {
    |           ^^^^^
 
 error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.
-   --> $DIR/infinite_loop.rs:144:15
+   --> $DIR/infinite_loop.rs:156:15
     |
-144 |         while self.count < n {
+156 |         while self.count < n {
     |               ^^^^^^^^^^^^^^
 
 error: aborting due to 9 previous errors