## Objective
Sometimes we can do a hash lookup twice, which is slow and unidiomatic.
We should use the Entry API instead.
## Solution
Add another `dylint` lint called `map_lookup_then_insert` that catches
those cases.
This works with HashMap, BTreeMap, HashSet, BTreeSet and
entry-compatible ADTs.
## What it catches
See the examples in the `tooling/lints/ui/map_lookup_then_insert.rs`.
It's mostly things of the form
```rust
if let Some(value) = map.get(&key) {
*value
} else {
map.insert(key, 1);
1
}
```
which should be
```rust
*map.entry(key).or_insert(1)
```
The two key expressions are matched up to borrows/derefs,
value-preserving
conversions (`clone`, `to_owned`, `to_string` on string types, `to_vec`
on
slices, `String::from` on `&str` — each resolved by definition, not by
name),
and immutable local aliases. So looking up `&key` and inserting
`key.clone()`
is caught too. When a conversion had to be peeled, the diagnostic
carries a
note that an entry-API rewrite would perform that copy on hits as well
as
misses.
## What it deliberately does not catch
Forms Clippy already catches.
## Findings
A `--workspace` run finds 31 warnings at 30 unique sites in `zed`:
the 16 the plain key matching finds, plus 15 more that require the
conversion/alias matching (e.g. `tiles_by_key.get(key)` followed by
`tiles_by_key.insert(key.clone(), tile)` in `metal_atlas.rs`).
Will post those fixes in a follow up PR.
## Testing
UI tests cover every `Entry` method positively and negatively, BTreeMap
mirrors, set cases, and the Clippy-deferral boundary. The
key-normalization logic is covered by nine positive cases (cloned keys,
UFCS forms, `to_owned`/`to_string`/`to_vec`/`String::from`, deref
layers, local aliases, sets) and six negative cases (inherent methods
merely named `clone`/`to_owned`, mutated aliases, a clone of a different
key, `to_string` on a non-string key, and side-effecting initializers).
## Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content adheres to Zed's UI standards
([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
and
[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)
guidelines)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
Release Notes:
- N/A
360 lines
13 KiB
Plaintext
360 lines
13 KiB
Plaintext
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:12:5
|
|
|
|
|
LL | / if let Some(value) = map.get(&key) {
|
|
LL | | *value
|
|
LL | | } else {
|
|
LL | | map.insert(key, 1);
|
|
LL | | 1
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: `-D map-lookup-then-insert` implied by `-D warnings`
|
|
= help: to override `-D warnings` add `#[allow(map_lookup_then_insert)]`
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:21:5
|
|
|
|
|
LL | / match map.get(&key).copied() {
|
|
LL | | Some(value) => value,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
... |
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:32:5
|
|
|
|
|
LL | / match map.get_mut(&key) {
|
|
LL | | Some(value) => *value += 1,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:41:5
|
|
|
|
|
LL | / match map.get_key_value(&key) {
|
|
LL | | Some((_, value)) => *value,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
... |
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:51:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(value) => *value,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
... |
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:61:5
|
|
|
|
|
LL | / if let Some(value) = map.get(&key) {
|
|
LL | | *value
|
|
LL | | } else {
|
|
LL | | map.insert(key, 1);
|
|
LL | | 1
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:71:5
|
|
|
|
|
LL | / match map.get_mut(&key) {
|
|
LL | | Some(value) => *value += 1,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:80:5
|
|
|
|
|
LL | / match map.get_key_value(&key) {
|
|
LL | | Some((_, value)) => *value,
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
... |
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:90:5
|
|
|
|
|
LL | / let Some(value) = map.get(&key) else {
|
|
LL | | map.insert(key, 1);
|
|
LL | | return 1;
|
|
LL | | };
|
|
| |______^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:100:5
|
|
|
|
|
LL | / if let Some(_current) = map.get(&key) {
|
|
LL | | map.insert(key, 2);
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this set lookup and insertion search for the same value twice
|
|
--> $DIR/map_lookup_then_insert.rs:108:5
|
|
|
|
|
LL | / if let Some(existing) = set.get(&value) {
|
|
LL | | existing.is_empty()
|
|
LL | | } else {
|
|
LL | | set.insert(value)
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present
|
|
|
|
error: this set lookup and insertion search for the same value twice
|
|
--> $DIR/map_lookup_then_insert.rs:116:5
|
|
|
|
|
LL | / match set.get(&value) {
|
|
LL | | Some(_) => false,
|
|
LL | | None => set.insert(value),
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:123:5
|
|
|
|
|
LL | / let Some(value) = map.get(&key) else {
|
|
LL | | map.insert(key, 1);
|
|
LL | | return 1;
|
|
LL | | };
|
|
| |______^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:136:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key, expensive());
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:146:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | let value = key.len();
|
|
... |
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:157:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key, usize::default());
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:167:5
|
|
|
|
|
LL | / if let Some(_current) = map.get(&key) {
|
|
LL | | map.insert(key, 2);
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:180:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key, 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:207:5
|
|
|
|
|
LL | / if map.get(&key).is_none() {
|
|
LL | | map.insert(key, 1);
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:217:5
|
|
|
|
|
LL | / if let Some(values) = map.get_mut(&key) {
|
|
LL | | values.push(value);
|
|
LL | | } else {
|
|
LL | | map.insert(key.clone(), vec![value]);
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:225:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(Clone::clone(&key), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:234:5
|
|
|
|
|
LL | / match map.get(key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key.to_owned(), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:243:5
|
|
|
|
|
LL | / match map.get(key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key.to_string(), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:252:5
|
|
|
|
|
LL | / match map.get(key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(String::from(key), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:261:5
|
|
|
|
|
LL | / match map.get(key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(key.to_vec(), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:271:5
|
|
|
|
|
LL | / match map.get(key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert((*key).clone(), 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this map lookup and insertion search for the same key twice
|
|
--> $DIR/map_lookup_then_insert.rs:282:5
|
|
|
|
|
LL | / match map.get(&key) {
|
|
LL | | Some(_) => {}
|
|
LL | | None => {
|
|
LL | | map.insert(owned, 1);
|
|
LL | | }
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: to search the map only once, use the entry API: https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: this set lookup and insertion search for the same value twice
|
|
--> $DIR/map_lookup_then_insert.rs:291:5
|
|
|
|
|
LL | / if let Some(existing) = set.get(&value) {
|
|
LL | | existing.is_empty()
|
|
LL | | } else {
|
|
LL | | set.insert(value.clone())
|
|
LL | | }
|
|
| |_____^
|
|
|
|
|
= help: `insert` alone searches once, skips duplicates, and returns `false` when the value was already present
|
|
= note: the inserted key is an owned copy of the looked-up key; a rewrite that passes the key by value up front would make that copy on hits as well as misses
|
|
|
|
error: aborting due to 28 previous errors
|
|
|