Merge pull request #185 from birkenfeld/iter_next_loop

new lint: iterating over any Iterator::next() result (fixes #182)

README.md

@@ -20,6 +20,7 @@ float_cmp            | warn    | using `==` or `!=` on float values (as floating
 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`
 inline_always        | warn    | `#[inline(always)]` is a bad idea in most cases
+iter_next_loop       | warn    | for-looping over `_.next()` which is probably not intended
 len_without_is_empty | warn    | traits and impls that have `.len()` but not `.is_empty()`
 len_zero             | warn    | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead
 let_and_return       | warn    | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a function

src/lib.rs

@@ -80,6 +80,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         len_zero::LEN_ZERO,
         lifetimes::NEEDLESS_LIFETIMES,
         loops::EXPLICIT_ITER_LOOP,
+        loops::ITER_NEXT_LOOP,
         loops::NEEDLESS_RANGE_LOOP,
         methods::OPTION_UNWRAP_USED,
         methods::RESULT_UNWRAP_USED,

src/loops.rs

@@ -1,9 +1,10 @@
 use rustc::lint::*;
 use syntax::ast::*;
 use syntax::visit::{Visitor, walk_expr};
+use rustc::middle::ty;
 use std::collections::HashSet;
 
-use utils::{snippet, span_lint, get_parent_expr};
+use utils::{snippet, span_lint, get_parent_expr, match_def_path};
 
 declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
                "for-looping over a range of indices where an iterator over items would do" }
@@ -11,12 +12,15 @@ declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
 declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn,
                "for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do" }
 
+declare_lint!{ pub ITER_NEXT_LOOP, Warn,
+               "for-looping over `_.next()` which is probably not intended" }
+
 #[derive(Copy, Clone)]
 pub struct LoopsPass;
 
 impl LintPass for LoopsPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP)
+        lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP)
     }
 
     fn check_expr(&mut self, cx: &Context, expr: &Expr) {
@@ -47,11 +51,11 @@ 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()
+                // just the receiver, no arguments
                 if args.len() == 1 {
                     let method_name = method.node.name;
+                    // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
                     if method_name == "iter" {
                         let object = snippet(cx, args[0].span, "_");
                         span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!(
@@ -62,6 +66,19 @@ impl LintPass for LoopsPass {
                         span_lint(cx, EXPLICIT_ITER_LOOP, expr.span, &format!(
                             "it is more idiomatic to loop over `&mut {}` instead of `{}.iter_mut()`",
                             object, object));
+                    // check for looping over Iterator::next() which is not what you want
+                    } else if method_name == "next" {
+                        let method_call = ty::MethodCall::expr(arg.id);
+                        let trt_id = cx.tcx.tables
+                                           .borrow().method_map.get(&method_call)
+                                           .and_then(|callee| cx.tcx.trait_of_item(callee.def_id));
+                        if let Some(trt_id) = trt_id {
+                            if match_def_path(cx, trt_id, &["core", "iter", "Iterator"]) {
+                                span_lint(cx, ITER_NEXT_LOOP, expr.span,
+                                          "you are iterating over `Iterator::next()` which is an Option; \
+                                           this will compile but is probably not what you want");
+                            }
+                        }
                     }
                 }
             }

tests/compile-fail/for_loop.rs

@@ -1,7 +1,14 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
-#[deny(needless_range_loop, explicit_iter_loop)]
+struct Unrelated(Vec<u8>);
+impl Unrelated {
+    fn next(&self) -> std::slice::Iter<u8> {
+        self.0.iter()
+    }
+}
+
+#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop)]
 fn main() {
     let mut vec = vec![1, 2, 3, 4];
     let vec2 = vec![1, 2, 3, 4];
@@ -20,4 +27,9 @@ fn main() {
 
     for _v in &vec { } // these are fine
     for _v in &mut vec { } // these are fine
+
+    for _v in vec.iter().next() { } //~ERROR you are iterating over `Iterator::next()`
+
+    let u = Unrelated(vec![]);
+    for _v in u.next() { } // no error
 }