From f87dd31f30c2e5f731a2408b0da18f43fbc34c6f Mon Sep 17 00:00:00 2001 From: Nathan Weston Date: Sun, 23 Aug 2015 13:25:45 -0400 Subject: [PATCH 1/4] New lint: loop with explicit counter variable (fixes #159) Avoiding false positives here turns out to be fairly complicated. --- README.md | 3 +- src/lib.rs | 1 + src/loops.rs | 229 ++++++++++++++++++++++++++++++++- tests/compile-fail/for_loop.rs | 64 ++++++++- 4 files changed, 291 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index ae711357901..011078c5407 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. [Jump to usage instructions](#usage) ##Lints -There are 57 lints included in this crate: +There are 58 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -21,6 +21,7 @@ name [cmp_owned](https://github.com/Manishearth/rust-clippy/wiki#cmp_owned) | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` [collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op) | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) +[explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` diff --git a/src/lib.rs b/src/lib.rs index 9cb9eb30561..b88d2c844b0 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -119,6 +119,7 @@ pub fn plugin_registrar(reg: &mut Registry) { len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, lifetimes::NEEDLESS_LIFETIMES, + loops::EXPLICIT_COUNTER_LOOP, loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, diff --git a/src/loops.rs b/src/loops.rs index 286b51ab04e..a381737fe3b 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -1,10 +1,12 @@ use rustc::lint::*; use rustc_front::hir::*; use reexport::*; -use rustc_front::visit::{Visitor, walk_expr}; +use rustc_front::visit::{Visitor, walk_expr, walk_block, walk_decl}; use rustc::middle::ty; +use rustc::middle::def::DefLocal; use consts::{constant_simple, Constant}; -use std::collections::HashSet; +use rustc::front::map::Node::{NodeBlock}; +use std::collections::{HashSet,HashMap}; use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro, expr_block, span_help_and_lint}; @@ -29,13 +31,16 @@ declare_lint!{ pub REVERSE_RANGE_LOOP, Warn, "Iterating over an empty range, such as `10..0` or `5..5`" } +declare_lint!{ pub EXPLICIT_COUNTER_LOOP, Warn, + "for-looping with an explicit counter when `_.enumerate()` would do" } + #[derive(Copy, Clone)] pub struct LoopsPass; impl LintPass for LoopsPass { fn get_lints(&self) -> LintArray { lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP, - WHILE_LET_LOOP, UNUSED_COLLECT, REVERSE_RANGE_LOOP) + WHILE_LET_LOOP, UNUSED_COLLECT, REVERSE_RANGE_LOOP, EXPLICIT_COUNTER_LOOP) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -120,6 +125,35 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) { } } } + + // Look for variables that are incremented once per loop iteration. + let mut visitor = IncrementVisitor { cx: cx, states: HashMap::new(), depth: 0, done: false }; + walk_expr(&mut visitor, body); + + // For each candidate, check the parent block to see if + // it's initialized to zero at the start of the loop. + let map = &cx.tcx.map; + let parent_scope = map.get_enclosing_scope(expr.id).and_then(|id| map.get_enclosing_scope(id) ); + if let Some(parent_id) = parent_scope { + if let NodeBlock(block) = map.get(parent_id) { + for (id, _) in visitor.states.iter().filter( |&(_,v)| *v == VarState::IncrOnce) { + let mut visitor2 = InitializeVisitor { cx: cx, end_expr: expr, var_id: id.clone(), + state: VarState::IncrOnce, name: None, + depth: 0, done: false }; + walk_block(&mut visitor2, block); + + if visitor2.state == VarState::Warn { + if let Some(name) = visitor2.name { + span_lint(cx, EXPLICIT_COUNTER_LOOP, expr.span, + &format!("the variable `{0}` is used as a loop counter. Consider \ + using `for ({0}, item) in _.iter().enumerate()` \ + or similar iterators.", + name)); + } + } + } + } + } } // check for `loop { if let {} else break }` that could be `while let` // (also matches explicit "match" instead of "if let") @@ -270,3 +304,192 @@ fn is_break_expr(expr: &Expr) -> bool { _ => false, } } + +// To trigger the EXPLICIT_COUNTER_LOOP lint, a variable must be +// incremented exactly once in the loop body, and initialized to zero +// at the start of the loop. +#[derive(PartialEq)] +enum VarState { + Initial, // Not examined yet + IncrOnce, // Incremented exactly once, may be a loop counter + Declared, // Declared but not (yet) initialized to zero + Warn, + DontWarn +} + +// Scan a for loop for variables that are incremented exactly once. +struct IncrementVisitor<'v, 't: 'v> { + cx: &'v Context<'v, 't>, // context reference + states: HashMap, // incremented variables + depth: u32, // depth of conditional expressions + done: bool +} + +impl<'v, 't> Visitor<'v> for IncrementVisitor<'v, 't> { + fn visit_expr(&mut self, expr: &'v Expr) { + if self.done { + return; + } + + // If node is a variable + if let Some(def_id) = var_def_id(self.cx, expr) { + if let Some(parent) = get_parent_expr(self.cx, expr) { + let state = self.states.entry(def_id).or_insert(VarState::Initial); + + match parent.node { + ExprAssignOp(op, ref lhs, ref rhs) => + if lhs.id == expr.id { + if op.node == BiAdd && is_lit_one(rhs) { + *state = match *state { + VarState::Initial if self.depth == 0 => VarState::IncrOnce, + _ => VarState::DontWarn + }; + } + else { + // Assigned some other value + *state = VarState::DontWarn; + } + }, + ExprAssign(ref lhs, _) if lhs.id == expr.id => *state = VarState::DontWarn, + _ => () + } + } + } + // Give up if there are nested loops + else if is_loop(expr) { + self.states.clear(); + self.done = true; + return; + } + // Keep track of whether we're inside a conditional expression + else if is_conditional(expr) { + self.depth += 1; + walk_expr(self, expr); + self.depth -= 1; + return; + } + walk_expr(self, expr); + } +} + +// Check whether a variable is initialized to zero at the start of a loop. +struct InitializeVisitor<'v, 't: 'v> { + cx: &'v Context<'v, 't>, // context reference + end_expr: &'v Expr, // the for loop. Stop scanning here. + var_id: NodeId, + state: VarState, + name: Option, + depth: u32, // depth of conditional expressions + done: bool +} + +impl<'v, 't> Visitor<'v> for InitializeVisitor<'v, 't> { + fn visit_decl(&mut self, decl: &'v Decl) { + // Look for declarations of the variable + if let DeclLocal(ref local) = decl.node { + if local.pat.id == self.var_id { + if let PatIdent(_, ref ident, _) = local.pat.node { + self.name = Some(ident.node.name); + + self.state = if let Some(ref init) = local.init { + if is_lit_zero(init) { + VarState::Warn + } else { + VarState::Declared + } + } + else { + VarState::Declared + } + } + } + } + walk_decl(self, decl); + } + + fn visit_expr(&mut self, expr: &'v Expr) { + if self.state == VarState::DontWarn || expr == self.end_expr { + self.done = true; + } + // No need to visit expressions before the variable is + // declared or after we've rejected it. + if self.state == VarState::IncrOnce || self.done { + return; + } + + // If node is the desired variable, see how it's used + if var_def_id(self.cx, expr) == Some(self.var_id) { + if let Some(parent) = get_parent_expr(self.cx, expr) { + match parent.node { + ExprAssignOp(_, ref lhs, _) if lhs.id == expr.id => { + self.state = VarState::DontWarn; + }, + ExprAssign(ref lhs, ref rhs) if lhs.id == expr.id => { + self.state = if is_lit_zero(rhs) && self.depth == 0 { + VarState::Warn + } else { + VarState::DontWarn + }}, + _ => () + } + } + } + // If there are other loops between the declaration and the target loop, give up + else if is_loop(expr) { + self.state = VarState::DontWarn; + self.done = true; + return; + } + // Keep track of whether we're inside a conditional expression + else if is_conditional(expr) { + self.depth += 1; + walk_expr(self, expr); + self.depth -= 1; + return; + } + walk_expr(self, expr); + } +} + +fn var_def_id(cx: &Context, expr: &Expr) -> Option { + if let Some(path_res) = cx.tcx.def_map.borrow().get(&expr.id) { + if let DefLocal(node_id) = path_res.base_def { + return Some(node_id) + } + } + None +} + +fn is_loop(expr: &Expr) -> bool { + match expr.node { + ExprLoop(..) | ExprWhile(..) => true, + _ => false + } +} + +fn is_conditional(expr: &Expr) -> bool { + match expr.node { + ExprIf(..) | ExprMatch(..) => true, + _ => false + } +} + +// FIXME: copy/paste from misc.rs +fn is_lit_one(expr: &Expr) -> bool { + if let ExprLit(ref spanned) = expr.node { + if let LitInt(1, _) = spanned.node { + return true; + } + } + false +} + +// FIXME: copy/paste from ranges.rs +fn is_lit_zero(expr: &Expr) -> bool { + if let ExprLit(ref spanned) = expr.node { + if let LitInt(0, _) = spanned.node { + return true; + } + } + false +} diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index a43ed7faa23..3320e9acc1d 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -14,9 +14,9 @@ fn iter(&self) -> std::slice::Iter { } } -#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop)] +#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)] #[deny(unused_collect)] -#[allow(linkedlist)] +#[allow(linkedlist,shadow_unrelated)] fn main() { let mut vec = vec![1, 2, 3, 4]; let vec2 = vec![1, 2, 3, 4]; @@ -119,4 +119,64 @@ fn main() { let mut out = vec![]; vec.iter().map(|x| out.push(x)).collect::>(); //~ERROR you are collect()ing an iterator let _y = vec.iter().map(|x| out.push(x)).collect::>(); // this is fine + + // Loop with explicit counter variable + let mut _index = 0; + for _v in &vec { _index += 1 } //~ERROR the variable `_index` is used as a loop counter + + let mut _index = 1; + _index = 0; + for _v in &vec { _index += 1 } //~ERROR the variable `_index` is used as a loop counter + + let mut _index; + _index = 0; + for _v in &vec { _index += 1 } //~ERROR the variable `_index` is used as a loop counter + for _v in &vec { _index += 1 } // But this does not warn + + // Potential false positives + let mut _index = 0; + _index = 1; + for _v in &vec { _index += 1 } + + let mut _index = 0; + _index += 1; + for _v in &vec { _index += 1 } + + let mut _index = 0; + if true { _index = 1 } + for _v in &vec { _index += 1 } + + let mut _index = 0; + let mut _index = 1; + for _v in &vec { _index += 1 } + + let mut _index = 0; + for _v in &vec { _index += 1; _index += 1 } + + let mut _index = 0; + for _v in &vec { _index *= 2; _index += 1 } + + let mut _index = 0; + for _v in &vec { _index = 1; _index += 1 } + + let mut _index = 0; + + for _v in &vec { let mut _index = 0; _index += 1 } + + let mut _index = 0; + for _v in &vec { _index += 1; _index = 0; } + + let mut _index = 0; + for _v in &vec { for _x in 0..1 { _index += 1; }; _index += 1 } + + let mut _index = 0; + for x in &vec { if *x == 1 { _index += 1 } } + + let mut _index = 0; + if true { _index = 1 }; + for _v in &vec { _index += 1 } + + let mut _index = 1; + if false { _index = 0 }; + for _v in &vec { _index += 1 } } From 1e320b38c18165964916829a990bff1b663bf718 Mon Sep 17 00:00:00 2001 From: Nathan Weston Date: Fri, 4 Sep 2015 09:26:58 -0400 Subject: [PATCH 2/4] Add is_integer_literal utility function Replaces is_lit_zero and is_lit_one which were used in a couple of places. --- src/loops.rs | 28 ++++------------------------ src/misc.rs | 13 ++----------- src/ranges.rs | 14 ++------------ src/utils.rs | 11 +++++++++++ 4 files changed, 19 insertions(+), 47 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index a381737fe3b..ade4f46b4a0 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -9,7 +9,7 @@ use std::collections::{HashSet,HashMap}; use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, - in_external_macro, expr_block, span_help_and_lint}; + in_external_macro, expr_block, span_help_and_lint, is_integer_literal}; use utils::{VEC_PATH, LL_PATH}; declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn, @@ -339,7 +339,7 @@ fn visit_expr(&mut self, expr: &'v Expr) { match parent.node { ExprAssignOp(op, ref lhs, ref rhs) => if lhs.id == expr.id { - if op.node == BiAdd && is_lit_one(rhs) { + if op.node == BiAdd && is_integer_literal(rhs, 1) { *state = match *state { VarState::Initial if self.depth == 0 => VarState::IncrOnce, _ => VarState::DontWarn @@ -392,7 +392,7 @@ fn visit_decl(&mut self, decl: &'v Decl) { self.name = Some(ident.node.name); self.state = if let Some(ref init) = local.init { - if is_lit_zero(init) { + if is_integer_literal(init, 0) { VarState::Warn } else { VarState::Declared @@ -425,7 +425,7 @@ fn visit_expr(&mut self, expr: &'v Expr) { self.state = VarState::DontWarn; }, ExprAssign(ref lhs, ref rhs) if lhs.id == expr.id => { - self.state = if is_lit_zero(rhs) && self.depth == 0 { + self.state = if is_integer_literal(rhs, 0) && self.depth == 0 { VarState::Warn } else { VarState::DontWarn @@ -473,23 +473,3 @@ fn is_conditional(expr: &Expr) -> bool { _ => false } } - -// FIXME: copy/paste from misc.rs -fn is_lit_one(expr: &Expr) -> bool { - if let ExprLit(ref spanned) = expr.node { - if let LitInt(1, _) = spanned.node { - return true; - } - } - false -} - -// FIXME: copy/paste from ranges.rs -fn is_lit_zero(expr: &Expr) -> bool { - if let ExprLit(ref spanned) = expr.node { - if let LitInt(0, _) = spanned.node { - return true; - } - } - false -} diff --git a/src/misc.rs b/src/misc.rs index 2cd6babb9c5..63c2082abde 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -7,7 +7,7 @@ use rustc_front::visit::FnKind; use rustc::middle::ty; -use utils::{get_item_name, match_path, snippet, span_lint, walk_ptrs_ty}; +use utils::{get_item_name, match_path, snippet, span_lint, walk_ptrs_ty, is_integer_literal}; use consts::constant; declare_lint!(pub TOPLEVEL_REF_ARG, Warn, @@ -183,7 +183,7 @@ fn get_lints(&self) -> LintArray { fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let ExprBinary(ref cmp, _, ref right) = expr.node { if let &Spanned {node: BinOp_::BiRem, ..} = cmp { - if is_lit_one(right) { + if is_integer_literal(right, 1) { cx.span_lint(MODULO_ONE, expr.span, "any number modulo 1 will be 0"); } } @@ -191,15 +191,6 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) { } } -fn is_lit_one(expr: &Expr) -> bool { - if let ExprLit(ref spanned) = expr.node { - if let LitInt(1, _) = spanned.node { - return true; - } - } - false -} - declare_lint!(pub REDUNDANT_PATTERN, Warn, "using `name @ _` in a pattern"); #[derive(Copy,Clone)] diff --git a/src/ranges.rs b/src/ranges.rs index 197afaf1163..97f59a3aadd 100644 --- a/src/ranges.rs +++ b/src/ranges.rs @@ -1,7 +1,7 @@ use rustc::lint::{Context, LintArray, LintPass}; use rustc_front::hir::*; use syntax::codemap::Spanned; -use utils::match_type; +use utils::{match_type, is_integer_literal}; declare_lint! { pub RANGE_STEP_BY_ZERO, Warn, @@ -21,7 +21,7 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) { ref args) = expr.node { // Only warn on literal ranges. if ident.name == "step_by" && args.len() == 2 && - is_range(cx, &args[0]) && is_lit_zero(&args[1]) { + is_range(cx, &args[0]) && is_integer_literal(&args[1], 0) { cx.span_lint(RANGE_STEP_BY_ZERO, expr.span, "Range::step_by(0) produces an infinite iterator. \ Consider using `std::iter::repeat()` instead") @@ -37,13 +37,3 @@ fn is_range(cx: &Context, expr: &Expr) -> bool { // Note: RangeTo and RangeFull don't have step_by match_type(cx, ty, &["core", "ops", "Range"]) || match_type(cx, ty, &["core", "ops", "RangeFrom"]) } - -fn is_lit_zero(expr: &Expr) -> bool { - // FIXME: use constant folding - if let ExprLit(ref spanned) = expr.node { - if let LitInt(0, _) = spanned.node { - return true; - } - } - false -} diff --git a/src/utils.rs b/src/utils.rs index d6e529048c9..01c9adf866c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -244,6 +244,17 @@ fn inner(ty: ty::Ty, depth: usize) -> (ty::Ty, usize) { inner(ty, 0) } +pub fn is_integer_literal(expr: &Expr, value: u64) -> bool +{ + // FIXME: use constant folding + if let ExprLit(ref spanned) = expr.node { + if let LitInt(v, _) = spanned.node { + return v == value; + } + } + false +} + /// Produce a nested chain of if-lets and ifs from the patterns: /// /// if_let_chain! { From 6b57924e816af4543fc6b3214ccc4d472f5c9434 Mon Sep 17 00:00:00 2001 From: Nathan Weston Date: Thu, 10 Sep 2015 08:26:31 -0400 Subject: [PATCH 3/4] Improve lint message Remove trailing period and include snippet of loop argument. --- src/loops.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index ade4f46b4a0..06892f6a74e 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -146,9 +146,9 @@ fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let Some(name) = visitor2.name { span_lint(cx, EXPLICIT_COUNTER_LOOP, expr.span, &format!("the variable `{0}` is used as a loop counter. Consider \ - using `for ({0}, item) in _.iter().enumerate()` \ - or similar iterators.", - name)); + using `for ({0}, item) in {1}.enumerate()` \ + or similar iterators", + name, snippet(cx, arg.span, "_"))); } } } From 8a5b4f19fd6cec7b3ef583afa592fde99dabe58e Mon Sep 17 00:00:00 2001 From: Nathan Weston Date: Thu, 10 Sep 2015 08:33:29 -0400 Subject: [PATCH 4/4] Check for mutable borrow of counter variable --- src/loops.rs | 2 ++ tests/compile-fail/for_loop.rs | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/loops.rs b/src/loops.rs index 06892f6a74e..406fbee0b7c 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -351,6 +351,7 @@ fn visit_expr(&mut self, expr: &'v Expr) { } }, ExprAssign(ref lhs, _) if lhs.id == expr.id => *state = VarState::DontWarn, + ExprAddrOf(mutability,_) if mutability == MutMutable => *state = VarState::DontWarn, _ => () } } @@ -430,6 +431,7 @@ fn visit_expr(&mut self, expr: &'v Expr) { } else { VarState::DontWarn }}, + ExprAddrOf(mutability,_) if mutability == MutMutable => self.state = VarState::DontWarn, _ => () } } diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 3320e9acc1d..d6d73db3c18 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -179,4 +179,8 @@ fn main() { let mut _index = 1; if false { _index = 0 }; for _v in &vec { _index += 1 } + + let mut _index = 0; + { let mut _x = &mut _index; } + for _v in &vec { _index += 1 } }