From ffed5b0b23ff8b3602a23227ecbfe7c9998d210b Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 18:26:20 +0200 Subject: [PATCH] loops: use a whitelist for the "x.iter() -> &x" lint (fixes #236) --- src/loops.rs | 48 +++++++++++++++++++++++++--------- src/utils.rs | 3 +-- tests/compile-fail/for_loop.rs | 24 +++++++++++++++++ 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index ca8d3990fc5..d12393dba68 100644 --- a/src/loops.rs +++ b/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 + } +} diff --git a/src/utils.rs b/src/utils.rs index 394204bedfc..d3ab2a586ea 100644 --- a/src/utils.rs +++ b/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 diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index a4e3cc31a88..eb7667b7fbd 100755 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -1,14 +1,21 @@ #![feature(plugin)] #![plugin(clippy)] +use std::collections::*; + struct Unrelated(Vec); impl Unrelated { fn next(&self) -> std::slice::Iter { self.0.iter() } + + fn iter(&self) -> std::slice::Iter { + 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 }