Merge pull request #2483 from kimsnj/infinite_loop

immutable while condition

clippy_lints/src/lib.rs

@@ -521,6 +521,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         loops::UNUSED_COLLECT,
         loops::WHILE_LET_LOOP,
         loops::WHILE_LET_ON_ITERATOR,
+        loops::WHILE_IMMUTABLE_CONDITION,
         map_clone::MAP_CLONE,
         matches::MATCH_AS_REF,
         matches::MATCH_BOOL,

clippy_lints/src/loops.rs

@@ -343,6 +343,27 @@ declare_lint! {
     "for loop over a range where one of the bounds is a mutable variable"
 }
 
+/// **What it does:** Checks whether variables used within while loop condition
+/// can be (and are) mutated in the body.
+///
+/// **Why is this bad?** If the condition is unchanged, entering the body of the loop
+/// will lead to an infinite loop.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+/// ```rust
+/// let i = 0;
+/// while i > 10 {
+///    println!("let me loop forever!");
+/// }
+/// ```
+declare_lint! {
+    pub WHILE_IMMUTABLE_CONDITION,
+    Warn,
+    "variables used within while expression are not mutated in the body"
+}
+
 #[derive(Copy, Clone)]
 pub struct Pass;
 
@@ -364,7 +385,8 @@ impl LintPass for Pass {
             WHILE_LET_ON_ITERATOR,
             FOR_KV_MAP,
             NEVER_LOOP,
-            MUT_RANGE_BOUND
+            MUT_RANGE_BOUND,
+            WHILE_IMMUTABLE_CONDITION,
         )
     }
 }
@@ -469,6 +491,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                 }
             }
         }
+
+        // check for while loops which conditions never change
+        if let ExprWhile(ref cond, ref block, _) = expr.node {
+            check_infinite_loop(cx, cond, block, expr);
+        }
     }
 
     fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
@@ -1372,14 +1399,14 @@ fn check_for_loop_over_map_kv<'a, 'tcx>(
     }
 }
 
-struct MutateDelegate {
+struct MutatePairDelegate {
     node_id_low: Option<NodeId>,
     node_id_high: Option<NodeId>,
     span_low: Option<Span>,
     span_high: Option<Span>,
 }
 
-impl<'tcx> Delegate<'tcx> for MutateDelegate {
+impl<'tcx> Delegate<'tcx> for MutatePairDelegate {
     fn consume(&mut self, _: NodeId, _: Span, _: cmt<'tcx>, _: ConsumeMode) {}
 
     fn matched_pat(&mut self, _: &Pat, _: cmt<'tcx>, _: MatchMode) {}
@@ -1413,7 +1440,7 @@ impl<'tcx> Delegate<'tcx> for MutateDelegate {
     fn decl_without_init(&mut self, _: NodeId, _: Span) {}
 }
 
-impl<'tcx> MutateDelegate {
+impl<'tcx> MutatePairDelegate {
     fn mutation_span(&self) -> (Option<Span>, Option<Span>) {
         (self.span_low, self.span_high)
     }
@@ -1472,7 +1499,7 @@ fn check_for_mutability(cx: &LateContext, bound: &Expr) -> Option<NodeId> {
 }
 
 fn check_for_mutation(cx: &LateContext, body: &Expr, bound_ids: &[Option<NodeId>]) -> (Option<Span>, Option<Span>) {
-    let mut delegate = MutateDelegate {
+    let mut delegate = MutatePairDelegate {
         node_id_low: bound_ids[0],
         node_id_high: bound_ids[1],
         span_low: None,
@@ -2113,3 +2140,106 @@ fn path_name(e: &Expr) -> Option<Name> {
     };
     None
 }
+
+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: HashMap::new(),
+        skip: false,
+    };
+    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 delegate = MutVarsDelegate {
+        mut_spans: 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()) {
+        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: HashMap<NodeId, Option<Span>>,
+    skip: bool,
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for MutableVarsVisitor<'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);
+            },
+
+            // If there is any fuction/method call… we just stop analysis
+            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
+    }
+}
+
+struct MutVarsDelegate {
+    mut_spans: HashMap<NodeId, Option<Span>>,
+}
+
+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)
+            }
+        }
+    }
+}
+
+
+impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
+    fn consume(&mut self, _: NodeId, _: Span, _: cmt<'tcx>, _: ConsumeMode) {}
+
+    fn matched_pat(&mut self, _: &Pat, _: cmt<'tcx>, _: MatchMode) {}
+
+    fn consume_pat(&mut self, _: &Pat, _: cmt<'tcx>, _: ConsumeMode) {}
+
+    fn borrow(&mut self, _: NodeId, sp: Span, cmt: cmt<'tcx>, _: ty::Region, bk: ty::BorrowKind, _: LoanCause) {
+        if let ty::BorrowKind::MutBorrow = bk {
+            self.update(&cmt.cat, sp)
+        }
+    }
+
+    fn mutate(&mut self, _: NodeId, sp: Span, cmt: cmt<'tcx>, _: MutateMode) {
+            self.update(&cmt.cat, sp)
+    }
+
+    fn decl_without_init(&mut self, _: NodeId, _: Span) {}
+}

tests/ui/infinite_loop.rs

@@ -0,0 +1,115 @@
+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!() }
+
+fn immutable_condition() {
+    // Should warn when all vars mentionned are immutable
+    let y = 0;
+    while y < 10 {
+        println!("KO - y is immutable");
+    }
+
+    let x = 0;
+    while y < 10 && x < 3 {
+        let mut k = 1;
+        k += 2;
+        println!("KO - x and y immutable");
+    }
+
+    let cond = false;
+    while !cond {
+        println!("KO - cond immutable");
+    }
+
+    let mut i = 0;
+    while y < 10 && i < 3 {
+        i += 1;
+        println!("OK - i is mutable");
+    }
+
+    let mut mut_cond = false;
+    while !mut_cond || cond {
+        mut_cond = true;
+        println!("OK - mut_cond is mutable");
+    }
+
+    while fooi() < x {
+        println!("OK - Fn call results may vary");
+    }
+
+    while foob() {
+        println!("OK - Fn call results may vary");
+    }
+
+}
+
+fn unused_var() {
+    // Should warn when a (mutable) var is not used in while body
+    let (mut i, mut j) = (0, 0);
+
+    while i < 3 {
+        j = 3;
+        println!("KO - i not mentionned");
+    }
+
+    while i < 3 && j > 0 {
+        println!("KO - i and j not mentionned");
+    }
+
+    while i < 3 {
+        let mut i = 5;
+        fn_mutref(&mut i);
+        println!("KO - shadowed");
+    }
+
+    while i < 3 && j > 0 {
+        i = 5;
+        println!("OK - i in cond and mentionned");
+    }
+}
+
+fn used_immutable() {
+    let mut i = 0;
+
+    while i < 3 {
+        fn_constref(&i);
+        println!("KO - const reference");
+    }
+
+    while i < 3 {
+        fn_val(i);
+        println!("KO - passed by value");
+    }
+
+    while i < 3 {
+        println!("OK - passed by mutable reference");
+        fn_mutref(&mut i)
+    }
+
+    while i < 3 {
+        fn_mutref(&mut i);
+        println!("OK - passed by mutable reference");
+    }
+}
+
+use std::cell::Cell;
+
+fn maybe_i_mutate(i: &Cell<bool>) { unimplemented!() }
+
+fn internally_mutable() {
+    let b = Cell::new(true);
+
+    while b.get() {       // b cannot be silently coerced to `bool`
+        maybe_i_mutate(&b);
+        println!("OK - Method call within condition");
+    }
+}
+
+fn main() {
+    immutable_condition();
+    unused_var();
+    used_immutable();
+    internally_mutable();
+}

tests/ui/infinite_loop.stderr

@@ -0,0 +1,67 @@
+error: all variables in condition are immutable. This might lead to infinite loops.
+  --> $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:15:11
+   |
+15 |     while y < 10 && x < 3 {
+   |           ^^^^^^^^^^^^^^^
+
+error: all variables in condition are immutable. This might lead to infinite loops.
+  --> $DIR/infinite_loop.rs:22:11
+   |
+22 |     while !cond {
+   |           ^^^^^
+
+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
+

tests/ui/never_loop.rs

@@ -1,6 +1,6 @@
 
 
-#![allow(single_match, unused_assignments, unused_variables)]
+#![allow(single_match, unused_assignments, unused_variables, while_immutable_condition)]
 
 fn test1() {
     let mut x = 0;