new lint: using for i in 0..x { .. vec[i] .. } instead of iterator (fixes #3)

Author: birkenfeld

src/lib.rs

@@ -32,6 +32,7 @@ pub mod unicode;
 pub mod strings;
 pub mod methods;
 pub mod returns;
+pub mod loops;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
@@ -59,6 +60,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box returns::ReturnPass as LintPassObject);
     reg.register_lint_pass(box methods::MethodsPass as LintPassObject);
     reg.register_lint_pass(box types::LetPass as LintPassObject);
+    reg.register_lint_pass(box loops::LoopsPass as LintPassObject);
 
     reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
                                            misc::SINGLE_MATCH,
@@ -87,5 +89,6 @@ pub fn plugin_registrar(reg: &mut Registry) {
                                            methods::STR_TO_STRING,
                                            methods::STRING_TO_STRING,
                                            types::LET_UNIT_VALUE,
+                                           loops::NEEDLESS_RANGE_LOOP,
                                            ]);
 }

src/loops.rs

@@ -0,0 +1,105 @@
+use rustc::lint::*;
+use syntax::ast::*;
+use syntax::visit::{Visitor, walk_expr};
+use std::collections::HashSet;
+
+use utils::{span_lint, get_parent_expr};
+
+declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
+               "Warn about looping over a range of indices if a normal iterator would do" }
+
+#[derive(Copy, Clone)]
+pub struct LoopsPass;
+
+impl LintPass for LoopsPass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(NEEDLESS_RANGE_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 {
+                    let mut visitor = VarVisitor { cx: cx, var: ident.node.name,
+                                                   indexed: HashSet::new(), nonindex: false };
+                    walk_expr(&mut visitor, body);
+                    // linting condition: we only indexed one variable
+                    if visitor.indexed.len() == 1 {
+                        let indexed = visitor.indexed.into_iter().next().unwrap();
+                        if visitor.nonindex {
+                            span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!(
+                                "the loop variable `{}` is used to index `{}`. Consider using \
+                                 `for ({}, item) in {}.iter().enumerate()` or similar iterators.",
+                                ident.node.name.as_str(), indexed.as_str(),
+                                ident.node.name.as_str(), indexed.as_str()));
+                        } else {
+                            span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!(
+                                "the loop variable `{}` is only used to index `{}`. \
+                                 Consider using `for item in &{}` or similar iterators.",
+                                ident.node.name.as_str(), indexed.as_str(), indexed.as_str()));
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+/// Recover the essential nodes of a desugared for loop:
+/// `for pat in arg { body }` becomes `(pat, arg, body)`.
+fn recover_for_loop<'a>(expr: &'a Expr) -> Option<(&'a Pat, &'a Expr, &'a Expr)> {
+    if_let_chain! {
+        [
+            let ExprMatch(ref iterexpr, ref arms, _) = expr.node,
+            let ExprCall(_, ref iterargs) = iterexpr.node,
+            iterargs.len() == 1,
+            arms.len() == 1 && arms[0].guard.is_none(),
+            let ExprLoop(ref block, _) = arms[0].body.node,
+            block.stmts.is_empty(),
+            let Some(ref loopexpr) = block.expr,
+            let ExprMatch(_, ref innerarms, MatchSource::ForLoopDesugar) = loopexpr.node,
+            innerarms.len() == 2 && innerarms[0].pats.len() == 1,
+            let PatEnum(_, Some(ref somepats)) = innerarms[0].pats[0].node,
+            somepats.len() == 1
+        ], {
+            return Some((&*somepats[0],
+                         &*iterargs[0],
+                         &*innerarms[0].body));
+        }
+    }
+    None
+}
+
+struct VarVisitor<'v, 't: 'v> {
+    cx: &'v Context<'v, 't>, // context reference
+    var: Name,               // var name to look for as index
+    indexed: HashSet<Name>,  // indexed variables
+    nonindex: bool,          // has the var been used otherwise?
+}
+
+impl<'v, 't> Visitor<'v> for VarVisitor<'v, 't> {
+    fn visit_expr(&mut self, expr: &'v Expr) {
+        if let ExprPath(None, ref path) = expr.node {
+            if path.segments.len() == 1 && path.segments[0].identifier.name == self.var {
+                // we are referencing our variable! now check if it's as an index
+                if_let_chain! {
+                    [
+                        let Some(parexpr) = get_parent_expr(self.cx, expr),
+                        let ExprIndex(ref seqexpr, _) = parexpr.node,
+                        let ExprPath(None, ref seqvar) = seqexpr.node,
+                        seqvar.segments.len() == 1
+                    ], {
+                        self.indexed.insert(seqvar.segments[0].identifier.name);
+                        return;  // no need to walk further
+                    }
+                }
+                // we are not indexing anything, record that
+                self.nonindex = true;
+                return;
+            }
+        }
+        walk_expr(self, expr);
+    }
+}

tests/compile-fail/for_loop.rs

@@ -0,0 +1,17 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#[deny(needless_range_loop)]
+fn main() {
+    let 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]);
+    }
+    for i in 0..vec.len() {      //~ERROR the loop variable `i` is used to index `vec`.
+        println!("{} {}", vec[i], i);
+    }
+    for i in 0..vec.len() {      // not an error, indexing more than one variable
+        println!("{} {}", vec[i], vec2[i]);
+    }
+}