From 431c446746a7893ca6c78048adebc55f2f308979 Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 19 Jan 2016 21:10:00 +0100 Subject: [PATCH 1/3] Lint looping on maps ignoring the keys or values --- README.md | 3 +- src/lib.rs | 1 + src/lifetimes.rs | 2 +- src/loops.rs | 82 +++++++++++++++++++++++++++++++--- tests/compile-fail/for_loop.rs | 16 +++++++ 5 files changed, 96 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index f7908ffdcab..59a12e09b17 100644 --- a/README.md +++ b/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` diff --git a/src/lib.rs b/src/lib.rs index 56a2a072421..3f18bcb17ce 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/lifetimes.rs b/src/lifetimes.rs index b6f8ebe5fd8..3e9880d5932 100644 --- a/src/lifetimes.rs +++ b/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"); } } diff --git a/src/loops.rs b/src/loops.rs index 49e073aacd0..830fac94e46 100644 --- a/src/loops.rs +++ b/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, expr); } /// Check for looping over a range and then indexing a sequence with it. @@ -499,6 +519,53 @@ 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, 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) => (&pat[1].span, "values"), + (_, value) if pat_is_wild(value) => (&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_) -> bool { + match *pat { + PatWild => true, + PatIdent(_, ident, None) if ident.node.name.as_str().starts_with('_') => true, + _ => false, + } +} + /// 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 +668,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"]) } diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 1fcbbf54d1f..7049a21b15e 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -280,4 +280,20 @@ fn main() { println!("index: {}", index); for_loop_over_option_and_result(); + + let m : HashMap = 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, _values) 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; + } } From 0f50b0981d33c2fcb591e2aab46bd8bd0497daff Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 5 Feb 2016 19:14:02 +0100 Subject: [PATCH 2/3] Check for pattern use in FOR_KV_MAP --- src/loops.rs | 38 ++++++++++++++++++++++++++++------ tests/compile-fail/for_loop.rs | 15 +++++++++++++- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index 830fac94e46..22dfef55ec1 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -289,7 +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, 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. @@ -520,12 +520,13 @@ 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, expr: &Expr) { +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) => (&pat[1].span, "values"), - (_, value) if pat_is_wild(value) => (&pat[0].span, "keys"), + (key, _) if pat_is_wild(key, body) => (&pat[1].span, "values"), + (_, value) if pat_is_wild(value, body) => (&pat[0].span, "keys"), _ => return }; @@ -558,14 +559,39 @@ fn check_for_loop_over_map_kv(cx: &LateContext, pat: &Pat, arg: &Expr, expr: &Ex } // Return true if the pattern is a `PatWild` or an ident prefixed with '_'. -fn pat_is_wild(pat: &Pat_) -> bool { +fn pat_is_wild(pat: &Pat_, body: &Expr) -> bool { match *pat { PatWild => true, - PatIdent(_, ident, None) if ident.node.name.as_str().starts_with('_') => 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)> { diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 7049a21b15e..e361ebe777f 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -290,10 +290,23 @@ fn main() { } let rm = &m; - for (k, _values) in rm { + 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 = HashMap::new(); + + // No error, _value is actually used + for (k, _value) in &m { + let _ = _value; + let _k = k; + } } From c0063e172de4854b6da3f097e5eb1da43dd3bd7a Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 5 Feb 2016 19:46:11 +0100 Subject: [PATCH 3/3] Improve error message --- src/loops.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/loops.rs b/src/loops.rs index 22dfef55ec1..3f430a1aaf0 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -547,7 +547,7 @@ fn check_for_loop_over_map_kv(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Ex |db| { db.span_suggestion(expr.span, "use the corresponding method", - format!("for {} in {}.{}()", + format!("for {} in {}.{}() {{...}}", snippet(cx, *pat_span, ".."), snippet(cx, arg_span, ".."), kind));