Merge pull request #2572 from flip1995/immut_while

Fix check of immutable condition in closure

Author: oli-obk

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
@@ -2152,11 +2154,15 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b
 
     let mut delegate = MutVarsDelegate {
         used_mutably: mut_var_visitor.ids,
+        skip: false,
     };
     let def_id = def_id::DefId::local(block.hir_id.owner);
     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,
@@ -2183,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);
+                    },
+                    _ => {},
+                }
             }
         }
     }
@@ -2211,6 +2221,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
 
 struct MutVarsDelegate {
     used_mutably: HashMap<NodeId, bool>,
+    skip: bool,
 }
 
 impl<'tcx> MutVarsDelegate {
@@ -2220,6 +2231,12 @@ impl<'tcx> MutVarsDelegate {
                 if let Some(used) = self.used_mutably.get_mut(&id) {
                     *used = 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.rs

@@ -1,9 +1,13 @@
+
+
+
 fn fn_val(i: i32) -> i32 { unimplemented!() }
 fn fn_constref(i: &i32) -> i32 { unimplemented!() }
 fn fn_mutref(i: &mut i32) { unimplemented!() }
 fn fooi() -> i32 { unimplemented!() }
 fn foob() -> bool { unimplemented!() }
 
+#[allow(many_single_char_names)]
 fn immutable_condition() {
     // Should warn when all vars mentionned are immutable
     let y = 0;
@@ -43,6 +47,14 @@ fn immutable_condition() {
         println!("OK - Fn call results may vary");
     }
 
+    let mut a = 0;
+    let mut c = move || {
+        while a < 5 {
+            a += 1;
+            println!("OK - a is mutable");
+        }
+    };
+    c();
 }
 
 fn unused_var() {

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