Use utils::sugg in FOR_KV_MAP

Author: mcarton

clippy_lints/src/loops.rs

@@ -596,33 +596,34 @@ 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) {
+    let pat_span = pat.span;
+
     if let PatKind::Tuple(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"),
+            let (new_pat_span, kind) = match (&pat[0].node, &pat[1].node) {
+                (key, _) if pat_is_wild(key, body) => (pat[1].span, "value"),
+                (_, value) if pat_is_wild(value, body) => (pat[0].span, "key"),
                 _ => return,
             };
 
-            let arg_span = match arg.node {
-                ExprAddrOf(MutImmutable, ref expr) => expr.span,
+            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.span, arg),
             };
 
             let ty = walk_ptrs_ty(cx.tcx.expr_ty(arg));
             if match_type(cx, ty, &paths::HASHMAP) || match_type(cx, ty, &paths::BTREEMAP) {
                 span_lint_and_then(cx,
                                    FOR_KV_MAP,
                                    expr.span,
-                                   &format!("you seem to want to iterate on a map's {}", kind),
+                                   &format!("you seem to want to iterate on a map's {}s", kind),
                                    |db| {
-                    db.span_suggestion(expr.span,
-                                       "use the corresponding method",
-                                       format!("for {} in {}.{}() {{ .. }}",
-                                               snippet(cx, *pat_span, ".."),
-                                               snippet(cx, arg_span, ".."),
-                                               kind));
+                    let map = sugg::Sugg::hir(cx, arg, "map");
+                    multispan_sugg(db, "use the corresponding method".into(), &[
+                        (pat_span, &snippet(cx, new_pat_span, kind)),
+                        (arg_span, &format!("{}.{}s()", map.maybe_par(), kind)),
+                    ]);
                 });
             }
         }

clippy_lints/src/transmute.rs

@@ -149,7 +149,7 @@ impl LateLintPass for Transmute {
                                     from_ty,
                                     to_ty),
                             |db| {
-                                let arg = &sugg::Sugg::hir(cx, &args[0], "..");
+                                let arg = sugg::Sugg::hir(cx, &args[0], "..");
                                 let (deref, cast) = if to_rty.mutbl == Mutability::MutMutable {
                                     ("&mut *", "*mut")
                                 } else {

clippy_lints/src/utils/sugg.rs

@@ -121,7 +121,7 @@ impl<'a> Sugg<'a> {
 
     /// Convenience method to create the `&<expr>` suggestion.
     pub fn addr(self) -> Sugg<'static> {
-        make_unop("&", &self)
+        make_unop("&", self)
     }
 
     /// Convenience method to create the `<lhs>..<rhs>` or `<lhs>...<rhs>` suggestion.
@@ -131,6 +131,15 @@ impl<'a> Sugg<'a> {
             ast::RangeLimits::Closed => make_assoc(AssocOp::DotDotDot, &self, &end),
         }
     }
+
+    /// Add parenthesis to any expression that might need them. Suitable to the `self` argument of
+    /// a method call (eg. to build `bar.foo()` or `(1 + 2).foo()`).
+    pub fn maybe_par(self) -> Self {
+        match self {
+            Sugg::NonParen(..) => self,
+            Sugg::MaybeParen(sugg) | Sugg::BinOp(_, sugg) => Sugg::NonParen(format!("({})", sugg).into()),
+        }
+    }
 }
 
 impl<'a, 'b> std::ops::Add<Sugg<'b>> for Sugg<'a> {
@@ -150,7 +159,7 @@ impl<'a, 'b> std::ops::Sub<Sugg<'b>> for Sugg<'a> {
 impl<'a> std::ops::Not for Sugg<'a> {
     type Output = Sugg<'static>;
     fn not(self) -> Sugg<'static> {
-        make_unop("!", &self)
+        make_unop("!", self)
     }
 }
 
@@ -178,13 +187,12 @@ impl<T: std::fmt::Display> std::fmt::Display for ParenHelper<T> {
     }
 }
 
-/// Build the string for `<op> <expr>` adding parenthesis when necessary.
+/// Build the string for `<op><expr>` adding parenthesis when necessary.
 ///
 /// For convenience, the operator is taken as a string because all unary operators have the same
 /// precedence.
-pub fn make_unop(op: &str, expr: &Sugg) -> Sugg<'static> {
-    let needs_paren = !matches!(*expr, Sugg::NonParen(..));
-    Sugg::MaybeParen(format!("{}{}", op, ParenHelper::new(needs_paren, expr)).into())
+pub fn make_unop(op: &str, expr: Sugg) -> Sugg<'static> {
+    Sugg::MaybeParen(format!("{}{}", op, expr.maybe_par()).into())
 }
 
 /// Build the string for `<lhs> <op> <rhs>` adding parenthesis when necessary.

tests/compile-fail/for_loop.rs

@@ -2,6 +2,7 @@
 #![plugin(clippy)]
 
 use std::collections::*;
+use std::rc::Rc;
 
 static STATIC: [usize; 4] = [ 0,  1,  8, 16 ];
 const CONST: [usize; 4] = [ 0,  1,  8, 16 ];
@@ -388,8 +389,20 @@ fn main() {
     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()
+        //~| HELP use the corresponding method
+        //~| SUGGESTION for v in m.values() {
+        let _v = v;
+    }
+
+    let m : Rc<HashMap<u64, u64>> = Rc::new(HashMap::new());
+    for (_, v) in &*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() {
         let _v = v;
+        // Here the `*` is not actually necesarry, but the test tests that we don't suggest
+        // `in *m.values()` as we used to
     }
 
     let mut m : HashMap<u64, u64> = HashMap::new();
@@ -403,7 +416,8 @@ fn main() {
     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()
+        //~| HELP use the corresponding method
+        //~| SUGGESTION for k in rm.keys() {
         let _k = k;
     }