Auto merge of rust-lang#10159 - khuey:iter_kv_map_ref_mut, r=llogiq

Make the iter_kv_map lint handle ref/mut annotations.

For the degenerate (`map(|(k, _)| k)`/`map(|(_, v)| v)`) cases a mut annotation is superfluous and a ref annotation won't compile, so no additional handling is required. For cases where the `map` call must be preserved ref/mut annotations should also be presereved so that the map body continues to work as expected.

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`iter_kv_map`]: handle ref/mut annotations

Author: bors

clippy_lints/src/methods/iter_kv_map.rs

@@ -6,7 +6,7 @@ use clippy_utils::source::{snippet, snippet_with_applicability};
 use clippy_utils::sugg;
 use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::is_local_used;
-use rustc_hir::{BindingAnnotation, Body, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind};
+use rustc_hir::{BindingAnnotation, Body, BorrowKind, ByRef, Expr, ExprKind, Mutability, Pat, PatKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::ty;
 use rustc_span::sym;
@@ -30,9 +30,9 @@ pub(super) fn check<'tcx>(
         if let Body {params: [p], value: body_expr, generator_kind: _ } = cx.tcx.hir().body(c.body);
         if let PatKind::Tuple([key_pat, val_pat], _) = p.pat.kind;
 
-        let (replacement_kind, binded_ident) = match (&key_pat.kind, &val_pat.kind) {
-            (key, PatKind::Binding(_, _, value, _)) if pat_is_wild(cx, key, m_arg) => ("value", value),
-            (PatKind::Binding(_, _, key, _), value) if pat_is_wild(cx, value, m_arg) => ("key", key),
+        let (replacement_kind, annotation, bound_ident) = match (&key_pat.kind, &val_pat.kind) {
+            (key, PatKind::Binding(ann, _, value, _)) if pat_is_wild(cx, key, m_arg) => ("value", ann, value),
+            (PatKind::Binding(ann, _, key, _), value) if pat_is_wild(cx, value, m_arg) => ("key", ann, key),
             _ => return,
         };
 
@@ -47,7 +47,7 @@ pub(super) fn check<'tcx>(
             if_chain! {
                 if let ExprKind::Path(rustc_hir::QPath::Resolved(_, path)) = body_expr.kind;
                 if let [local_ident] = path.segments;
-                if local_ident.ident.as_str() == binded_ident.as_str();
+                if local_ident.ident.as_str() == bound_ident.as_str();
 
                 then {
                     span_lint_and_sugg(
@@ -60,13 +60,23 @@ pub(super) fn check<'tcx>(
                         applicability,
                     );
                 } else {
+                    let ref_annotation = if annotation.0 == ByRef::Yes {
+                        "ref "
+                    } else {
+                        ""
+                    };
+                    let mut_annotation = if annotation.1 == Mutability::Mut {
+                        "mut "
+                    } else {
+                        ""
+                    };
                     span_lint_and_sugg(
                         cx,
                         ITER_KV_MAP,
                         expr.span,
                         &format!("iterating on a map's {replacement_kind}s"),
                         "try",
-                        format!("{recv_snippet}.{into_prefix}{replacement_kind}s().map(|{binded_ident}| {})",
+                        format!("{recv_snippet}.{into_prefix}{replacement_kind}s().map(|{ref_annotation}{mut_annotation}{bound_ident}| {})",
                             snippet_with_applicability(cx, body_expr.span, "/* body */", &mut applicability)),
                         applicability,
                     );

tests/ui/iter_kv_map.fixed

@@ -1,14 +1,15 @@
 // run-rustfix
 
 #![warn(clippy::iter_kv_map)]
-#![allow(clippy::redundant_clone)]
-#![allow(clippy::suspicious_map)]
-#![allow(clippy::map_identity)]
+#![allow(unused_mut, clippy::redundant_clone, clippy::suspicious_map, clippy::map_identity)]
 
 use std::collections::{BTreeMap, HashMap};
 
 fn main() {
     let get_key = |(key, _val)| key;
+    fn ref_acceptor(v: &u32) -> u32 {
+        *v
+    }
 
     let map: HashMap<u32, u32> = HashMap::new();
 
@@ -36,6 +37,20 @@ fn main() {
     let _ = map.keys().map(|key| key * 9).count();
     let _ = map.values().map(|value| value * 17).count();
 
+    // Preserve the ref in the fix.
+    let _ = map.clone().into_values().map(|ref val| ref_acceptor(val)).count();
+
+    // Preserve the mut in the fix.
+    let _ = map
+        .clone().into_values().map(|mut val| {
+            val += 2;
+            val
+        })
+        .count();
+
+    // Don't let a mut interfere.
+    let _ = map.clone().into_values().count();
+
     let map: BTreeMap<u32, u32> = BTreeMap::new();
 
     let _ = map.keys().collect::<Vec<_>>();
@@ -61,4 +76,18 @@ fn main() {
     // Lint
     let _ = map.keys().map(|key| key * 9).count();
     let _ = map.values().map(|value| value * 17).count();
+
+    // Preserve the ref in the fix.
+    let _ = map.clone().into_values().map(|ref val| ref_acceptor(val)).count();
+
+    // Preserve the mut in the fix.
+    let _ = map
+        .clone().into_values().map(|mut val| {
+            val += 2;
+            val
+        })
+        .count();
+
+    // Don't let a mut interfere.
+    let _ = map.clone().into_values().count();
 }

tests/ui/iter_kv_map.rs

@@ -1,14 +1,15 @@
 // run-rustfix
 
 #![warn(clippy::iter_kv_map)]
-#![allow(clippy::redundant_clone)]
-#![allow(clippy::suspicious_map)]
-#![allow(clippy::map_identity)]
+#![allow(unused_mut, clippy::redundant_clone, clippy::suspicious_map, clippy::map_identity)]
 
 use std::collections::{BTreeMap, HashMap};
 
 fn main() {
     let get_key = |(key, _val)| key;
+    fn ref_acceptor(v: &u32) -> u32 {
+        *v
+    }
 
     let map: HashMap<u32, u32> = HashMap::new();
 
@@ -36,6 +37,22 @@ fn main() {
     let _ = map.iter().map(|(key, _value)| key * 9).count();
     let _ = map.iter().map(|(_key, value)| value * 17).count();
 
+    // Preserve the ref in the fix.
+    let _ = map.clone().into_iter().map(|(_, ref val)| ref_acceptor(val)).count();
+
+    // Preserve the mut in the fix.
+    let _ = map
+        .clone()
+        .into_iter()
+        .map(|(_, mut val)| {
+            val += 2;
+            val
+        })
+        .count();
+
+    // Don't let a mut interfere.
+    let _ = map.clone().into_iter().map(|(_, mut val)| val).count();
+
     let map: BTreeMap<u32, u32> = BTreeMap::new();
 
     let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
@@ -61,4 +78,20 @@ fn main() {
     // Lint
     let _ = map.iter().map(|(key, _value)| key * 9).count();
     let _ = map.iter().map(|(_key, value)| value * 17).count();
+
+    // Preserve the ref in the fix.
+    let _ = map.clone().into_iter().map(|(_, ref val)| ref_acceptor(val)).count();
+
+    // Preserve the mut in the fix.
+    let _ = map
+        .clone()
+        .into_iter()
+        .map(|(_, mut val)| {
+            val += 2;
+            val
+        })
+        .count();
+
+    // Don't let a mut interfere.
+    let _ = map.clone().into_iter().map(|(_, mut val)| val).count();
 }