From e066997046378ce9ebb9f1f65fe2ef7fdefcb823 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 10 Jan 2017 08:33:20 +0100 Subject: [PATCH] FOR_KV_MAP can now lint on mutable maps due to values_mut() --- clippy_lints/src/loops.rs | 27 ++++++++++++++++----------- tests/compile-fail/for_loop.rs | 16 ++++++++++++++-- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 9a3defec72df..27bfa16d496a 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -16,7 +16,7 @@ use utils::sugg; use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, multispan_sugg, in_external_macro, is_refutable, span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, higher, - walk_ptrs_ty, last_path_segment}; + last_path_segment}; use utils::paths; /// **What it does:** Checks for looping over the range of `0..len` of some @@ -712,19 +712,24 @@ fn check_for_loop_over_map_kv<'a, 'tcx>( if let PatKind::Tuple(ref pat, _) = pat.node { if pat.len() == 2 { - let (new_pat_span, kind) = match (&pat[0].node, &pat[1].node) { - (key, _) if pat_is_wild(cx, key, body) => (pat[1].span, "value"), - (_, value) if pat_is_wild(cx, value, body) => (pat[0].span, "key"), + let arg_span = arg.span; + let (new_pat_span, kind, ty, mutbl) = match cx.tcx.tables().expr_ty(arg).sty { + ty::TyRef(_, ref tam) => match (&pat[0].node, &pat[1].node) { + (key, _) if pat_is_wild(cx, key, body) => (pat[1].span, "value", tam.ty, tam.mutbl), + (_, value) if pat_is_wild(cx, value, body) => (pat[0].span, "key", tam.ty, MutImmutable), + _ => return, + }, _ => return, }; - - let (arg_span, arg) = match arg.node { - ExprAddrOf(MutImmutable, ref expr) => (arg.span, &**expr), - ExprAddrOf(MutMutable, _) => return, // for _ in &mut _, there is no {values,keys}_mut method - _ => (arg.span, arg), + let mutbl = match mutbl { + MutImmutable => "", + MutMutable => "_mut", + }; + let arg = match arg.node { + ExprAddrOf(_, ref expr) => &**expr, + _ => arg, }; - let ty = walk_ptrs_ty(cx.tcx.tables().expr_ty(arg)); if match_type(cx, ty, &paths::HASHMAP) || match_type(cx, ty, &paths::BTREEMAP) { span_lint_and_then(cx, FOR_KV_MAP, @@ -735,7 +740,7 @@ fn check_for_loop_over_map_kv<'a, 'tcx>( multispan_sugg(db, "use the corresponding method".into(), &[(pat_span, &snippet(cx, new_pat_span, kind)), - (arg_span, &format!("{}.{}s()", map.maybe_par(), kind))]); + (arg_span, &format!("{}.{}s{}()", map.maybe_par(), kind, mutbl))]); }); } } diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 22fcbff64b15..659935e6098e 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -87,7 +87,7 @@ impl Unrelated { } } -#[deny(needless_range_loop, explicit_iter_loop, explicit_into_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)] +#[deny(needless_range_loop, explicit_iter_loop, explicit_into_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop, for_kv_map)] #[deny(unused_collect)] #[allow(linkedlist, shadow_unrelated, unnecessary_mut_passed, cyclomatic_complexity, similar_names)] #[allow(many_single_char_names)] @@ -417,11 +417,23 @@ fn main() { let mut m : HashMap = HashMap::new(); for (_, v) in &mut m { - // Ok, there is no values_mut method or equivalent + //~^ you seem to want to iterate on a map's values + //~| HELP use the corresponding method + //~| HELP use the corresponding method + //~| SUGGESTION for v in m.values_mut() let _v = v; } + let m: &mut HashMap = &mut HashMap::new(); + for (_, v) in &mut *m { + //~^ you seem to want to iterate on a map's values + //~| HELP use the corresponding method + //~| HELP use the corresponding method + //~| SUGGESTION for v in (*m).values_mut() + let _v = v; + } + let m : HashMap = HashMap::new(); let rm = &m; for (k, _value) in rm { //~^ you seem to want to iterate on a map's keys