diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7f3b176b889..fbfa598e77d 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index ca1d987dbf2..d70d32f2306 100644 --- a/clippy_lints/src/loops.rs +++ b/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) { @@ -662,6 +689,46 @@ fn check_for_loop<'a, 'tcx>( detect_manual_memcpy(cx, pat, arg, body, expr); } +fn search_mutable_vars<'a, 'tcx> ( + cx: &LateContext<'a, 'tcx>, + ex: &'tcx Expr, + acc: &mut Vec, +) -> bool { + match ex.node { + ExprBinary(_, ref a, ref b) => + search_mutable_vars(cx, a, acc) && search_mutable_vars(cx, b, acc), + + ExprUnary(_, ref a) => search_mutable_vars(cx, a, acc), + ExprPath(_) => { + if let Some(node_id) = check_for_mutability(cx, &ex) { + acc.push(node_id); + } + true + } + ExprLit(_) => true, + + // Skip if any method or function call is encountered + _ => false + } +} + +fn check_infinite_loop<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + cond: &'tcx Expr, + _block: &'tcx Block, + _expr: &'tcx Expr, +) { + let mut mutable_vars = Vec::new(); + if search_mutable_vars(cx, cond, &mut mutable_vars) && mutable_vars.len() == 0 { + span_lint( + cx, + WHILE_IMMUTABLE_CONDITION, + cond.span, + "all variables in condition are immutable. This might lead to infinite loops." + ) + } +} + fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr, var: ast::NodeId) -> bool { if_chain! { if let ExprPath(ref qpath) = expr.node; diff --git a/tests/ui/infinite_loop.rs b/tests/ui/infinite_loop.rs new file mode 100644 index 00000000000..e6078a8ef98 --- /dev/null +++ b/tests/ui/infinite_loop.rs @@ -0,0 +1,89 @@ +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 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 { + 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 foo() < x { + 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 { + fn_mutref(&mut i); + println!("OK - passed by mutable reference"); + } +} + +fn main() { + immutable_condition(); + unused_var(); + used_immutable(); +} diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr new file mode 100644 index 00000000000..ddc556f426c --- /dev/null +++ b/tests/ui/infinite_loop.stderr @@ -0,0 +1,22 @@ +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` + +error: all variables in condition are immutable. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:14:11 + | +14 | while y < 10 && x < 3 { + | ^^^^^^^^^^^^^^^ + +error: all variables in condition are immutable. This might lead to infinite loops. + --> $DIR/infinite_loop.rs:19:11 + | +19 | while !cond { + | ^^^^^ + +error: aborting due to 3 previous errors + diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index 2712db2bd31..20500126662 100644 --- a/tests/ui/never_loop.rs +++ b/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;