map_entry improvements
Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.
This commit is contained in:
Jason Newcomb
parent
ce5e927713
commit
b63a5b56d6
4 changed files with 354 additions and 2 deletions
|
|
@ -1,7 +1,7 @@
|
|||
use clippy_utils::{
|
||||
diagnostics::span_lint_and_sugg,
|
||||
is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while,
|
||||
source::{snippet_indent, snippet_with_applicability, snippet_with_context},
|
||||
source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context},
|
||||
SpanlessEq,
|
||||
};
|
||||
use rustc_errors::Applicability;
|
||||
|
|
@ -75,7 +75,84 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
|
|||
let mut app = Applicability::MachineApplicable;
|
||||
let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0;
|
||||
let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0;
|
||||
let sugg = if !contains_expr.negated || else_expr.is_some() || then_search.insertions.is_empty() {
|
||||
let sugg = if let Some(else_expr) = else_expr {
|
||||
// if .. { .. } else { .. }
|
||||
let else_search = match find_insert_calls(cx, &contains_expr, else_expr) {
|
||||
Some(search) if !(then_search.insertions.is_empty() && search.insertions.is_empty()) => search,
|
||||
_ => return,
|
||||
};
|
||||
|
||||
if then_search.insertions.is_empty() || else_search.insertions.is_empty() {
|
||||
// if .. { insert } else { .. } or if .. { .. } else { then } of
|
||||
let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() {
|
||||
if contains_expr.negated {
|
||||
(
|
||||
then_search.snippet_vacant(cx, then_expr.span, &mut app),
|
||||
snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
|
||||
"Vacant(e)",
|
||||
)
|
||||
} else {
|
||||
(
|
||||
then_search.snippet_occupied(cx, then_expr.span, &mut app),
|
||||
snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
|
||||
"Occupied(mut e)",
|
||||
)
|
||||
}
|
||||
} else if contains_expr.negated {
|
||||
(
|
||||
else_search.snippet_occupied(cx, else_expr.span, &mut app),
|
||||
snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
|
||||
"Occupied(mut e)",
|
||||
)
|
||||
} else {
|
||||
(
|
||||
else_search.snippet_vacant(cx, else_expr.span, &mut app),
|
||||
snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
|
||||
"Vacant(e)",
|
||||
)
|
||||
};
|
||||
format!(
|
||||
"if let {}::{} = {}.entry({}) {} else {}",
|
||||
map_ty.entry_path(),
|
||||
entry_kind,
|
||||
map_str,
|
||||
key_str,
|
||||
then_str,
|
||||
else_str,
|
||||
)
|
||||
} else {
|
||||
// if .. { insert } else { insert }
|
||||
let (then_str, else_str, then_entry, else_entry) = if contains_expr.negated {
|
||||
(
|
||||
then_search.snippet_vacant(cx, then_expr.span, &mut app),
|
||||
else_search.snippet_occupied(cx, else_expr.span, &mut app),
|
||||
"Vacant(e)",
|
||||
"Occupied(mut e)",
|
||||
)
|
||||
} else {
|
||||
(
|
||||
then_search.snippet_occupied(cx, then_expr.span, &mut app),
|
||||
else_search.snippet_vacant(cx, else_expr.span, &mut app),
|
||||
"Occupied(mut e)",
|
||||
"Vacant(e)",
|
||||
)
|
||||
};
|
||||
let indent_str = snippet_indent(cx, expr.span);
|
||||
let indent_str = indent_str.as_deref().unwrap_or("");
|
||||
format!(
|
||||
"match {}.entry({}) {{\n{indent} {entry}::{} => {}\n\
|
||||
{indent} {entry}::{} => {}\n{indent}}}",
|
||||
map_str,
|
||||
key_str,
|
||||
then_entry,
|
||||
reindent_multiline(then_str.into(), true, Some(4 + indent_str.len())),
|
||||
else_entry,
|
||||
reindent_multiline(else_str.into(), true, Some(4 + indent_str.len())),
|
||||
entry = map_ty.entry_path(),
|
||||
indent = indent_str,
|
||||
)
|
||||
}
|
||||
} else if then_search.insertions.is_empty() || !contains_expr.negated {
|
||||
return;
|
||||
} else {
|
||||
// if .. { insert }
|
||||
|
|
|
|||
73
tests/ui/entry_with_else.fixed
Normal file
73
tests/ui/entry_with_else.fixed
Normal file
|
|
@ -0,0 +1,73 @@
|
|||
// run-rustfix
|
||||
|
||||
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
|
||||
#![warn(clippy::map_entry)]
|
||||
|
||||
use std::collections::{BTreeMap, HashMap};
|
||||
use std::hash::Hash;
|
||||
|
||||
macro_rules! m {
|
||||
($e:expr) => {{ $e }};
|
||||
}
|
||||
|
||||
fn foo() {}
|
||||
|
||||
fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
|
||||
match m.entry(k) {
|
||||
std::collections::hash_map::Entry::Vacant(e) => {
|
||||
e.insert(v);
|
||||
}
|
||||
std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
e.insert(v2);
|
||||
}
|
||||
}
|
||||
|
||||
match m.entry(k) {
|
||||
std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
e.insert(v);
|
||||
}
|
||||
std::collections::hash_map::Entry::Vacant(e) => {
|
||||
e.insert(v2);
|
||||
}
|
||||
}
|
||||
|
||||
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
|
||||
e.insert(v);
|
||||
} else {
|
||||
foo();
|
||||
}
|
||||
|
||||
if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
|
||||
e.insert(v);
|
||||
} else {
|
||||
foo();
|
||||
}
|
||||
|
||||
match m.entry(k) {
|
||||
std::collections::hash_map::Entry::Vacant(e) => {
|
||||
e.insert(v);
|
||||
}
|
||||
std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
e.insert(v2);
|
||||
}
|
||||
}
|
||||
|
||||
match m.entry(k) {
|
||||
std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
|
||||
}
|
||||
std::collections::hash_map::Entry::Vacant(e) => {
|
||||
e.insert(v);
|
||||
None
|
||||
}
|
||||
};
|
||||
|
||||
if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
|
||||
foo();
|
||||
Some(e.insert(v))
|
||||
} else {
|
||||
None
|
||||
};
|
||||
}
|
||||
|
||||
fn main() {}
|
||||
60
tests/ui/entry_with_else.rs
Normal file
60
tests/ui/entry_with_else.rs
Normal file
|
|
@ -0,0 +1,60 @@
|
|||
// run-rustfix
|
||||
|
||||
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
|
||||
#![warn(clippy::map_entry)]
|
||||
|
||||
use std::collections::{BTreeMap, HashMap};
|
||||
use std::hash::Hash;
|
||||
|
||||
macro_rules! m {
|
||||
($e:expr) => {{ $e }};
|
||||
}
|
||||
|
||||
fn foo() {}
|
||||
|
||||
fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
|
||||
if !m.contains_key(&k) {
|
||||
m.insert(k, v);
|
||||
} else {
|
||||
m.insert(k, v2);
|
||||
}
|
||||
|
||||
if m.contains_key(&k) {
|
||||
m.insert(k, v);
|
||||
} else {
|
||||
m.insert(k, v2);
|
||||
}
|
||||
|
||||
if !m.contains_key(&k) {
|
||||
m.insert(k, v);
|
||||
} else {
|
||||
foo();
|
||||
}
|
||||
|
||||
if !m.contains_key(&k) {
|
||||
foo();
|
||||
} else {
|
||||
m.insert(k, v);
|
||||
}
|
||||
|
||||
if !m.contains_key(&k) {
|
||||
m.insert(k, v);
|
||||
} else {
|
||||
m.insert(k, v2);
|
||||
}
|
||||
|
||||
if m.contains_key(&k) {
|
||||
if true { m.insert(k, v) } else { m.insert(k, v2) }
|
||||
} else {
|
||||
m.insert(k, v)
|
||||
};
|
||||
|
||||
if m.contains_key(&k) {
|
||||
foo();
|
||||
m.insert(k, v)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
}
|
||||
|
||||
fn main() {}
|
||||
142
tests/ui/entry_with_else.stderr
Normal file
142
tests/ui/entry_with_else.stderr
Normal file
|
|
@ -0,0 +1,142 @@
|
|||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:16:5
|
||||
|
|
||||
LL | / if !m.contains_key(&k) {
|
||||
LL | | m.insert(k, v);
|
||||
LL | | } else {
|
||||
LL | | m.insert(k, v2);
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
= note: `-D clippy::map-entry` implied by `-D warnings`
|
||||
help: try this
|
||||
|
|
||||
LL | match m.entry(k) {
|
||||
LL | std::collections::hash_map::Entry::Vacant(e) => {
|
||||
LL | e.insert(v);
|
||||
LL | }
|
||||
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
LL | e.insert(v2);
|
||||
...
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:22:5
|
||||
|
|
||||
LL | / if m.contains_key(&k) {
|
||||
LL | | m.insert(k, v);
|
||||
LL | | } else {
|
||||
LL | | m.insert(k, v2);
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | match m.entry(k) {
|
||||
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
LL | e.insert(v);
|
||||
LL | }
|
||||
LL | std::collections::hash_map::Entry::Vacant(e) => {
|
||||
LL | e.insert(v2);
|
||||
...
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:28:5
|
||||
|
|
||||
LL | / if !m.contains_key(&k) {
|
||||
LL | | m.insert(k, v);
|
||||
LL | | } else {
|
||||
LL | | foo();
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
|
||||
LL | e.insert(v);
|
||||
LL | } else {
|
||||
LL | foo();
|
||||
LL | }
|
||||
|
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:34:5
|
||||
|
|
||||
LL | / if !m.contains_key(&k) {
|
||||
LL | | foo();
|
||||
LL | | } else {
|
||||
LL | | m.insert(k, v);
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
|
||||
LL | e.insert(v);
|
||||
LL | } else {
|
||||
LL | foo();
|
||||
LL | }
|
||||
|
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:40:5
|
||||
|
|
||||
LL | / if !m.contains_key(&k) {
|
||||
LL | | m.insert(k, v);
|
||||
LL | | } else {
|
||||
LL | | m.insert(k, v2);
|
||||
LL | | }
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | match m.entry(k) {
|
||||
LL | std::collections::hash_map::Entry::Vacant(e) => {
|
||||
LL | e.insert(v);
|
||||
LL | }
|
||||
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
LL | e.insert(v2);
|
||||
...
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:46:5
|
||||
|
|
||||
LL | / if m.contains_key(&k) {
|
||||
LL | | if true { m.insert(k, v) } else { m.insert(k, v2) }
|
||||
LL | | } else {
|
||||
LL | | m.insert(k, v)
|
||||
LL | | };
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | match m.entry(k) {
|
||||
LL | std::collections::hash_map::Entry::Occupied(mut e) => {
|
||||
LL | if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
|
||||
LL | }
|
||||
LL | std::collections::hash_map::Entry::Vacant(e) => {
|
||||
LL | e.insert(v);
|
||||
...
|
||||
|
||||
error: usage of `contains_key` followed by `insert` on a `HashMap`
|
||||
--> $DIR/entry_with_else.rs:52:5
|
||||
|
|
||||
LL | / if m.contains_key(&k) {
|
||||
LL | | foo();
|
||||
LL | | m.insert(k, v)
|
||||
LL | | } else {
|
||||
LL | | None
|
||||
LL | | };
|
||||
| |_____^
|
||||
|
|
||||
help: try this
|
||||
|
|
||||
LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
|
||||
LL | foo();
|
||||
LL | Some(e.insert(v))
|
||||
LL | } else {
|
||||
LL | None
|
||||
LL | };
|
||||
|
|
||||
|
||||
error: aborting due to 7 previous errors
|
||||
|
||||
Loading…
Reference in a new issue