Skip to content

map_entry, suggestion produces invalid borrow inside closure #11976

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
Lunderberg opened this issue Dec 16, 2023 · 3 comments · Fixed by #14307
Closed

map_entry, suggestion produces invalid borrow inside closure #11976

Lunderberg opened this issue Dec 16, 2023 · 3 comments · Fixed by #14307
Assignees
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

@Lunderberg
Copy link

Lunderberg commented Dec 16, 2023

Summary

The map_entry lint is suppressed if the hash map is accessed between the call to HashMap::contains_key and the later call to HashMap::insert. However, it does not check for access of the hash map occurring within a closure. As a result, the suggestion's use of HashMap::entry starts a mutable borrow of the entire hashmap, conflicting with call to HashMap::get that occurred between the contains_key and insert.

In the example below, if the line let _ = || hashmap.get(&0); is replaced with hashmap.get(&0);, the lint is no longer suggested.

Lint Name

map_entry

Reproducer

I tried this code:

fn main() {
    let mut hashmap = std::collections::HashMap::new();
    if !hashmap.contains_key(&0) {
        let _ = || hashmap.get(&0);
        hashmap.insert(0, 0);
    }
}

I saw this happen:

warning: usage of `contains_key` followed by `insert` on a `HashMap`
 --> src/main.rs:3:5
  |
3 | /     if !hashmap.contains_key(&0) {
4 | |         let _ = || hashmap.get(&0);
5 | |         hashmap.insert(0, 0);
6 | |     }
  | |_____^
  |
  = 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
  |
3 ~     hashmap.entry(0).or_insert_with(|| {
4 +         let _ = || hashmap.get(&0);
5 +         0
6 +     });
  |

warning: `temp` (bin "temp") generated 1 warning (run `cargo clippy --fix --bin "temp"` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

I expected to see this happen: No clippy suggestions produced.

Version

rustc 1.76.0-nightly (06e02d5b2 2023-12-09)
binary: rustc
commit-hash: 06e02d5b259c1e88cbf0c74366d9e0a4c7cfd6d9
commit-date: 2023-12-09
host: x86_64-unknown-linux-gnu
release: 1.76.0-nightly
LLVM version: 17.0.5

Additional Labels

No response

@Lunderberg Lunderberg 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 Dec 16, 2023
@dhilipsiva
Copy link

@dhilipsiva
Copy link

More context from the above PR:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
  --> packages/media_record/src/media.rs:69:34
   |
68 |                 if let std::collections::hash_map::Entry::Vacant(e) = self.tracks.entry(id) {
   |                                                                       ----------- first mutable borrow occurs here
69 |                     let writer = self.get_free_writer_for(event.ts, media.meta.is_audio());
   |                                  ^^^^ second mutable borrow occurs here
...
76 |                     e.insert(TrackWriter { writer });
   |                     - first borrow later used here

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0499`.
Original diagnostics will follow.

warning: usage of `contains_key` followed by `insert` on a `HashMap`
  --> packages/media_record/src/media.rs:68:17
   |
68 | /                 if !self.tracks.contains_key(&id) {
69 | |                     let writer = self.get_free_writer_for(event.ts, media.meta.is_audio());
70 | |                     if media.meta.is_audio() {
71 | |                         self.writers[writer].audio_inuse = true;
...  |
76 | |                     self.tracks.insert(id, TrackWriter { writer });
77 | |                 }
   | |_________________^
   |
   = 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
   |
68 ~                 if let std::collections::hash_map::Entry::Vacant(e) = self.tracks.entry(id) {
69 +                     let writer = self.get_free_writer_for(event.ts, media.meta.is_audio());
70 +                     if media.meta.is_audio() {
71 +                         self.writers[writer].audio_inuse = true;
72 +                     } else {
73 +                         self.writers[writer].video_inuse = true;
74 +                     }
75 +                     log::info!("write track {:?} to writer {writer}", id);
76 +                     e.insert(TrackWriter { writer });
77 +                 }
   |

warning: `media-server-record` (lib) generated 1 warning (run `cargo clippy --fix --lib -p media-server-record` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.92s

@profetia
Copy link
Contributor

@rustbot claim

github-merge-queue bot pushed a commit that referenced this issue Feb 27, 2025
Closes #11976

changelog: [`map_entry`]: fix FP inside closure
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

Successfully merging a pull request may close this issue.

3 participants