mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-27 07:00:55 +00:00
Merge pull request #314 from nweston/loop-counter
Lint for loops with explicit counter variable (#159)
This commit is contained in:
Manish Goregaokar
commit
68d4b3af12
7 changed files with 293 additions and 30 deletions
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
213
src/loops.rs
213
src/loops.rs
|
|
@ -1,13 +1,15 @@
|
|||
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};
|
||||
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,
|
||||
|
|
@ -29,13 +31,16 @@ declare_lint!{ pub UNUSED_COLLECT, Warn,
|
|||
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 @@ impl LintPass for LoopsPass {
|
|||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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 {1}.enumerate()` \
|
||||
or similar iterators",
|
||||
name, snippet(cx, arg.span, "_")));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
// check for `loop { if let {} else break }` that could be `while let`
|
||||
// (also matches explicit "match" instead of "if let")
|
||||
|
|
@ -270,3 +304,174 @@ 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<NodeId, VarState>, // 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_integer_literal(rhs, 1) {
|
||||
*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,
|
||||
ExprAddrOf(mutability,_) if mutability == MutMutable => *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<Name>,
|
||||
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_integer_literal(init, 0) {
|
||||
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_integer_literal(rhs, 0) && self.depth == 0 {
|
||||
VarState::Warn
|
||||
} else {
|
||||
VarState::DontWarn
|
||||
}},
|
||||
ExprAddrOf(mutability,_) if mutability == MutMutable => self.state = 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<NodeId> {
|
||||
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
|
||||
}
|
||||
}
|
||||
|
|
|
|||
13
src/misc.rs
13
src/misc.rs
|
|
@ -7,7 +7,7 @@ use syntax::codemap::{Span, Spanned};
|
|||
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 @@ impl LintPass for ModuloOne {
|
|||
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 @@ impl LintPass for ModuloOne {
|
|||
}
|
||||
}
|
||||
|
||||
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)]
|
||||
|
|
|
|||
|
|
@ -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 @@ impl LintPass for StepByZero {
|
|||
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
|
||||
}
|
||||
|
|
|
|||
11
src/utils.rs
11
src/utils.rs
|
|
@ -244,6 +244,17 @@ pub fn walk_ptrs_ty_depth(ty: ty::Ty) -> (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! {
|
||||
|
|
|
|||
|
|
@ -14,9 +14,9 @@ impl Unrelated {
|
|||
}
|
||||
}
|
||||
|
||||
#[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,68 @@ fn main() {
|
|||
let mut out = vec![];
|
||||
vec.iter().map(|x| out.push(x)).collect::<Vec<_>>(); //~ERROR you are collect()ing an iterator
|
||||
let _y = vec.iter().map(|x| out.push(x)).collect::<Vec<_>>(); // 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 }
|
||||
|
||||
let mut _index = 0;
|
||||
{ let mut _x = &mut _index; }
|
||||
for _v in &vec { _index += 1 }
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue