From 7d35fab304aa154a9f0d8236e3fe176ab68f01ea Mon Sep 17 00:00:00 2001 From: Karim Snj Date: Thu, 1 Mar 2018 23:23:41 +0100 Subject: [PATCH] lint: while loop: detect if no var from the condition is mutated --- clippy_lints/src/loops.rs | 66 +++++++++++++++++++++++++++++++--- tests/ui/infinite_loop.rs | 16 +++++++-- tests/ui/infinite_loop.stderr | 67 +++++++++++++++++++++++++++++------ 3 files changed, 131 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 20623620f3e..51ccbc1297a 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2140,23 +2140,45 @@ fn path_name(e: &Expr) -> Option { None } -fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, _block: &'tcx Block, _expr: &'tcx Expr) { +fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, block: &'tcx Block, expr: &'tcx Expr) { let mut mut_var_visitor = MutableVarsVisitor { cx, ids: HashSet::new(), skip: false, }; - walk_expr(&mut mut_var_visitor, cond); - if !mut_var_visitor.skip && mut_var_visitor.ids.len() == 0 { + walk_expr(&mut mut_var_visitor, expr); + if mut_var_visitor.skip { + return; + } + + if mut_var_visitor.ids.len() == 0 { span_lint( cx, WHILE_IMMUTABLE_CONDITION, cond.span, "all variables in condition are immutable. This might lead to infinite loops.", - ) + ); + return; + } + + let mut use_visitor = MutablyUsedVisitor { + cx, + ids: mut_var_visitor.ids, + any_used: false, + }; + walk_block(&mut use_visitor, block); + if !use_visitor.any_used { + span_lint( + cx, + WHILE_IMMUTABLE_CONDITION, + expr.span, + "Variable in the condition are not mutated in the loop body. This might lead to infinite loops.", + ); } } +/// Collects the set of mutable variable in an expression +/// Stops analysis if a function call is found struct MutableVarsVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, ids: HashSet, @@ -2171,12 +2193,46 @@ fn visit_expr(&mut self, ex: &'tcx Expr) { }, // If there is any fuction/method call… we just stop analysis - ExprCall(_, _) | ExprMethodCall(_, _, _) => self.skip = true, + ExprCall(..) | ExprMethodCall(..) => self.skip = true, _ => walk_expr(self, ex), } } + fn visit_block(&mut self, _b: &'tcx Block) {} + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} + +/// checks within an expression/statement if any of the variables are used mutably +struct MutablyUsedVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + ids: HashSet, + any_used: bool, +} + +impl<'a, 'tcx> Visitor<'tcx> for MutablyUsedVisitor<'a, 'tcx> { + fn visit_expr(&mut self, ex: &'tcx Expr) { + if self.any_used { return; } + + match ex.node { + ExprAddrOf(MutMutable, ref p) | ExprAssign(ref p, _) | ExprAssignOp(_, ref p, _) => + if let Some(id) = check_for_mutability(self.cx, p) { + self.any_used = self.ids.contains(&id); + } + _ => walk_expr(self, ex) + } + } + + fn visit_stmt(&mut self, s: &'tcx Stmt) { + match s.node { + StmtExpr(..) | StmtSemi (..) => walk_stmt(self, s), + _ => {} + } + } + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::None } diff --git a/tests/ui/infinite_loop.rs b/tests/ui/infinite_loop.rs index ecdf42c6d3d..d86e6c042b3 100644 --- a/tests/ui/infinite_loop.rs +++ b/tests/ui/infinite_loop.rs @@ -1,7 +1,8 @@ fn fn_val(i: i32) -> i32 { unimplemented!() } fn fn_constref(i: &i32) -> i32 { unimplemented!() } fn fn_mutref(i: &mut i32) { unimplemented!() } -fn foo() -> i32 { unimplemented!() } +fn fooi() -> i32 { unimplemented!() } +fn foob() -> bool { unimplemented!() } fn immutable_condition() { // Should warn when all vars mentionned are immutable @@ -12,6 +13,8 @@ fn immutable_condition() { let x = 0; while y < 10 && x < 3 { + let mut k = 1; + k += 2; println!("KO - x and y immutable"); } @@ -32,7 +35,11 @@ fn immutable_condition() { println!("OK - mut_cond is mutable"); } - while foo() < x { + while fooi() < x { + println!("OK - Fn call results may vary"); + } + + while foob() { println!("OK - Fn call results may vary"); } @@ -80,6 +87,11 @@ fn used_immutable() { println!("OK - passed by mutable reference"); fn_mutref(&mut i) } + + while i < 3 { + fn_mutref(&mut i); + println!("OK - passed by mutable reference"); + } } fn main() { diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index ddc556f426c..fba90823173 100644 --- a/tests/ui/infinite_loop.stderr +++ b/tests/ui/infinite_loop.stderr @@ -1,22 +1,67 @@ error: all variables in condition are immutable. This might lead to infinite loops. - --> $DIR/infinite_loop.rs:9:11 - | -9 | while y < 10 { - | ^^^^^^ - | - = note: `-D while-immutable-condition` implied by `-D warnings` + --> $DIR/infinite_loop.rs:10:11 + | +10 | while y < 10 { + | ^^^^^^ + | + = note: `-D while-immutable-condition` implied by `-D warnings` error: all variables in condition are immutable. This might lead to infinite loops. - --> $DIR/infinite_loop.rs:14:11 + --> $DIR/infinite_loop.rs:15:11 | -14 | while y < 10 && x < 3 { +15 | while y < 10 && x < 3 { | ^^^^^^^^^^^^^^^ error: all variables in condition are immutable. This might lead to infinite loops. - --> $DIR/infinite_loop.rs:19:11 + --> $DIR/infinite_loop.rs:22:11 | -19 | while !cond { +22 | while !cond { | ^^^^^ -error: aborting due to 3 previous errors +error: Variable in the condition are not mutated in the loop body. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:52:5 + | +52 | / while i < 3 { +53 | | j = 3; +54 | | println!("KO - i not mentionned"); +55 | | } + | |_____^ + +error: Variable in the condition are not mutated in the loop body. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:57:5 + | +57 | / while i < 3 && j > 0 { +58 | | println!("KO - i and j not mentionned"); +59 | | } + | |_____^ + +error: Variable in the condition are not mutated in the loop body. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:61:5 + | +61 | / while i < 3 { +62 | | let mut i = 5; +63 | | fn_mutref(&mut i); +64 | | println!("KO - shadowed"); +65 | | } + | |_____^ + +error: Variable in the condition are not mutated in the loop body. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:76:5 + | +76 | / while i < 3 { +77 | | fn_constref(&i); +78 | | println!("KO - const reference"); +79 | | } + | |_____^ + +error: Variable in the condition are not mutated in the loop body. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:81:5 + | +81 | / while i < 3 { +82 | | fn_val(i); +83 | | println!("KO - passed by value"); +84 | | } + | |_____^ + +error: aborting due to 8 previous errors -- 2.44.0