Skip to content

Commit 1cfc95c

Browse files
authored
fix: map_entry: don't emit lint before checks have been performed (#14568)
Fixes #14449, introduced in #14314 changelog: [`map_entry`]: fix a false positive where the lint would trigger without any insert calls present
2 parents e463309 + 189b076 commit 1cfc95c

File tree

3 files changed

+62
-15
lines changed

3 files changed

+62
-15
lines changed

Diff for: clippy_lints/src/entry.rs

+18-15
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,13 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
9595
return;
9696
};
9797

98-
if then_search.is_key_used_and_no_copy || else_search.is_key_used_and_no_copy {
99-
span_lint(cx, MAP_ENTRY, expr.span, lint_msg);
100-
return;
101-
}
102-
10398
if then_search.edits.is_empty() && else_search.edits.is_empty() {
10499
// No insertions
105100
return;
101+
} else if then_search.is_key_used_and_no_copy || else_search.is_key_used_and_no_copy {
102+
// If there are other uses of the key, and the key is not copy,
103+
// we cannot perform a fix automatically, but continue to emit a lint.
104+
None
106105
} else if then_search.edits.is_empty() || else_search.edits.is_empty() {
107106
// if .. { insert } else { .. } or if .. { .. } else { insert }
108107
let ((then_str, entry_kind), else_str) = match (else_search.edits.is_empty(), contains_expr.negated) {
@@ -123,10 +122,10 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
123122
snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
124123
),
125124
};
126-
format!(
125+
Some(format!(
127126
"if let {}::{entry_kind} = {map_str}.entry({key_str}) {then_str} else {else_str}",
128127
map_ty.entry_path(),
129-
)
128+
))
130129
} else {
131130
// if .. { insert } else { insert }
132131
let ((then_str, then_entry), (else_str, else_entry)) = if contains_expr.negated {
@@ -142,13 +141,13 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
142141
};
143142
let indent_str = snippet_indent(cx, expr.span);
144143
let indent_str = indent_str.as_deref().unwrap_or("");
145-
format!(
144+
Some(format!(
146145
"match {map_str}.entry({key_str}) {{\n{indent_str} {entry}::{then_entry} => {}\n\
147146
{indent_str} {entry}::{else_entry} => {}\n{indent_str}}}",
148147
reindent_multiline(&then_str, true, Some(4 + indent_str.len())),
149148
reindent_multiline(&else_str, true, Some(4 + indent_str.len())),
150149
entry = map_ty.entry_path(),
151-
)
150+
))
152151
}
153152
} else {
154153
if then_search.edits.is_empty() {
@@ -163,17 +162,17 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
163162
} else {
164163
then_search.snippet_occupied(cx, then_expr.span, &mut app)
165164
};
166-
format!(
165+
Some(format!(
167166
"if let {}::{entry_kind} = {map_str}.entry({key_str}) {body_str}",
168167
map_ty.entry_path(),
169-
)
168+
))
170169
} else if let Some(insertion) = then_search.as_single_insertion() {
171170
let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "..", &mut app).0;
172171
if contains_expr.negated {
173172
if insertion.value.can_have_side_effects() {
174-
format!("{map_str}.entry({key_str}).or_insert_with(|| {value_str});")
173+
Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {value_str});"))
175174
} else {
176-
format!("{map_str}.entry({key_str}).or_insert({value_str});")
175+
Some(format!("{map_str}.entry({key_str}).or_insert({value_str});"))
177176
}
178177
} else {
179178
// TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here.
@@ -183,7 +182,7 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
183182
} else {
184183
let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app);
185184
if contains_expr.negated {
186-
format!("{map_str}.entry({key_str}).or_insert_with(|| {block_str});")
185+
Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {block_str});"))
187186
} else {
188187
// TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here.
189188
// This would need to be a different lint.
@@ -192,7 +191,11 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
192191
}
193192
};
194193

195-
span_lint_and_sugg(cx, MAP_ENTRY, expr.span, lint_msg, "try", sugg, app);
194+
if let Some(sugg) = sugg {
195+
span_lint_and_sugg(cx, MAP_ENTRY, expr.span, lint_msg, "try", sugg, app);
196+
} else {
197+
span_lint(cx, MAP_ENTRY, expr.span, lint_msg);
198+
}
196199
}
197200
}
198201

Diff for: tests/ui/entry.fixed

+22
Original file line numberDiff line numberDiff line change
@@ -226,4 +226,26 @@ fn issue11976() {
226226
}
227227
}
228228

229+
mod issue14449 {
230+
use std::collections::BTreeMap;
231+
232+
pub struct Meow {
233+
map: BTreeMap<String, String>,
234+
}
235+
236+
impl Meow {
237+
fn pet(&self, _key: &str, _v: u32) -> u32 {
238+
42
239+
}
240+
}
241+
242+
pub fn f(meow: &Meow, x: String) {
243+
if meow.map.contains_key(&x) {
244+
let _ = meow.pet(&x, 1);
245+
} else {
246+
let _ = meow.pet(&x, 0);
247+
}
248+
}
249+
}
250+
229251
fn main() {}

Diff for: tests/ui/entry.rs

+22
Original file line numberDiff line numberDiff line change
@@ -232,4 +232,26 @@ fn issue11976() {
232232
}
233233
}
234234

235+
mod issue14449 {
236+
use std::collections::BTreeMap;
237+
238+
pub struct Meow {
239+
map: BTreeMap<String, String>,
240+
}
241+
242+
impl Meow {
243+
fn pet(&self, _key: &str, _v: u32) -> u32 {
244+
42
245+
}
246+
}
247+
248+
pub fn f(meow: &Meow, x: String) {
249+
if meow.map.contains_key(&x) {
250+
let _ = meow.pet(&x, 1);
251+
} else {
252+
let _ = meow.pet(&x, 0);
253+
}
254+
}
255+
}
256+
235257
fn main() {}

0 commit comments

Comments
 (0)