Merge pull request #564 from mcarton/hashmap

Lint looping on maps ignoring the keys or values

Author: llogiq

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 111 lints included in this crate:
+There are 112 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -42,6 +42,7 @@ name
 [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice)                         | warn    | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice
 [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next)                                     | warn    | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`
 [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp)                                         | warn    | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds)
+[for_kv_map](https://github.com/Manishearth/rust-clippy/wiki#for_kv_map)                                       | warn    | looping on a map using `iter` when `keys` or `values` would do
 [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option)                   | warn    | for-looping over an `Option`, which is more clearly expressed as an `if let`
 [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result)                   | warn    | for-looping over a `Result`, which is more clearly expressed as an `if let`
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                                     | warn    | using identity operations, e.g. `x + 0` or `y / 1`

src/lib.rs

@@ -205,6 +205,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         loops::EMPTY_LOOP,
         loops::EXPLICIT_COUNTER_LOOP,
         loops::EXPLICIT_ITER_LOOP,
+        loops::FOR_KV_MAP,
         loops::FOR_LOOP_OVER_OPTION,
         loops::FOR_LOOP_OVER_RESULT,
         loops::ITER_NEXT_LOOP,

src/lifetimes.rs

@@ -352,7 +352,7 @@ fn report_extra_lifetimes(cx: &LateContext, func: &FnDecl, generics: &Generics,
         }
     }
 
-    for (_, v) in checker.0 {
+    for &v in checker.0.values() {
         span_lint(cx, UNUSED_LIFETIMES, v, "this lifetime isn't used in the function definition");
     }
 }

src/loops.rs

@@ -10,8 +10,8 @@ use std::borrow::Cow;
 use std::collections::{HashSet, HashMap};
 
 use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro, expr_block,
-            span_help_and_lint, is_integer_literal, get_enclosing_block};
-use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH, OPTION_PATH, RESULT_PATH};
+            span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, walk_ptrs_ty};
+use utils::{BTREEMAP_PATH, HASHMAP_PATH, LL_PATH, OPTION_PATH, RESULT_PATH, VEC_PATH};
 
 /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default.
 ///
@@ -141,6 +141,24 @@ declare_lint!{ pub EMPTY_LOOP, Warn, "empty `loop {}` detected" }
 /// **Example:** `while let Some(val) = iter() { .. }`
 declare_lint!{ pub WHILE_LET_ON_ITERATOR, Warn, "using a while-let loop instead of a for loop on an iterator" }
 
+/// **What it does:** This warns when you iterate on a map (`HashMap` or `BTreeMap`) and ignore
+/// either the keys or values.
+///
+/// **Why is this bad?** Readability. There are `keys` and `values` methods that can be used to
+/// express that don't need the values or keys.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+/// ```rust
+/// for (k, _) in &map { .. }
+/// ```
+/// could be replaced by
+/// ```rust
+/// for k in map.keys() { .. }
+/// ```
+declare_lint!{ pub FOR_KV_MAP, Warn, "looping on a map using `iter` when `keys` or `values` would do" }
+
 #[derive(Copy, Clone)]
 pub struct LoopsPass;
 
@@ -154,7 +172,8 @@ impl LintPass for LoopsPass {
                     REVERSE_RANGE_LOOP,
                     EXPLICIT_COUNTER_LOOP,
                     EMPTY_LOOP,
-                    WHILE_LET_ON_ITERATOR)
+                    WHILE_LET_ON_ITERATOR,
+                    FOR_KV_MAP)
     }
 }
 
@@ -270,6 +289,7 @@ fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &E
     check_for_loop_reverse_range(cx, arg, expr);
     check_for_loop_arg(cx, pat, arg, expr);
     check_for_loop_explicit_counter(cx, arg, body, expr);
+    check_for_loop_over_map_kv(cx, pat, arg, body, expr);
 }
 
 /// Check for looping over a range and then indexing a sequence with it.
@@ -499,6 +519,79 @@ fn check_for_loop_explicit_counter(cx: &LateContext, arg: &Expr, body: &Expr, ex
     }
 }
 
+// Check for the FOR_KV_MAP lint.
+fn check_for_loop_over_map_kv(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
+    if let PatTup(ref pat) = pat.node {
+        if pat.len() == 2 {
+
+            let (pat_span, kind) = match (&pat[0].node, &pat[1].node) {
+                (key, _) if pat_is_wild(key, body) => (&pat[1].span, "values"),
+                (_, value) if pat_is_wild(value, body) => (&pat[0].span, "keys"),
+                _ => return
+            };
+
+            let ty = walk_ptrs_ty(cx.tcx.expr_ty(arg));
+            let arg_span = if let ExprAddrOf(_, ref expr) = arg.node {
+                expr.span
+            }
+            else {
+                arg.span
+            };
+
+            if match_type(cx, ty, &HASHMAP_PATH) ||
+               match_type(cx, ty, &BTREEMAP_PATH) {
+                span_lint_and_then(cx,
+                          FOR_KV_MAP,
+                          expr.span,
+                          &format!("you seem to want to iterate on a map's {}", kind),
+                          |db| {
+                    db.span_suggestion(expr.span,
+                                       "use the corresponding method",
+                                       format!("for {} in {}.{}() {{...}}",
+                                               snippet(cx, *pat_span, ".."),
+                                               snippet(cx, arg_span, ".."),
+                                               kind));
+                });
+            }
+        }
+    }
+
+}
+
+// Return true if the pattern is a `PatWild` or an ident prefixed with '_'.
+fn pat_is_wild(pat: &Pat_, body: &Expr) -> bool {
+    match *pat {
+        PatWild => true,
+        PatIdent(_, ident, None) if ident.node.name.as_str().starts_with('_') => {
+            let mut visitor = UsedVisitor {
+                var: ident.node,
+                used: false,
+            };
+            walk_expr(&mut visitor, body);
+            !visitor.used
+        },
+        _ => false,
+    }
+}
+
+struct UsedVisitor {
+    var: Ident, // var to look for
+    used: bool, // has the var been used otherwise?
+}
+
+impl<'a> Visitor<'a> for UsedVisitor {
+    fn visit_expr(&mut self, expr: &Expr) {
+        if let ExprPath(None, ref path) = expr.node {
+            if path.segments.len() == 1 && path.segments[0].identifier == self.var {
+                self.used = true;
+                return
+            }
+        }
+
+        walk_expr(self, expr);
+    }
+}
+
 /// Recover the essential nodes of a desugared for loop:
 /// `for pat in arg { body }` becomes `(pat, arg, body)`.
 fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> {
@@ -601,11 +694,14 @@ fn is_ref_iterable_type(cx: &LateContext, e: &Expr) -> bool {
     // no walk_ptrs_ty: calling iter() on a reference can make sense because it
     // will allow further borrows afterwards
     let ty = cx.tcx.expr_ty(e);
-    is_iterable_array(ty) || match_type(cx, ty, &VEC_PATH) || match_type(cx, ty, &LL_PATH) ||
-    match_type(cx, ty, &HASHMAP_PATH) || match_type(cx, ty, &["std", "collections", "hash", "set", "HashSet"]) ||
+    is_iterable_array(ty) ||
+    match_type(cx, ty, &VEC_PATH) ||
+    match_type(cx, ty, &LL_PATH) ||
+    match_type(cx, ty, &HASHMAP_PATH) ||
+    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, &BTREEMAP_PATH) ||
     match_type(cx, ty, &["collections", "btree", "set", "BTreeSet"])
 }
 

tests/compile-fail/for_loop.rs

@@ -280,4 +280,33 @@ fn main() {
     println!("index: {}", index);
 
     for_loop_over_option_and_result();
+
+    let m : HashMap<u64, u64> = HashMap::new();
+    for (_, v) in &m {
+        //~^ you seem to want to iterate on a map's values
+        //~| HELP use the corresponding method
+        //~| SUGGESTION for v in &m.values()
+        let _v = v;
+    }
+
+    let rm = &m;
+    for (k, _value) in rm {
+        //~^ you seem to want to iterate on a map's keys
+        //~| HELP use the corresponding method
+        //~| SUGGESTION for k in rm.keys()
+        let _k = k;
+    }
+
+    test_for_kv_map();
+}
+
+#[allow(used_underscore_binding)]
+fn test_for_kv_map() {
+    let m : HashMap<u64, u64> = HashMap::new();
+
+    // No error, _value is actually used
+    for (k, _value) in &m {
+        let _ = _value;
+        let _k = k;
+    }
 }