Auto merge of #6937 - Jarcho:map_entry_suggestion, r=giraffate

Improve `map_entry` suggestion

fixes: #5176
fixes: #4674
fixes: #4664
fixes: #1450

Still need to handle the value returned by `insert` correctly.

changelog: Improve `map_entry` suggestion. Will now suggest `or_insert`, `insert_with` or `match _.entry(_)` as appopriate.
changelog: Fix `map_entry` false positives where the entry api can't be used. e.g. when the map is used for multiple things.
This commit is contained in:
bors 2021-04-16 13:23:23 +00:00
commit 1e0a3ff55c
19 changed files with 1531 additions and 384 deletions

155
tests/ui/entry.fixed Normal file
View file

@ -0,0 +1,155 @@
// run-rustfix
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
#![warn(clippy::map_entry)]
#![feature(asm)]
use std::collections::{BTreeMap, HashMap};
use std::hash::Hash;
macro_rules! m {
($e:expr) => {{ $e }};
}
macro_rules! insert {
($map:expr, $key:expr, $val:expr) => {
$map.insert($key, $val)
};
}
fn foo() {}
fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, k: K, k2: K, v: V, v2: V) {
// or_insert(v)
m.entry(k).or_insert(v);
// semicolon on insert, use or_insert_with(..)
m.entry(k).or_insert_with(|| {
if true {
v
} else {
v2
}
});
// semicolon on if, use or_insert_with(..)
m.entry(k).or_insert_with(|| {
if true {
v
} else {
v2
}
});
// early return, use if let
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
if true {
e.insert(v);
} else {
e.insert(v2);
return;
}
}
// use or_insert_with(..)
m.entry(k).or_insert_with(|| {
foo();
v
});
// semicolon on insert and match, use or_insert_with(..)
m.entry(k).or_insert_with(|| {
match 0 {
1 if true => {
v
},
_ => {
v2
},
}
});
// one branch doesn't insert, use if let
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
match 0 {
0 => foo(),
_ => {
e.insert(v2);
},
};
}
// use or_insert_with
m.entry(k).or_insert_with(|| {
foo();
match 0 {
0 if false => {
v
},
1 => {
foo();
v
},
2 | 3 => {
for _ in 0..2 {
foo();
}
if true {
v
} else {
v2
}
},
_ => {
v2
},
}
});
// ok, insert in loop
if !m.contains_key(&k) {
for _ in 0..2 {
m.insert(k, v);
}
}
// macro_expansion test, use or_insert(..)
m.entry(m!(k)).or_insert_with(|| m!(v));
// ok, map used before insertion
if !m.contains_key(&k) {
let _ = m.len();
m.insert(k, v);
}
// ok, inline asm
if !m.contains_key(&k) {
unsafe { asm!("nop") }
m.insert(k, v);
}
// ok, different keys.
if !m.contains_key(&k) {
m.insert(k2, v);
}
// ok, different maps
if !m.contains_key(&k) {
m2.insert(k, v);
}
// ok, insert in macro
if !m.contains_key(&k) {
insert!(m, k, v);
}
}
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
// insert then do something, use if let
if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
e.insert(v);
foo();
}
}
fn main() {}

159
tests/ui/entry.rs Normal file
View file

@ -0,0 +1,159 @@
// run-rustfix
#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
#![warn(clippy::map_entry)]
#![feature(asm)]
use std::collections::{BTreeMap, HashMap};
use std::hash::Hash;
macro_rules! m {
($e:expr) => {{ $e }};
}
macro_rules! insert {
($map:expr, $key:expr, $val:expr) => {
$map.insert($key, $val)
};
}
fn foo() {}
fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, k: K, k2: K, v: V, v2: V) {
// or_insert(v)
if !m.contains_key(&k) {
m.insert(k, v);
}
// semicolon on insert, use or_insert_with(..)
if !m.contains_key(&k) {
if true {
m.insert(k, v);
} else {
m.insert(k, v2);
}
}
// semicolon on if, use or_insert_with(..)
if !m.contains_key(&k) {
if true {
m.insert(k, v)
} else {
m.insert(k, v2)
};
}
// early return, use if let
if !m.contains_key(&k) {
if true {
m.insert(k, v);
} else {
m.insert(k, v2);
return;
}
}
// use or_insert_with(..)
if !m.contains_key(&k) {
foo();
m.insert(k, v);
}
// semicolon on insert and match, use or_insert_with(..)
if !m.contains_key(&k) {
match 0 {
1 if true => {
m.insert(k, v);
},
_ => {
m.insert(k, v2);
},
};
}
// one branch doesn't insert, use if let
if !m.contains_key(&k) {
match 0 {
0 => foo(),
_ => {
m.insert(k, v2);
},
};
}
// use or_insert_with
if !m.contains_key(&k) {
foo();
match 0 {
0 if false => {
m.insert(k, v);
},
1 => {
foo();
m.insert(k, v);
},
2 | 3 => {
for _ in 0..2 {
foo();
}
if true {
m.insert(k, v);
} else {
m.insert(k, v2);
};
},
_ => {
m.insert(k, v2);
},
}
}
// ok, insert in loop
if !m.contains_key(&k) {
for _ in 0..2 {
m.insert(k, v);
}
}
// macro_expansion test, use or_insert(..)
if !m.contains_key(&m!(k)) {
m.insert(m!(k), m!(v));
}
// ok, map used before insertion
if !m.contains_key(&k) {
let _ = m.len();
m.insert(k, v);
}
// ok, inline asm
if !m.contains_key(&k) {
unsafe { asm!("nop") }
m.insert(k, v);
}
// ok, different keys.
if !m.contains_key(&k) {
m.insert(k2, v);
}
// ok, different maps
if !m.contains_key(&k) {
m2.insert(k, v);
}
// ok, insert in macro
if !m.contains_key(&k) {
insert!(m, k, v);
}
}
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
// insert then do something, use if let
if !m.contains_key(&k) {
m.insert(k, v);
foo();
}
}
fn main() {}

186
tests/ui/entry.stderr Normal file
View file

@ -0,0 +1,186 @@
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:24:5
|
LL | / if !m.contains_key(&k) {
LL | | m.insert(k, v);
LL | | }
| |_____^ help: try this: `m.entry(k).or_insert(v);`
|
= note: `-D clippy::map-entry` implied by `-D warnings`
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:29:5
|
LL | / if !m.contains_key(&k) {
LL | | if true {
LL | | m.insert(k, v);
LL | | } else {
LL | | m.insert(k, v2);
LL | | }
LL | | }
| |_____^
|
help: try this
|
LL | m.entry(k).or_insert_with(|| {
LL | if true {
LL | v
LL | } else {
LL | v2
LL | }
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:38:5
|
LL | / if !m.contains_key(&k) {
LL | | if true {
LL | | m.insert(k, v)
LL | | } else {
LL | | m.insert(k, v2)
LL | | };
LL | | }
| |_____^
|
help: try this
|
LL | m.entry(k).or_insert_with(|| {
LL | if true {
LL | v
LL | } else {
LL | v2
LL | }
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:47:5
|
LL | / if !m.contains_key(&k) {
LL | | if true {
LL | | m.insert(k, v);
LL | | } else {
... |
LL | | }
LL | | }
| |_____^
|
help: try this
|
LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
LL | if true {
LL | e.insert(v);
LL | } else {
LL | e.insert(v2);
LL | return;
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:57:5
|
LL | / if !m.contains_key(&k) {
LL | | foo();
LL | | m.insert(k, v);
LL | | }
| |_____^
|
help: try this
|
LL | m.entry(k).or_insert_with(|| {
LL | foo();
LL | v
LL | });
|
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:63:5
|
LL | / if !m.contains_key(&k) {
LL | | match 0 {
LL | | 1 if true => {
LL | | m.insert(k, v);
... |
LL | | };
LL | | }
| |_____^
|
help: try this
|
LL | m.entry(k).or_insert_with(|| {
LL | match 0 {
LL | 1 if true => {
LL | v
LL | },
LL | _ => {
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:75:5
|
LL | / if !m.contains_key(&k) {
LL | | match 0 {
LL | | 0 => foo(),
LL | | _ => {
... |
LL | | };
LL | | }
| |_____^
|
help: try this
|
LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
LL | match 0 {
LL | 0 => foo(),
LL | _ => {
LL | e.insert(v2);
LL | },
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:85:5
|
LL | / if !m.contains_key(&k) {
LL | | foo();
LL | | match 0 {
LL | | 0 if false => {
... |
LL | | }
LL | | }
| |_____^
|
help: try this
|
LL | m.entry(k).or_insert_with(|| {
LL | foo();
LL | match 0 {
LL | 0 if false => {
LL | v
LL | },
...
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry.rs:119:5
|
LL | / if !m.contains_key(&m!(k)) {
LL | | m.insert(m!(k), m!(v));
LL | | }
| |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));`
error: usage of `contains_key` followed by `insert` on a `BTreeMap`
--> $DIR/entry.rs:153:5
|
LL | / if !m.contains_key(&k) {
LL | | m.insert(k, v);
LL | | foo();
LL | | }
| |_____^
|
help: try this
|
LL | if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
LL | e.insert(v);
LL | foo();
LL | }
|
error: aborting due to 10 previous errors

View file

@ -1,15 +0,0 @@
// run-rustfix
#![allow(unused, clippy::needless_pass_by_value)]
#![warn(clippy::map_entry)]
use std::collections::{BTreeMap, HashMap};
use std::hash::Hash;
fn foo() {}
fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
m.entry(k).or_insert(v);
}
fn main() {}

View file

@ -1,17 +0,0 @@
// run-rustfix
#![allow(unused, clippy::needless_pass_by_value)]
#![warn(clippy::map_entry)]
use std::collections::{BTreeMap, HashMap};
use std::hash::Hash;
fn foo() {}
fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
m.insert(k, v);
}
}
fn main() {}

View file

@ -1,12 +0,0 @@
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry_fixable.rs:12:5
|
LL | / if !m.contains_key(&k) {
LL | | m.insert(k, v);
LL | | }
| |_____^ help: consider using: `m.entry(k).or_insert(v);`
|
= note: `-D clippy::map-entry` implied by `-D warnings`
error: aborting due to previous error

View file

@ -1,73 +0,0 @@
#![allow(unused, clippy::needless_pass_by_value)]
#![warn(clippy::map_entry)]
use std::collections::{BTreeMap, HashMap};
use std::hash::Hash;
fn foo() {}
fn insert_if_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
m.insert(k, v)
} else {
None
};
}
fn insert_if_present2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if m.contains_key(&k) {
None
} else {
m.insert(k, v)
};
}
fn insert_if_absent3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
foo();
m.insert(k, v)
} else {
None
};
}
fn insert_if_present3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if m.contains_key(&k) {
None
} else {
foo();
m.insert(k, v)
};
}
fn insert_in_btreemap<K: Ord, V>(m: &mut BTreeMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
foo();
m.insert(k, v)
} else {
None
};
}
// should not trigger
fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) {
if !m.contains_key(&k) {
m.insert(o, v);
}
}
// should not trigger, because the one uses different HashMap from another one
fn insert_from_different_map<K: Eq + Hash, V>(m: HashMap<K, V>, n: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
n.insert(k, v);
}
}
// should not trigger, because the one uses different HashMap from another one
fn insert_from_different_map2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, n: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) {
n.insert(k, v);
}
}
fn main() {}

View file

@ -1,57 +0,0 @@
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry_unfixable.rs:10:5
|
LL | / if !m.contains_key(&k) {
LL | | m.insert(k, v)
LL | | } else {
LL | | None
LL | | };
| |_____^ consider using `m.entry(k)`
|
= note: `-D clippy::map-entry` implied by `-D warnings`
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry_unfixable.rs:18:5
|
LL | / if m.contains_key(&k) {
LL | | None
LL | | } else {
LL | | m.insert(k, v)
LL | | };
| |_____^ consider using `m.entry(k)`
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry_unfixable.rs:26:5
|
LL | / if !m.contains_key(&k) {
LL | | foo();
LL | | m.insert(k, v)
LL | | } else {
LL | | None
LL | | };
| |_____^ consider using `m.entry(k)`
error: usage of `contains_key` followed by `insert` on a `HashMap`
--> $DIR/entry_unfixable.rs:35:5
|
LL | / if m.contains_key(&k) {
LL | | None
LL | | } else {
LL | | foo();
LL | | m.insert(k, v)
LL | | };
| |_____^ consider using `m.entry(k)`
error: usage of `contains_key` followed by `insert` on a `BTreeMap`
--> $DIR/entry_unfixable.rs:44:5
|
LL | / if !m.contains_key(&k) {
LL | | foo();
LL | | m.insert(k, v)
LL | | } else {
LL | | None
LL | | };
| |_____^ consider using `m.entry(k)`
error: aborting due to 5 previous errors

View 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() {}

View 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() {}

View 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

View file

@ -22,7 +22,7 @@ fn str_lit_as_bytes() {
let current_version = env!("CARGO_PKG_VERSION").as_bytes();
let includestr = include_bytes!("entry_unfixable.rs");
let includestr = include_bytes!("string_lit_as_bytes.rs");
let _ = b"string with newline\t\n";
}

View file

@ -22,7 +22,7 @@ fn str_lit_as_bytes() {
let current_version = env!("CARGO_PKG_VERSION").as_bytes();
let includestr = include_str!("entry_unfixable.rs").as_bytes();
let includestr = include_str!("string_lit_as_bytes.rs").as_bytes();
let _ = "string with newline\t\n".as_bytes();
}

View file

@ -27,8 +27,8 @@ LL | let bs = "lit to owned".to_owned().into_bytes();
error: calling `as_bytes()` on `include_str!(..)`
--> $DIR/string_lit_as_bytes.rs:25:22
|
LL | let includestr = include_str!("entry_unfixable.rs").as_bytes();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("entry_unfixable.rs")`
LL | let includestr = include_str!("string_lit_as_bytes.rs").as_bytes();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("string_lit_as_bytes.rs")`
error: calling `as_bytes()` on a string literal
--> $DIR/string_lit_as_bytes.rs:27:13