Skip to content

Commit 5afc261

Browse files
kartvakartva
committed
Add iter_kv_map lint
1 parent 69f6009 commit 5afc261

File tree

11 files changed

+428
-0
lines changed

11 files changed

+428
-0
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3800,6 +3800,7 @@ Released 2018-09-13
38003800
[`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements
38013801
[`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect
38023802
[`iter_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_count
3803+
[`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map
38033804
[`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop
38043805
[`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice
38053806
[`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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
171171
LintId::of(methods::ITERATOR_STEP_BY_ZERO),
172172
LintId::of(methods::ITER_CLONED_COLLECT),
173173
LintId::of(methods::ITER_COUNT),
174+
LintId::of(methods::ITER_KV_MAP),
174175
LintId::of(methods::ITER_NEXT_SLICE),
175176
LintId::of(methods::ITER_NTH),
176177
LintId::of(methods::ITER_NTH_ZERO),

‎clippy_lints/src/lib.register_complexity.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
4040
LintId::of(methods::GET_LAST_WITH_LEN),
4141
LintId::of(methods::INSPECT_FOR_EACH),
4242
LintId::of(methods::ITER_COUNT),
43+
LintId::of(methods::ITER_KV_MAP),
4344
LintId::of(methods::MANUAL_FILTER_MAP),
4445
LintId::of(methods::MANUAL_FIND_MAP),
4546
LintId::of(methods::MANUAL_SPLIT_ONCE),

‎clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,7 @@ store.register_lints(&[
313313
methods::ITERATOR_STEP_BY_ZERO,
314314
methods::ITER_CLONED_COLLECT,
315315
methods::ITER_COUNT,
316+
methods::ITER_KV_MAP,
316317
methods::ITER_NEXT_SLICE,
317318
methods::ITER_NTH,
318319
methods::ITER_NTH_ZERO,

‎clippy_lints/src/methods/iter_kv_map.rs

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
#![allow(unused_imports)]
2+
3+
use super::ITER_KV_MAP;
4+
use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_sugg, span_lint_and_then};
5+
use clippy_utils::source::{snippet, snippet_with_applicability};
6+
use clippy_utils::sugg;
7+
use clippy_utils::ty::is_type_diagnostic_item;
8+
use clippy_utils::visitors::is_local_used;
9+
use rustc_hir::{BindingAnnotation, Body, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind};
10+
use rustc_lint::{LateContext, LintContext};
11+
use rustc_middle::ty;
12+
use rustc_span::sym;
13+
use rustc_span::Span;
14+
15+
/// lint use of:
16+
/// - `hashmap.iter().map(|(_, v)| v)`
17+
/// - `hashmap.into_iter().map(|(_, v)| v)`
18+
/// on `HashMaps` and `BTreeMaps` in std
19+
20+
pub(super) fn check<'tcx>(
21+
cx: &LateContext<'tcx>,
22+
map_type: &'tcx str, // iter / into_iter
23+
expr: &'tcx Expr<'tcx>, // .iter().map(|(_, v_| v))
24+
recv: &'tcx Expr<'tcx>, // hashmap
25+
m_arg: &'tcx Expr<'tcx>, // |(_, v)| v
26+
) {
27+
if_chain! {
28+
if !expr.span.from_expansion();
29+
if let ExprKind::Closure(c) = m_arg.kind;
30+
if let Body {params: [p], value: body_expr, generator_kind: _ } = cx.tcx.hir().body(c.body);
31+
if let PatKind::Tuple([key_pat, val_pat], _) = p.pat.kind;
32+
33+
let (replacement_kind, binded_ident) = match (&key_pat.kind, &val_pat.kind) {
34+
(key, PatKind::Binding(_, _, value, _)) if pat_is_wild(cx, key, m_arg) => ("value", value),
35+
(PatKind::Binding(_, _, key, _), value) if pat_is_wild(cx, value, m_arg) => ("key", key),
36+
_ => return,
37+
};
38+
39+
let ty = cx.typeck_results().expr_ty(recv);
40+
if is_type_diagnostic_item(cx, ty, sym::HashMap) || is_type_diagnostic_item(cx, ty, sym::BTreeMap);
41+
42+
then {
43+
let mut applicability = rustc_errors::Applicability::MachineApplicable;
44+
let recv_snippet = snippet_with_applicability(cx, recv.span, "map", &mut applicability);
45+
let into_prefix = if map_type == "into_iter" {"into_"} else {""};
46+
47+
if_chain! {
48+
if let ExprKind::Path(rustc_hir::QPath::Resolved(_, path)) = body_expr.kind;
49+
if let [local_ident] = path.segments;
50+
if local_ident.ident.as_str() == binded_ident.as_str();
51+
52+
then {
53+
span_lint_and_sugg(
54+
cx,
55+
ITER_KV_MAP,
56+
expr.span,
57+
&format!("iterating on a map's {}s", replacement_kind),
58+
"try",
59+
format!("{}.{}{}s()", recv_snippet, into_prefix, replacement_kind),
60+
applicability,
61+
);
62+
} else {
63+
span_lint_and_sugg(
64+
cx,
65+
ITER_KV_MAP,
66+
expr.span,
67+
&format!("iterating on a map's {}s", replacement_kind),
68+
"try",
69+
format!("{}.{}{}s().map(|{}| {})", recv_snippet, into_prefix, replacement_kind, binded_ident,
70+
snippet_with_applicability(cx, body_expr.span, "/* body */", &mut applicability)),
71+
applicability,
72+
);
73+
}
74+
}
75+
}
76+
}
77+
}
78+
79+
/// Returns `true` if the pattern is a `PatWild`, or is an ident prefixed with `_`
80+
/// that is not locally used.
81+
fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
82+
match *pat {
83+
PatKind::Wild => true,
84+
PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id),
85+
_ => false,
86+
}
87+
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ mod into_iter_on_ref;
3535
mod is_digit_ascii_radix;
3636
mod iter_cloned_collect;
3737
mod iter_count;
38+
mod iter_kv_map;
3839
mod iter_next_slice;
3940
mod iter_nth;
4041
mod iter_nth_zero;
@@ -3036,6 +3037,37 @@ declare_clippy_lint! {
30363037
"use of `File::read_to_end` or `File::read_to_string`"
30373038
}
30383039

3040+
declare_clippy_lint! {
3041+
/// ### What it does
3042+
///
3043+
/// Checks for iterating a map (`HashMap` or `BTreeMap`) and
3044+
/// ignoring either the keys or values.
3045+
///
3046+
/// ### Why is this bad?
3047+
///
3048+
/// Readability. There are `keys` and `values` methods that
3049+
/// can be used to express that we only need the keys or the values.
3050+
///
3051+
/// ### Example
3052+
///
3053+
/// ```
3054+
/// # use std::collections::HashMap;
3055+
/// let map: HashMap<u32, u32> = HashMap::new();
3056+
/// let values = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
3057+
/// ```
3058+
///
3059+
/// Use instead:
3060+
/// ```
3061+
/// # use std::collections::HashMap;
3062+
/// let map: HashMap<u32, u32> = HashMap::new();
3063+
/// let values = map.values().collect::<Vec<_>>();
3064+
/// ```
3065+
#[clippy::version = "1.65.0"]
3066+
pub ITER_KV_MAP,
3067+
complexity,
3068+
"iterating on map using `iter` when `keys` or `values` would do"
3069+
}
3070+
30393071
pub struct Methods {
30403072
avoid_breaking_exported_api: bool,
30413073
msrv: Option<RustcVersion>,
@@ -3159,6 +3191,7 @@ impl_lint_pass!(Methods => [
31593191
UNNECESSARY_SORT_BY,
31603192
VEC_RESIZE_TO_ZERO,
31613193
VERBOSE_FILE_READS,
3194+
ITER_KV_MAP,
31623195
]);
31633196

31643197
/// Extracts a method call name, args, and `Span` of the method name.
@@ -3498,6 +3531,9 @@ impl Methods {
34983531
(name @ ("map" | "map_err"), [m_arg]) => {
34993532
if name == "map" {
35003533
map_clone::check(cx, expr, recv, m_arg, self.msrv);
3534+
if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _)) = method_call(recv) {
3535+
iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
3536+
}
35013537
} else {
35023538
map_err_ignore::check(cx, expr, m_arg);
35033539
}

‎src/docs.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ docs! {
221221
"items_after_statements",
222222
"iter_cloned_collect",
223223
"iter_count",
224+
"iter_kv_map",
224225
"iter_next_loop",
225226
"iter_next_slice",
226227
"iter_not_returning_iterator",

‎src/docs/iter_kv_map.txt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
### What it does
2+
3+
Checks for iterating a map (`HashMap` or `BTreeMap`) and
4+
ignoring either the keys or values.
5+
6+
### Why is this bad?
7+
8+
Readability. There are `keys` and `values` methods that
9+
can be used to express that we only need the keys or the values.
10+
11+
### Example
12+
13+
```
14+
let map: HashMap<u32, u32> = HashMap::new();
15+
let values = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
16+
```
17+
18+
Use instead:
19+
```
20+
let map: HashMap<u32, u32> = HashMap::new();
21+
let values = map.values().collect::<Vec<_>>();
22+
```

‎tests/ui/iter_kv_map.fixed

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::iter_kv_map)]
4+
#![allow(clippy::redundant_clone)]
5+
#![allow(clippy::suspicious_map)]
6+
7+
use std::collections::{BTreeMap, HashMap};
8+
9+
fn main() {
10+
let get_key = |(key, _val)| key;
11+
12+
let map: HashMap<u32, u32> = HashMap::new();
13+
14+
let _ = map.keys().collect::<Vec<_>>();
15+
let _ = map.values().collect::<Vec<_>>();
16+
let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
17+
let _ = map.values().map(|val| {val}).collect::<Vec<_>>();
18+
19+
let _ = map.clone().into_keys().collect::<Vec<_>>();
20+
let _ = map.clone().into_keys().map(|key| key + 2).collect::<Vec<_>>();
21+
22+
let _ = map.clone().into_values().collect::<Vec<_>>();
23+
let _ = map.clone().into_values().map(|val| val + 2).collect::<Vec<_>>();
24+
25+
let _ = map.clone().values().collect::<Vec<_>>();
26+
map.keys().filter(|x| *x % 2 == 0).count();
27+
28+
// Don't lint
29+
map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
30+
map.iter().map(get_key).collect::<Vec<_>>();
31+
32+
// Linting the following could be an improvement to the lint
33+
// map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
34+
35+
// Lint
36+
map.keys().map(|key| key * 9).count();
37+
map.values().map(|value| value * 17).count();
38+
39+
let map: BTreeMap<u32, u32> = BTreeMap::new();
40+
41+
let _ = map.keys().collect::<Vec<_>>();
42+
let _ = map.values().collect::<Vec<_>>();
43+
let _ = map.values().map(|v| v + 2).collect::<Vec<_>>();
44+
let _ = map.values().map(|val| {val}).collect::<Vec<_>>();
45+
46+
let _ = map.clone().into_keys().collect::<Vec<_>>();
47+
let _ = map.clone().into_keys().map(|key| key + 2).collect::<Vec<_>>();
48+
49+
let _ = map.clone().into_values().collect::<Vec<_>>();
50+
let _ = map.clone().into_values().map(|val| val + 2).collect::<Vec<_>>();
51+
52+
let _ = map.clone().values().collect::<Vec<_>>();
53+
map.keys().filter(|x| *x % 2 == 0).count();
54+
55+
// Don't lint
56+
map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
57+
map.iter().map(get_key).collect::<Vec<_>>();
58+
59+
// Linting the following could be an improvement to the lint
60+
// map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
61+
62+
// Lint
63+
map.keys().map(|key| key * 9).count();
64+
map.values().map(|value| value * 17).count();
65+
}

‎tests/ui/iter_kv_map.rs

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::iter_kv_map)]
4+
#![allow(clippy::redundant_clone)]
5+
#![allow(clippy::suspicious_map)]
6+
7+
use std::collections::{BTreeMap, HashMap};
8+
9+
fn main() {
10+
let get_key = |(key, _val)| key;
11+
12+
let map: HashMap<u32, u32> = HashMap::new();
13+
14+
let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
15+
let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
16+
let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
17+
let _ = map.iter().map(|(_, val)| val).collect::<Vec<_>>();
18+
19+
let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
20+
let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
21+
22+
let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
23+
let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
24+
25+
let _ = map.clone().iter().map(|(_, val)| val).collect::<Vec<_>>();
26+
map.iter().map(|(key, _)| key).filter(|x| *x % 2 == 0).count();
27+
28+
// Don't lint
29+
map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
30+
map.iter().map(get_key).collect::<Vec<_>>();
31+
32+
// Linting the following could be an improvement to the lint
33+
// map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
34+
35+
// Lint
36+
map.iter().map(|(key, _value)| key * 9).count();
37+
map.iter().map(|(_key, value)| value * 17).count();
38+
39+
let map: BTreeMap<u32, u32> = BTreeMap::new();
40+
41+
let _ = map.iter().map(|(key, _)| key).collect::<Vec<_>>();
42+
let _ = map.iter().map(|(_, value)| value).collect::<Vec<_>>();
43+
let _ = map.iter().map(|(_, v)| v + 2).collect::<Vec<_>>();
44+
let _ = map.iter().map(|(_, val)| val).collect::<Vec<_>>();
45+
46+
let _ = map.clone().into_iter().map(|(key, _)| key).collect::<Vec<_>>();
47+
let _ = map.clone().into_iter().map(|(key, _)| key + 2).collect::<Vec<_>>();
48+
49+
let _ = map.clone().into_iter().map(|(_, val)| val).collect::<Vec<_>>();
50+
let _ = map.clone().into_iter().map(|(_, val)| val + 2).collect::<Vec<_>>();
51+
52+
let _ = map.clone().iter().map(|(_, val)| val).collect::<Vec<_>>();
53+
map.iter().map(|(key, _)| key).filter(|x| *x % 2 == 0).count();
54+
55+
// Don't lint
56+
map.iter().filter(|(_, val)| *val % 2 == 0).map(|(key, _)| key).count();
57+
map.iter().map(get_key).collect::<Vec<_>>();
58+
59+
// Linting the following could be an improvement to the lint
60+
// map.iter().filter_map(|(_, val)| (val % 2 == 0).then(val * 17)).count();
61+
62+
// Lint
63+
map.iter().map(|(key, _value)| key * 9).count();
64+
map.iter().map(|(_key, value)| value * 17).count();
65+
}

0 commit comments

Comments
 (0)