mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #564 from mcarton/hashmap

Browse Source 
Lint looping on maps ignoring the keys or values
This commit is contained in:
llogiq 2016-02-05 20:42:39 +01:00
commit 0494071ab7
5 changed files with 135 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
README.md
View File

@ -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`

1
src/lib.rs
View File

@ -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,

2
src/lifetimes.rs
View File

@ -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");
}
}

108
src/loops.rs
View File

@ -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"])
}

29
tests/compile-fail/for_loop.rs
View File

@ -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;
}
}