Merge pull request #237 from birkenfeld/iter_fix

loops: use a whitelist for the "x.iter() -> &x" lint (fixes #236)

src/loops.rs

@@ -1,9 +1,11 @@
 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, match_trait_method};
+use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty};
+use utils::{VEC_PATH, LL_PATH};
 
 declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
                "for-looping over a range of indices where an iterator over items would do" }
@@ -55,18 +57,17 @@ impl LintPass for LoopsPass {
                 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!(
-                            "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));
+                    if method_name == "iter" || method_name == "iter_mut" {
+                        if is_ref_iterable_type(cx, &args[0]) {
+                            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 `{}.{}()`",
+                                if method_name == "iter_mut" { "mut " } else { "" },
+                                object, object, method_name));
+                        }
+                    }
                     // check for looping over Iterator::next() which is not what you want
-                    } else if method_name == "next" {
+                    else if method_name == "next" {
                         if match_trait_method(cx, arg, &["core", "iter", "Iterator"]) {
                             span_lint(cx, ITER_NEXT_LOOP, expr.span,
                                       "you are iterating over `Iterator::next()` which is an Option; \
@@ -134,3 +135,26 @@ impl<'v, 't> Visitor<'v> for VarVisitor<'v, 't> {
         walk_expr(self, expr);
     }
 }
+
+/// Return true if the type of expr is one that provides IntoIterator impls
+/// for &T and &mut T, such as Vec.
+fn is_ref_iterable_type(cx: &Context, e: &Expr) -> bool {
+    let ty = walk_ptrs_ty(cx.tcx.expr_ty(e));
+    println!("mt {:?} {:?}", e, ty);
+    is_array(ty) ||
+        match_type(cx, ty, &VEC_PATH) ||
+        match_type(cx, ty, &LL_PATH) ||
+        match_type(cx, ty, &["std", "collections", "hash", "map", "HashMap"]) ||
+        match_type(cx, ty, &["std", "collections", "hash", "set", "HashSet"]) ||
+        match_type(cx, ty, &["collections", "vec_deque", "VecDeque"]) ||
+        match_type(cx, ty, &["collections", "binary_heap", "BinaryHeap"]) ||
+        match_type(cx, ty, &["collections", "btree", "map", "BTreeMap"]) ||
+        match_type(cx, ty, &["collections", "btree", "set", "BTreeSet"])
+}
+
+fn is_array(ty: ty::Ty) -> bool {
+    match ty.sty {
+        ty::TyArray(..) => true,
+        _ => false
+    }
+}

src/utils.rs

@@ -39,8 +39,7 @@ pub fn in_external_macro(cx: &Context, span: Span) -> bool {
 /// usage e.g. with
 /// `match_def_path(cx, id, &["core", "option", "Option"])`
 pub fn match_def_path(cx: &Context, def_id: DefId, path: &[&str]) -> bool {
-    cx.tcx.with_path(def_id, |iter| iter.map(|elem| elem.name())
-        .zip(path.iter()).all(|(nm, p)| nm == p))
+    cx.tcx.with_path(def_id, |iter| iter.zip(path).all(|(nm, p)| nm.name() == p))
 }
 
 /// check if type is struct or enum type with given def path

tests/compile-fail/for_loop.rs

@@ -1,14 +1,21 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
+use std::collections::*;
+
 struct Unrelated(Vec<u8>);
 impl Unrelated {
     fn next(&self) -> std::slice::Iter<u8> {
         self.0.iter()
     }
+
+    fn iter(&self) -> std::slice::Iter<u8> {
+        self.0.iter()
+    }
 }
 
 #[deny(needless_range_loop, explicit_iter_loop, iter_next_loop)]
+#[allow(linkedlist)]
 fn main() {
     let mut vec = vec![1, 2, 3, 4];
     let vec2 = vec![1, 2, 3, 4];
@@ -28,8 +35,25 @@ fn main() {
     for _v in &vec { } // these are fine
     for _v in &mut vec { } // these are fine
 
+    for _v in [1, 2, 3].iter() { } //~ERROR it is more idiomatic to loop over `&[
+    let ll: LinkedList<()> = LinkedList::new();
+    for _v in ll.iter() { } //~ERROR it is more idiomatic to loop over `&ll`
+    let vd: VecDeque<()> = VecDeque::new();
+    for _v in vd.iter() { } //~ERROR it is more idiomatic to loop over `&vd`
+    let bh: BinaryHeap<()> = BinaryHeap::new();
+    for _v in bh.iter() { } //~ERROR it is more idiomatic to loop over `&bh`
+    let hm: HashMap<(), ()> = HashMap::new();
+    for _v in hm.iter() { } //~ERROR it is more idiomatic to loop over `&hm`
+    let bt: BTreeMap<(), ()> = BTreeMap::new();
+    for _v in bt.iter() { } //~ERROR it is more idiomatic to loop over `&bt`
+    let hs: HashSet<()> = HashSet::new();
+    for _v in hs.iter() { } //~ERROR it is more idiomatic to loop over `&hs`
+    let bs: BTreeSet<()> = BTreeSet::new();
+    for _v in bs.iter() { } //~ERROR it is more idiomatic to loop over `&bs`
+
     for _v in vec.iter().next() { } //~ERROR you are iterating over `Iterator::next()`
 
     let u = Unrelated(vec![]);
     for _v in u.next() { } // no error
+    for _v in u.iter() { } // no error
 }