From 957840363802eb817e1285f0879c8474caff465d Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Thu, 13 Aug 2015 15:36:31 +0200 Subject: [PATCH] new lint: looping over x.iter() or x.iter_mut() (fixes #157) --- README.md | 1 + src/attrs.rs | 2 +- src/lib.rs | 1 + src/loops.rs | 35 ++++++++++++++++++++++++++++------ src/types.rs | 2 +- tests/compile-fail/for_loop.rs | 10 ++++++++-- 6 files changed, 41 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index f6b65b952..dd7d0158f 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ cmp_nan | deny | comparisons to NAN (which will always return fa cmp_owned | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` 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 | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) +explicit_iter_loop | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do 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 | warn | using identity operations, e.g. `x + 0` or `y / 1` ineffective_bit_mask | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` diff --git a/src/attrs.rs b/src/attrs.rs index ca5b6c55c..ef3320d25 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -64,7 +64,7 @@ fn is_relevant_trait(item: &TraitItem) -> bool { } fn is_relevant_block(block: &Block) -> bool { - for stmt in block.stmts.iter() { + for stmt in &block.stmts { match stmt.node { StmtDecl(_, _) => return true, StmtExpr(ref expr, _) | StmtSemi(ref expr, _) => { diff --git a/src/lib.rs b/src/lib.rs index 4c29b8f1b..7cb2877b2 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -76,6 +76,7 @@ pub fn plugin_registrar(reg: &mut Registry) { len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, lifetimes::NEEDLESS_LIFETIMES, + loops::EXPLICIT_ITER_LOOP, loops::NEEDLESS_RANGE_LOOP, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, diff --git a/src/loops.rs b/src/loops.rs index b406bc679..bfb5ad686 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -3,25 +3,29 @@ use syntax::ast::*; use syntax::visit::{Visitor, walk_expr}; use std::collections::HashSet; -use utils::{span_lint, get_parent_expr}; +use utils::{snippet, span_lint, get_parent_expr}; declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn, "for-looping over a range of indices where an iterator over items would do" } +declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn, + "for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do" } + #[derive(Copy, Clone)] pub struct LoopsPass; impl LintPass for LoopsPass { fn get_lints(&self) -> LintArray { - lint_array!(NEEDLESS_RANGE_LOOP) + lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let Some((pat, arg, body)) = recover_for_loop(expr) { - // the var must be a single name - if let PatIdent(_, ref ident, _) = pat.node { - // the iteratee must be a range literal - if let ExprRange(_, _) = arg.node { + // check for looping over a range and then indexing a sequence with it + // -> the iteratee must be a range literal + if let ExprRange(_, _) = arg.node { + // the var must be a single name + if let PatIdent(_, ref ident, _) = pat.node { let mut visitor = VarVisitor { cx: cx, var: ident.node.name, indexed: HashSet::new(), nonindex: false }; walk_expr(&mut visitor, body); @@ -43,6 +47,25 @@ impl LintPass for LoopsPass { } } } + + // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x + if let ExprMethodCall(ref method, _, ref args) = arg.node { + // just the receiver, no arguments to iter() or iter_mut() + if args.len() == 1 { + let method_name = method.node.name.as_str(); + if method_name == "iter" { + let object = snippet(cx, args[0].span, "_"); + span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!( + "it is more idiomatic to loop over `&{}` instead of `{}.iter()`", + object, object)); + } else if method_name == "iter_mut" { + let object = snippet(cx, args[0].span, "_"); + span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!( + "it is more idiomatic to loop over `&mut {}` instead of `{}.iter_mut()`", + object, object)); + } + } + } } } } diff --git a/src/types.rs b/src/types.rs index ad7b767d9..53d8850c5 100644 --- a/src/types.rs +++ b/src/types.rs @@ -67,7 +67,7 @@ impl LintPass for TypePass { let dlists = [vec!["std","collections","linked_list","LinkedList"], vec!["std","collections","linked_list","LinkedList"], vec!["collections","linked_list","LinkedList"]]; - for path in dlists.iter() { + for path in &dlists { if match_ty_unwrap(ty, &path[..]).is_some() { span_help_and_lint(cx, LINKEDLIST, ty.span, "I see you're using a LinkedList! Perhaps you meant some other data structure?", diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 318e6fc85..550f98692 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -1,9 +1,9 @@ #![feature(plugin)] #![plugin(clippy)] -#[deny(needless_range_loop)] +#[deny(needless_range_loop, explicit_iter_loop)] fn main() { - let vec = vec![1, 2, 3, 4]; + let mut vec = vec![1, 2, 3, 4]; let vec2 = vec![1, 2, 3, 4]; for i in 0..vec.len() { //~ERROR the loop variable `i` is only used to index `vec`. println!("{}", vec[i]); @@ -14,4 +14,10 @@ fn main() { for i in 0..vec.len() { // not an error, indexing more than one variable println!("{} {}", vec[i], vec2[i]); } + + for _v in vec.iter() { } //~ERROR it is more idiomatic to loop over `&vec` + for _v in vec.iter_mut() { } //~ERROR it is more idiomatic to loop over `&mut vec` + + for _v in &vec { } // these are fine + for _v in &mut vec { } // these are fine }