Merge pull request #147 from birkenfeld/forloop

Lint for for loops over indexes instead of items

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);
+    }
+}

src/returns.rs

@@ -69,17 +69,16 @@ impl ReturnPass {
         // we need both a let-binding stmt and an expr
         if_let_chain! {
             [
-                Some(stmt) = block.stmts.last(),
+                let Some(stmt) = block.stmts.last(),
-                StmtDecl(ref decl, _) = stmt.node,
+                let StmtDecl(ref decl, _) = stmt.node,
-                DeclLocal(ref local) = decl.node,
+                let DeclLocal(ref local) = decl.node,
-                Some(ref initexpr) = local.init,
+                let Some(ref initexpr) = local.init,
-                PatIdent(_, Spanned { node: id, .. }, _) = local.pat.node,
+                let PatIdent(_, Spanned { node: id, .. }, _) = local.pat.node,
-                Some(ref retexpr) = block.expr,
+                let Some(ref retexpr) = block.expr,
-                ExprPath(_, ref path) = retexpr.node
+                let ExprPath(_, ref path) = retexpr.node,
+                match_path(path, &[&*id.name.as_str()])
             ], {
-                if match_path(path, &[&*id.name.as_str()]) {
+                self.emit_let_lint(cx, retexpr.span, initexpr.span);
-                    self.emit_let_lint(cx, retexpr.span, initexpr.span);
-                }
             }
         }
     }

src/utils.rs

@@ -93,9 +93,14 @@ pub fn walk_ptrs_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
     }
 }
 
-/// Produce a nested chain of if-lets from the patterns:
+/// Produce a nested chain of if-lets and ifs from the patterns:
 ///
-///     if_let_chain! {[Some(y) = x, Some(z) = y],
+///     if_let_chain! {
+///         [
+///             Some(y) = x,
+///             y.len() == 2,
+///             Some(z) = y,
+///         ],
 ///         {
 ///             block
 ///         }
@@ -104,20 +109,32 @@ pub fn walk_ptrs_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
 /// becomes
 ///
 ///     if let Some(y) = x {
-///         if let Some(z) = y {
+///         if y.len() == 2 {
-///             block
+///             if let Some(z) = y {
+///                 block
+///             }
 ///         }
 ///     }
 #[macro_export]
 macro_rules! if_let_chain {
-    ([$pat:pat = $expr:expr, $($p2:pat = $e2:expr),+], $block:block) => {
+    ([let $pat:pat = $expr:expr, $($tt:tt)+], $block:block) => {
         if let $pat = $expr {
-           if_let_chain!{ [$($p2 = $e2),+], $block }
+           if_let_chain!{ [$($tt)+], $block }
         }
     };
-    ([$pat:pat = $expr:expr], $block:block) => {
+    ([let $pat:pat = $expr:expr], $block:block) => {
         if let $pat = $expr {
            $block
         }
     };
+    ([$expr:expr, $($tt:tt)+], $block:block) => {
+        if $expr {
+           if_let_chain!{ [$($tt)+], $block }
+        }
+    };
+    ([$expr:expr], $block:block) => {
+        if $expr {
+           $block
+        }
+    };
 }

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]);
+    }
+}