Merge pull request #158 from birkenfeld/iter_methods

new lint: looping over x.iter() or x.iter_mut() (fixes #157)

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`

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, _) => {

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,

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
+            // check for looping over a range and then indexing a sequence with it
-            if let PatIdent(_, ref ident, _) = pat.node {
+            // -> the iteratee must be a range literal
-                // the iteratee must be a range literal
+            if let ExprRange(_, _) = arg.node {
-                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));
+                    }
+                }
+            }
         }
     }
 }

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?",

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
 }