Auto merge of #9409 - DesmondWillowbrook:iter_kv_map, r=xFrednet

Add `iter_kv_map` lint

fixes #9376

| before | after |
| -------------- | ------------------------- |
| `hmap.iter().map(\|(key, _)\| key)` | `hmap.keys()` |
| `hmap.iter().map(\|(_, v)\| v + 2)` | `hmap.values().map(\|v\| v + 2)` |
| `hmap.into_iter().map(\|(key, _)\| key)` | `hmap.into_keys()` |

Is `MachineApplicable`

changelog: [`iter_kv_map`]: added lint

Author: bors

CHANGELOG.md

@@ -3800,6 +3800,7 @@ Released 2018-09-13
 [`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements
 [`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect
 [`iter_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_count
+[`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map
 [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop
 [`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice
 [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator

clippy_lints/src/lib.register_all.rs

@@ -171,6 +171,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::ITERATOR_STEP_BY_ZERO),
     LintId::of(methods::ITER_CLONED_COLLECT),
     LintId::of(methods::ITER_COUNT),
+    LintId::of(methods::ITER_KV_MAP),
     LintId::of(methods::ITER_NEXT_SLICE),
     LintId::of(methods::ITER_NTH),
     LintId::of(methods::ITER_NTH_ZERO),

clippy_lints/src/lib.register_complexity.rs

@@ -40,6 +40,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(methods::GET_LAST_WITH_LEN),
     LintId::of(methods::INSPECT_FOR_EACH),
     LintId::of(methods::ITER_COUNT),
+    LintId::of(methods::ITER_KV_MAP),
     LintId::of(methods::MANUAL_FILTER_MAP),
     LintId::of(methods::MANUAL_FIND_MAP),
     LintId::of(methods::MANUAL_SPLIT_ONCE),

clippy_lints/src/lib.register_lints.rs

@@ -313,6 +313,7 @@ store.register_lints(&[
     methods::ITERATOR_STEP_BY_ZERO,
     methods::ITER_CLONED_COLLECT,
     methods::ITER_COUNT,
+    methods::ITER_KV_MAP,
     methods::ITER_NEXT_SLICE,
     methods::ITER_NTH,
     methods::ITER_NTH_ZERO,

clippy_lints/src/methods/iter_kv_map.rs

@@ -0,0 +1,87 @@
+#![allow(unused_imports)]
+
+use super::ITER_KV_MAP;
+use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_sugg, span_lint_and_then};
+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_lint::{LateContext, LintContext};
+use rustc_middle::ty;
+use rustc_span::sym;
+use rustc_span::Span;
+
+/// lint use of:
+/// - `hashmap.iter().map(|(_, v)| v)`
+/// - `hashmap.into_iter().map(|(_, v)| v)`
+/// on `HashMaps` and `BTreeMaps` in std
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    map_type: &'tcx str,     // iter / into_iter
+    expr: &'tcx Expr<'tcx>,  // .iter().map(|(_, v_| v))
+    recv: &'tcx Expr<'tcx>,  // hashmap
+    m_arg: &'tcx Expr<'tcx>, // |(_, v)| v
+) {
+    if_chain! {
+        if !expr.span.from_expansion();
+        if let ExprKind::Closure(c) = m_arg.kind;
+        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),
+            _ => return,
+        };
+
+        let ty = cx.typeck_results().expr_ty(recv);
+        if is_type_diagnostic_item(cx, ty, sym::HashMap) || is_type_diagnostic_item(cx, ty, sym::BTreeMap);
+
+        then {
+            let mut applicability = rustc_errors::Applicability::MachineApplicable;
+            let recv_snippet = snippet_with_applicability(cx, recv.span, "map", &mut applicability);
+            let into_prefix = if map_type == "into_iter" {"into_"} else {""};
+
+            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();
+
+                then {
+                    span_lint_and_sugg(
+                        cx,
+                        ITER_KV_MAP,
+                        expr.span,
+                        &format!("iterating on a map's {}s", replacement_kind),
+                        "try",
+                        format!("{}.{}{}s()", recv_snippet, into_prefix, replacement_kind),
+                        applicability,
+                    );
+                } else {
+                    span_lint_and_sugg(
+                        cx,
+                        ITER_KV_MAP,
+                        expr.span,
+                        &format!("iterating on a map's {}s", replacement_kind),
+                        "try",
+                        format!("{}.{}{}s().map(|{}| {})", recv_snippet, into_prefix, replacement_kind, binded_ident,
+                            snippet_with_applicability(cx, body_expr.span, "/* body */", &mut applicability)),
+                        applicability,
+                    );
+                }
+            }
+        }
+    }
+}
+
+/// Returns `true` if the pattern is a `PatWild`, or is an ident prefixed with `_`
+/// that is not locally used.
+fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
+    match *pat {
+        PatKind::Wild => true,
+        PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id),
+        _ => false,
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -35,6 +35,7 @@ mod into_iter_on_ref;
 mod is_digit_ascii_radix;
 mod iter_cloned_collect;
 mod iter_count;
+mod iter_kv_map;
 mod iter_next_slice;
 mod iter_nth;
 mod iter_nth_zero;
@@ -3036,6 +3037,37 @@ declare_clippy_lint! {
     "use of `File::read_to_end` or `File::read_to_string`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for iterating a map (`HashMap` or `BTreeMap`) and
+    /// ignoring either the keys or values.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Readability. There are `keys` and `values` methods that
+    /// can be used to express that we only need the keys or the values.
+    ///
+    /// ### Example
+    ///
+    /// ```
+    /// # use std::collections::HashMap;
+    /// let map: HashMap<u32, u32> = HashMap::new();
+    /// let values = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
+    /// ```
+    ///
+    /// Use instead:
+    /// ```
+    /// # use std::collections::HashMap;
+    /// let map: HashMap<u32, u32> = HashMap::new();
+    /// let values = map.values().collect::<Vec<_>>();
+    /// ```
+    #[clippy::version = "1.65.0"]
+    pub ITER_KV_MAP,
+    complexity,
+    "iterating on map using `iter` when `keys` or `values` would do"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Option<RustcVersion>,
@@ -3159,6 +3191,7 @@ impl_lint_pass!(Methods => [
     UNNECESSARY_SORT_BY,
     VEC_RESIZE_TO_ZERO,
     VERBOSE_FILE_READS,
+    ITER_KV_MAP,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3498,6 +3531,9 @@ impl Methods {
                 (name @ ("map" | "map_err"), [m_arg]) => {
                     if name == "map" {
                         map_clone::check(cx, expr, recv, m_arg, self.msrv);
+                        if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _)) = method_call(recv) {
+                            iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
+                        }
                     } else {
                         map_err_ignore::check(cx, expr, m_arg);
                     }

src/docs.rs

@@ -221,6 +221,7 @@ docs! {
     "items_after_statements",
     "iter_cloned_collect",
     "iter_count",
+    "iter_kv_map",
     "iter_next_loop",
     "iter_next_slice",
     "iter_not_returning_iterator",

src/docs/iter_kv_map.txt

@@ -0,0 +1,22 @@
+### What it does
+
+Checks for iterating a map (`HashMap` or `BTreeMap`) and
+ignoring either the keys or values.
+
+### Why is this bad?
+
+Readability. There are `keys` and `values` methods that
+can be used to express that we only need the keys or the values.
+
+### Example
+
+```
+let map: HashMap<u32, u32> = HashMap::new();
+let values = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
+```
+
+Use instead:
+```
+let map: HashMap<u32, u32> = HashMap::new();
+let values = map.values().collect::<Vec<_>>();
+```

tests/ui/iter_kv_map.fixed

@@ -0,0 +1,64 @@
+// run-rustfix
+
+#![warn(clippy::iter_kv_map)]
+#![allow(clippy::redundant_clone)]
+#![allow(clippy::suspicious_map)]
+#![allow(clippy::map_identity)]
+
+use std::collections::{BTreeMap, HashMap};
+
+fn main() {
+    let get_key = |(key, _val)| key;
+
+    let map: HashMap<u32, u32> = HashMap::new();
+
+    let _ = map.keys().collect::<Vec<_>>();
+    let _ = map.values().collect::<Vec<_>>();
+    let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_keys().collect::<Vec<_>>();
+    let _ = map.clone().into_keys().map(|key| key + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_values().collect::<Vec<_>>();
+    let _ = map.clone().into_values().map(|val| val + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().values().collect::<Vec<_>>();
+    let _ = map.keys().filter(|x| *x % 2 == 0).count();
+
+    // Don't lint
+    let _ = map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
+    let _ = map.iter().map(get_key).collect::<Vec<_>>();
+
+    // Linting the following could be an improvement to the lint
+    // map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
+
+    // Lint
+    let _ = map.keys().map(|key| key * 9).count();
+    let _ = map.values().map(|value| value * 17).count();
+
+    let map: BTreeMap<u32, u32> = BTreeMap::new();
+
+    let _ = map.keys().collect::<Vec<_>>();
+    let _ = map.values().collect::<Vec<_>>();
+    let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_keys().collect::<Vec<_>>();
+    let _ = map.clone().into_keys().map(|key| key + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_values().collect::<Vec<_>>();
+    let _ = map.clone().into_values().map(|val| val + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().values().collect::<Vec<_>>();
+    let _ = map.keys().filter(|x| *x % 2 == 0).count();
+
+    // Don't lint
+    let _ = map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
+    let _ = map.iter().map(get_key).collect::<Vec<_>>();
+
+    // Linting the following could be an improvement to the lint
+    // map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
+
+    // Lint
+    let _ = map.keys().map(|key| key * 9).count();
+    let _ = map.values().map(|value| value * 17).count();
+}

tests/ui/iter_kv_map.rs

@@ -0,0 +1,64 @@
+// run-rustfix
+
+#![warn(clippy::iter_kv_map)]
+#![allow(clippy::redundant_clone)]
+#![allow(clippy::suspicious_map)]
+#![allow(clippy::map_identity)]
+
+use std::collections::{BTreeMap, HashMap};
+
+fn main() {
+    let get_key = |(key, _val)| key;
+
+    let map: HashMap<u32, u32> = HashMap::new();
+
+    let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
+    let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
+    let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
+    let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
+    let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().iter().map(|(_, val)| val).collect::<Vec<_>>();
+    let _ = map.iter().map(|(key, _)| key).filter(|x| *x % 2 == 0).count();
+
+    // Don't lint
+    let _ = map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
+    let _ = map.iter().map(get_key).collect::<Vec<_>>();
+
+    // Linting the following could be an improvement to the lint
+    // map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
+
+    // Lint
+    let _ = map.iter().map(|(key, _value)| key * 9).count();
+    let _ = map.iter().map(|(_key, value)| value * 17).count();
+
+    let map: BTreeMap<u32, u32> = BTreeMap::new();
+
+    let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
+    let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
+    let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
+    let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
+    let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
+
+    let _ = map.clone().iter().map(|(_, val)| val).collect::<Vec<_>>();
+    let _ = map.iter().map(|(key, _)| key).filter(|x| *x % 2 == 0).count();
+
+    // Don't lint
+    let _ = map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
+    let _ = map.iter().map(get_key).collect::<Vec<_>>();
+
+    // Linting the following could be an improvement to the lint
+    // map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
+
+    // Lint
+    let _ = map.iter().map(|(key, _value)| key * 9).count();
+    let _ = map.iter().map(|(_key, value)| value * 17).count();
+}