From 85bcaad41278644f61155c749cd08a6ace12a8a6 Mon Sep 17 00:00:00 2001 From: Karim Snj Date: Mon, 26 Mar 2018 20:37:34 +0200 Subject: [PATCH 1/2] while_immutable_condition: fix handling of self --- clippy_lints/src/loops.rs | 61 +++++++++++++++++++---------------- tests/ui/infinite_loop.rs | 27 ++++++++++++++++ tests/ui/infinite_loop.stderr | 42 ++++++++++++++++-------- 3 files changed, 89 insertions(+), 41 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 7bf814afa3d..b3f20bfc654 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2140,7 +2140,7 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b return; } - let mut mut_var_visitor = MutableVarsVisitor { + let mut mut_var_visitor = VarCollectorVisitor { cx, ids: HashMap::new(), skip: false, @@ -2150,25 +2150,14 @@ 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 { - mut_spans: mut_var_visitor.ids, + used_mutably: mut_var_visitor.ids, }; 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.mut_spans.iter().any(|(_, v)| v.is_some()) { + if !delegate.used_mutably.iter().any(|(_, v)| *v) { span_lint( cx, WHILE_IMMUTABLE_CONDITION, @@ -2178,21 +2167,34 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b } } -/// Collects the set of mutable variable in an expression +/// Collects the set of variables in an expression /// Stops analysis if a function call is found -struct MutableVarsVisitor<'a, 'tcx: 'a> { +/// Note: In some cases such as `self`, there are no mutable annotation, +/// All variables definition IDs are collected +struct VarCollectorVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, - ids: HashMap>, + ids: HashMap, skip: bool, } -impl<'a, 'tcx> Visitor<'tcx> for MutableVarsVisitor<'a, 'tcx> { +impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> { + fn insert_def_id(&mut self, ex: &'tcx Expr) { + if_chain! { + 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); + } + } + } +} + +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, None); - }, - + ExprPath(_) => self.insert_def_id(ex), // If there is any fuction/method call… we just stop analysis ExprCall(..) | ExprMethodCall(..) => self.skip = true, @@ -2208,15 +2210,18 @@ impl<'a, 'tcx> Visitor<'tcx> for MutableVarsVisitor<'a, 'tcx> { } struct MutVarsDelegate { - mut_spans: HashMap>, + used_mutably: HashMap, } impl<'tcx> MutVarsDelegate { fn update(&mut self, cat: &'tcx Categorization, sp: Span) { - if let Categorization::Local(id) = *cat { - if let Some(span) = self.mut_spans.get_mut(&id) { - *span = Some(sp) - } + match *cat { + Categorization::Local(id) => + if let Some(used) = self.used_mutably.get_mut(&id) { + *used = true; + }, + Categorization::Deref(ref cmt, _) => self.update(&cmt.cat, sp), + _ => {} } } } @@ -2236,7 +2241,7 @@ impl<'tcx> Delegate<'tcx> for MutVarsDelegate { } fn mutate(&mut self, _: NodeId, sp: Span, cmt: cmt<'tcx>, _: MutateMode) { - self.update(&cmt.cat, sp) + self.update(&cmt.cat, sp) } fn decl_without_init(&mut self, _: NodeId, _: Span) {} diff --git a/tests/ui/infinite_loop.rs b/tests/ui/infinite_loop.rs index 560400f359d..30f86129803 100644 --- a/tests/ui/infinite_loop.rs +++ b/tests/ui/infinite_loop.rs @@ -124,9 +124,36 @@ fn internally_mutable() { } } +struct Counter { + count: usize, +} + +impl Counter { + fn inc(&mut self) { + self.count += 1; + } + + fn inc_n(&mut self, n: usize) { + while self.count < n { + self.inc(); + } + println!("OK - self borrowed mutably"); + } + + fn print_n(&self, n: usize) { + while self.count < n { + println!("KO - {} is not mutated", self.count); + } + } +} + fn main() { immutable_condition(); unused_var(); used_immutable(); internally_mutable(); + + let mut c = Counter { count: 0 }; + c.inc_n(5); + c.print_n(2); } diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index 2addd4819e6..ecb6ac58960 100644 --- a/tests/ui/infinite_loop.stderr +++ b/tests/ui/infinite_loop.stderr @@ -1,22 +1,30 @@ -error: all variables in condition are immutable. This either leads to an infinite or to a never running loop. - --> $DIR/infinite_loop.rs:10:11 +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:5 | -10 | while y < 10 { - | ^^^^^^ +10 | / while y < 10 { +11 | | println!("KO - y is immutable"); +12 | | } + | |_____^ | = note: `-D while-immutable-condition` implied by `-D warnings` -error: all variables in condition are immutable. This either leads to an infinite or to a never running loop. - --> $DIR/infinite_loop.rs:15:11 +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:5 | -15 | while y < 10 && x < 3 { - | ^^^^^^^^^^^^^^^ +15 | / while y < 10 && x < 3 { +16 | | let mut k = 1; +17 | | k += 2; +18 | | println!("KO - x and y immutable"); +19 | | } + | |_____^ -error: all variables in condition are immutable. This either leads to an infinite or to a never running loop. - --> $DIR/infinite_loop.rs:22:11 +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:5 | -22 | while !cond { - | ^^^^^ +22 | / while !cond { +23 | | println!("KO - cond immutable"); +24 | | } + | |_____^ 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:5 @@ -63,5 +71,13 @@ error: Variable in the condition are not mutated in the loop body. This either l 84 | | } | |_____^ -error: aborting due to 8 previous errors +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:9 + | +144 | / while self.count < n { +145 | | println!("KO - {} is not mutated", self.count); +146 | | } + | |_________^ + +error: aborting due to 9 previous errors From 737247e50e3709e0b040120dc7604362548bf8a7 Mon Sep 17 00:00:00 2001 From: Karim Snj Date: Mon, 26 Mar 2018 23:24:57 +0200 Subject: [PATCH 2/2] while_immutable_condition: limit suggestion span to condition --- clippy_lints/src/loops.rs | 2 +- tests/ui/infinite_loop.stderr | 79 ++++++++++++----------------------- 2 files changed, 28 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index b3f20bfc654..eaef31b2892 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2161,7 +2161,7 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b span_lint( cx, WHILE_IMMUTABLE_CONDITION, - expr.span, + cond.span, "Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop.", ); } diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index ecb6ac58960..67648dc9110 100644 --- a/tests/ui/infinite_loop.stderr +++ b/tests/ui/infinite_loop.stderr @@ -1,83 +1,58 @@ 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:5 + --> $DIR/infinite_loop.rs:10:11 | -10 | / while y < 10 { -11 | | println!("KO - y is immutable"); -12 | | } - | |_____^ +10 | 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:5 + --> $DIR/infinite_loop.rs:15:11 | -15 | / while y < 10 && x < 3 { -16 | | let mut k = 1; -17 | | k += 2; -18 | | println!("KO - x and y immutable"); -19 | | } - | |_____^ +15 | 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:5 + --> $DIR/infinite_loop.rs:22:11 | -22 | / while !cond { -23 | | println!("KO - cond immutable"); -24 | | } - | |_____^ +22 | 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:5 + --> $DIR/infinite_loop.rs:52:11 | -52 | / while i < 3 { -53 | | j = 3; -54 | | println!("KO - i not mentionned"); -55 | | } - | |_____^ +52 | 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:5 + --> $DIR/infinite_loop.rs:57:11 | -57 | / while i < 3 && j > 0 { -58 | | println!("KO - i and j not mentionned"); -59 | | } - | |_____^ +57 | 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:5 + --> $DIR/infinite_loop.rs:61:11 | -61 | / while i < 3 { -62 | | let mut i = 5; -63 | | fn_mutref(&mut i); -64 | | println!("KO - shadowed"); -65 | | } - | |_____^ +61 | 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:5 + --> $DIR/infinite_loop.rs:76:11 | -76 | / while i < 3 { -77 | | fn_constref(&i); -78 | | println!("KO - const reference"); -79 | | } - | |_____^ +76 | 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:5 + --> $DIR/infinite_loop.rs:81:11 | -81 | / while i < 3 { -82 | | fn_val(i); -83 | | println!("KO - passed by value"); -84 | | } - | |_____^ +81 | 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:9 + --> $DIR/infinite_loop.rs:144:15 | -144 | / while self.count < n { -145 | | println!("KO - {} is not mutated", self.count); -146 | | } - | |_________^ +144 | while self.count < n { + | ^^^^^^^^^^^^^^ error: aborting due to 9 previous errors