Skip to content

Commit a4c2d8d

Browse files
JarchoJarcho
committed
map_entry improvements
Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.
1 parent 49ef8f7 commit a4c2d8d

File tree

4 files changed

+354
-2
lines changed

4 files changed

+354
-2
lines changed

‎clippy_lints/src/entry.rs

Lines changed: 79 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use clippy_utils::{
22
diagnostics::span_lint_and_sugg,
33
is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while,
4-
source::{snippet_indent, snippet_with_applicability, snippet_with_context},
4+
source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context},
55
SpanlessEq,
66
};
77
use rustc_errors::Applicability;
@@ -75,7 +75,84 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
7575
let mut app = Applicability::MachineApplicable;
7676
let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0;
7777
let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0;
78-
let sugg = if !contains_expr.negated || else_expr.is_some() || then_search.insertions.is_empty() {
78+
let sugg = if let Some(else_expr) = else_expr {
79+
// if .. { .. } else { .. }
80+
let else_search = match find_insert_calls(cx, &contains_expr, else_expr) {
81+
Some(search) if !(then_search.insertions.is_empty() && search.insertions.is_empty()) => search,
82+
_ => return,
83+
};
84+
85+
if then_search.insertions.is_empty() || else_search.insertions.is_empty() {
86+
// if .. { insert } else { .. } or if .. { .. } else { then } of
87+
let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() {
88+
if contains_expr.negated {
89+
(
90+
then_search.snippet_vacant(cx, then_expr.span, &mut app),
91+
snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
92+
"Vacant(e)",
93+
)
94+
} else {
95+
(
96+
then_search.snippet_occupied(cx, then_expr.span, &mut app),
97+
snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
98+
"Occupied(mut e)",
99+
)
100+
}
101+
} else if contains_expr.negated {
102+
(
103+
else_search.snippet_occupied(cx, else_expr.span, &mut app),
104+
snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
105+
"Occupied(mut e)",
106+
)
107+
} else {
108+
(
109+
else_search.snippet_vacant(cx, else_expr.span, &mut app),
110+
snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
111+
"Vacant(e)",
112+
)
113+
};
114+
format!(
115+
"if let {}::{} = {}.entry({}) {} else {}",
116+
map_ty.entry_path(),
117+
entry_kind,
118+
map_str,
119+
key_str,
120+
then_str,
121+
else_str,
122+
)
123+
} else {
124+
// if .. { insert } else { insert }
125+
let (then_str, else_str, then_entry, else_entry) = if contains_expr.negated {
126+
(
127+
then_search.snippet_vacant(cx, then_expr.span, &mut app),
128+
else_search.snippet_occupied(cx, else_expr.span, &mut app),
129+
"Vacant(e)",
130+
"Occupied(mut e)",
131+
)
132+
} else {
133+
(
134+
then_search.snippet_occupied(cx, then_expr.span, &mut app),
135+
else_search.snippet_vacant(cx, else_expr.span, &mut app),
136+
"Occupied(mut e)",
137+
"Vacant(e)",
138+
)
139+
};
140+
let indent_str = snippet_indent(cx, expr.span);
141+
let indent_str = indent_str.as_deref().unwrap_or("");
142+
format!(
143+
"match {}.entry({}) {{\n{indent} {entry}::{} => {}\n\
144+
{indent} {entry}::{} => {}\n{indent}}}",
145+
map_str,
146+
key_str,
147+
then_entry,
148+
reindent_multiline(then_str.into(), true, Some(4 + indent_str.len())),
149+
else_entry,
150+
reindent_multiline(else_str.into(), true, Some(4 + indent_str.len())),
151+
entry = map_ty.entry_path(),
152+
indent = indent_str,
153+
)
154+
}
155+
} else if then_search.insertions.is_empty() || !contains_expr.negated {
79156
return;
80157
} else {
81158
// if .. { insert }

‎tests/ui/entry_with_else.fixed

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
4+
#![warn(clippy::map_entry)]
5+
6+
use std::collections::{BTreeMap, HashMap};
7+
use std::hash::Hash;
8+
9+
macro_rules! m {
10+
($e:expr) => {{ $e }};
11+
}
12+
13+
fn foo() {}
14+
15+
fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
16+
match m.entry(k) {
17+
std::collections::hash_map::Entry::Vacant(e) => {
18+
e.insert(v);
19+
}
20+
std::collections::hash_map::Entry::Occupied(mut e) => {
21+
e.insert(v2);
22+
}
23+
}
24+
25+
match m.entry(k) {
26+
std::collections::hash_map::Entry::Occupied(mut e) => {
27+
e.insert(v);
28+
}
29+
std::collections::hash_map::Entry::Vacant(e) => {
30+
e.insert(v2);
31+
}
32+
}
33+
34+
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
35+
e.insert(v);
36+
} else {
37+
foo();
38+
}
39+
40+
if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
41+
e.insert(v);
42+
} else {
43+
foo();
44+
}
45+
46+
match m.entry(k) {
47+
std::collections::hash_map::Entry::Vacant(e) => {
48+
e.insert(v);
49+
}
50+
std::collections::hash_map::Entry::Occupied(mut e) => {
51+
e.insert(v2);
52+
}
53+
}
54+
55+
match m.entry(k) {
56+
std::collections::hash_map::Entry::Occupied(mut e) => {
57+
if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
58+
}
59+
std::collections::hash_map::Entry::Vacant(e) => {
60+
e.insert(v);
61+
None
62+
}
63+
};
64+
65+
if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
66+
foo();
67+
Some(e.insert(v))
68+
} else {
69+
None
70+
};
71+
}
72+
73+
fn main() {}

‎tests/ui/entry_with_else.rs

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
4+
#![warn(clippy::map_entry)]
5+
6+
use std::collections::{BTreeMap, HashMap};
7+
use std::hash::Hash;
8+
9+
macro_rules! m {
10+
($e:expr) => {{ $e }};
11+
}
12+
13+
fn foo() {}
14+
15+
fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
16+
if !m.contains_key(&k) {
17+
m.insert(k, v);
18+
} else {
19+
m.insert(k, v2);
20+
}
21+
22+
if m.contains_key(&k) {
23+
m.insert(k, v);
24+
} else {
25+
m.insert(k, v2);
26+
}
27+
28+
if !m.contains_key(&k) {
29+
m.insert(k, v);
30+
} else {
31+
foo();
32+
}
33+
34+
if !m.contains_key(&k) {
35+
foo();
36+
} else {
37+
m.insert(k, v);
38+
}
39+
40+
if !m.contains_key(&k) {
41+
m.insert(k, v);
42+
} else {
43+
m.insert(k, v2);
44+
}
45+
46+
if m.contains_key(&k) {
47+
if true { m.insert(k, v) } else { m.insert(k, v2) }
48+
} else {
49+
m.insert(k, v)
50+
};
51+
52+
if m.contains_key(&k) {
53+
foo();
54+
m.insert(k, v)
55+
} else {
56+
None
57+
};
58+
}
59+
60+
fn main() {}

‎tests/ui/entry_with_else.stderr

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
error: usage of `contains_key` followed by `insert` on a `HashMap`
2+
--> $DIR/entry_with_else.rs:16:5
3+
|
4+
LL | / if !m.contains_key(&k) {
5+
LL | | m.insert(k, v);
6+
LL | | } else {
7+
LL | | m.insert(k, v2);
8+
LL | | }
9+
| |_____^
10+
|
11+
= note: `-D clippy::map-entry` implied by `-D warnings`
12+
help: try this
13+
|
14+
LL | match m.entry(k) {
15+
LL | std::collections::hash_map::Entry::Vacant(e) => {
16+
LL | e.insert(v);
17+
LL | }
18+
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
19+
LL | e.insert(v2);
20+
...
21+
22+
error: usage of `contains_key` followed by `insert` on a `HashMap`
23+
--> $DIR/entry_with_else.rs:22:5
24+
|
25+
LL | / if m.contains_key(&k) {
26+
LL | | m.insert(k, v);
27+
LL | | } else {
28+
LL | | m.insert(k, v2);
29+
LL | | }
30+
| |_____^
31+
|
32+
help: try this
33+
|
34+
LL | match m.entry(k) {
35+
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
36+
LL | e.insert(v);
37+
LL | }
38+
LL | std::collections::hash_map::Entry::Vacant(e) => {
39+
LL | e.insert(v2);
40+
...
41+
42+
error: usage of `contains_key` followed by `insert` on a `HashMap`
43+
--> $DIR/entry_with_else.rs:28:5
44+
|
45+
LL | / if !m.contains_key(&k) {
46+
LL | | m.insert(k, v);
47+
LL | | } else {
48+
LL | | foo();
49+
LL | | }
50+
| |_____^
51+
|
52+
help: try this
53+
|
54+
LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
55+
LL | e.insert(v);
56+
LL | } else {
57+
LL | foo();
58+
LL | }
59+
|
60+
61+
error: usage of `contains_key` followed by `insert` on a `HashMap`
62+
--> $DIR/entry_with_else.rs:34:5
63+
|
64+
LL | / if !m.contains_key(&k) {
65+
LL | | foo();
66+
LL | | } else {
67+
LL | | m.insert(k, v);
68+
LL | | }
69+
| |_____^
70+
|
71+
help: try this
72+
|
73+
LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
74+
LL | e.insert(v);
75+
LL | } else {
76+
LL | foo();
77+
LL | }
78+
|
79+
80+
error: usage of `contains_key` followed by `insert` on a `HashMap`
81+
--> $DIR/entry_with_else.rs:40:5
82+
|
83+
LL | / if !m.contains_key(&k) {
84+
LL | | m.insert(k, v);
85+
LL | | } else {
86+
LL | | m.insert(k, v2);
87+
LL | | }
88+
| |_____^
89+
|
90+
help: try this
91+
|
92+
LL | match m.entry(k) {
93+
LL | std::collections::hash_map::Entry::Vacant(e) => {
94+
LL | e.insert(v);
95+
LL | }
96+
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
97+
LL | e.insert(v2);
98+
...
99+
100+
error: usage of `contains_key` followed by `insert` on a `HashMap`
101+
--> $DIR/entry_with_else.rs:46:5
102+
|
103+
LL | / if m.contains_key(&k) {
104+
LL | | if true { m.insert(k, v) } else { m.insert(k, v2) }
105+
LL | | } else {
106+
LL | | m.insert(k, v)
107+
LL | | };
108+
| |_____^
109+
|
110+
help: try this
111+
|
112+
LL | match m.entry(k) {
113+
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
114+
LL | if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
115+
LL | }
116+
LL | std::collections::hash_map::Entry::Vacant(e) => {
117+
LL | e.insert(v);
118+
...
119+
120+
error: usage of `contains_key` followed by `insert` on a `HashMap`
121+
--> $DIR/entry_with_else.rs:52:5
122+
|
123+
LL | / if m.contains_key(&k) {
124+
LL | | foo();
125+
LL | | m.insert(k, v)
126+
LL | | } else {
127+
LL | | None
128+
LL | | };
129+
| |_____^
130+
|
131+
help: try this
132+
|
133+
LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
134+
LL | foo();
135+
LL | Some(e.insert(v))
136+
LL | } else {
137+
LL | None
138+
LL | };
139+
|
140+
141+
error: aborting due to 7 previous errors
142+

0 commit comments

Comments
 (0)