Skip to content

map_entry warning for HashMap suggests code that does not compile #14224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
connortsui20 opened this issue Feb 15, 2025 · 2 comments
Closed

map_entry warning for HashMap suggests code that does not compile #14224

connortsui20 opened this issue Feb 15, 2025 · 2 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@connortsui20
Copy link

Summary

The map_entry clippy warning can suggest code that does not compile. See below.

Lint Name

map_entry

Reproducer

If clippy sees an contains_key followed by an insert on HashMap, it will automatically (and correctly) suggest using entry() instead. However, if doesn't seem to take into account all branches. and can suggest code that does not compile.

pub fn insert_42_into_multimap(map: &mut HashMap<String, Vec<i32>>, key: String) {
    if !map.contains_key(&key) {
        map.get_mut(&key).unwrap().push(42);
    } else {
        map.insert(key, vec![42]);
    }
}

Note that this code is definitely not idiomatic, but the reason I know about this is that a student of mine (new to Rust) wrote something similar to the above code and got something similar to the following error message:

warning: usage of `contains_key` followed by `insert` on a `HashMap`
 --> src/lib.rs:4:5
  |
4 | /     if !map.contains_key(&key) {
5 | |         map.get_mut(&key).unwrap().push(42);
6 | |     } else {
7 | |         map.insert(key, vec![42]);
8 | |     }
  | |_____^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
  = note: `#[warn(clippy::map_entry)]` on by default
help: try
  |
4 ~     if let std::collections::hash_map::Entry::Occupied(mut e) = map.entry(key) {
5 +         e.insert(vec![42]);
6 +     } else {
7 +         map.get_mut(&key).unwrap().push(42);
8 +     }
  |

This suggestion is incorrect because key is moved and the other branch tries to borrow &key, thus this function will not compile.

I think the issue might be that if the insert following the contains_key is in a specific branch, clippy does not consider the other branches.

I'm wondering if it is possible to make clippy smart enough to suggest something like this (which would be the "idiomatic" way to write this code):

pub fn insert_42_into_multimap(map: &mut HashMap<String, Vec<i32>>, key: String) {
    map.entry(key).or_default().push(42);
}

I'm not super familiar with the internals of clippy but I'm assuming that this might be pretty hard. So maybe instead just make sure that it doesn't suggest code that is never going to compile in this case? Honestly not too sure what the solution should be here.

However, I would still treat this seriously because the entry API is not the most immediately intuitive to new Rust programmers, and I think clippy should do its best to not suggest things that will give new Rustaceans more headaches than they probably already have (though of course I understand this is super hard).

Version

rustc 1.84.1 (e71f9a9a9 2025-01-27)
binary: rustc
commit-hash: e71f9a9a98b0faf423844bf0ba7438f29dc27d58
commit-date: 2025-01-27
host: x86_64-pc-windows-msvc
release: 1.84.1
LLVM version: 19.1.5

(This isn't platform specific, I ran the above code on the playground + on Linux)

Additional Labels

No response

@connortsui20 connortsui20 added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 15, 2025
@connortsui20
Copy link
Author

Whoops this is a duplicate of #9305, #9470, #9925, #11976, #13306, and #13934

@jespiron
Copy link

Alas, like Napoleon, one does not simply invade rust-lang in the winter
but like Napoleon, better battles will be won

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

2 participants