From d458f22d89eff815b2a0f2cf3d1655d393b26714 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sun, 25 Mar 2018 17:23:31 +0200 Subject: [PATCH 1/2] Fix check of immutable condition in closure --- clippy_lints/src/loops.rs | 19 ++++++++++++++++++- tests/ui/infinite_loop.rs | 12 ++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index eaef31b2892..94adb4d03a3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2150,8 +2150,20 @@ 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, }; let def_id = def_id::DefId::local(block.hir_id.owner); let region_scope_tree = &cx.tcx.region_scope_tree(def_id); @@ -2194,7 +2206,10 @@ 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(_) => self.insert_def_id(ex), + ExprPath(_) => if let Some(node_id) = check_for_mutability(self.cx, ex) { + self.ids.insert(node_id, false); + }, + // If there is any fuction/method call… we just stop analysis ExprCall(..) | ExprMethodCall(..) => self.skip = true, @@ -2211,6 +2226,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> { struct MutVarsDelegate { used_mutably: HashMap, + skip: bool, } impl<'tcx> MutVarsDelegate { @@ -2220,6 +2236,7 @@ impl<'tcx> MutVarsDelegate { if let Some(used) = self.used_mutably.get_mut(&id) { *used = true; }, + Categorization::Upvar(_) => skip = true, Categorization::Deref(ref cmt, _) => self.update(&cmt.cat, sp), _ => {} } diff --git a/tests/ui/infinite_loop.rs b/tests/ui/infinite_loop.rs index 30f86129803..4029f9a9b29 100644 --- a/tests/ui/infinite_loop.rs +++ b/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() { From 7d290751321f9dcaa91cf4a925e7d68d3ce68817 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 26 Mar 2018 12:32:21 +0200 Subject: [PATCH 2/2] Skip the mutation in while body case for closures --- clippy_lints/src/loops.rs | 38 +++++++++++++++++------------------ tests/ui/infinite_loop.stderr | 36 ++++++++++++++++----------------- 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 94adb4d03a3..a76b46a1bcb 100644 --- a/clippy_lints/src/loops.rs +++ b/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), _ => {} } diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index 67648dc9110..0bf14bb723b 100644 --- a/tests/ui/infinite_loop.stderr +++ b/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