Skip to content
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

clippy::map_entry false positive with let-else #12489

Closed
0vercl0k opened this issue Mar 15, 2024 · 1 comment · Fixed by #12498
Closed

clippy::map_entry false positive with let-else #12489

0vercl0k opened this issue Mar 15, 2024 · 1 comment · Fixed by #12498
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

@0vercl0k
Copy link

Summary

The clippy::map_entry seems to have a false-positive when using let-else. I've looked for similar issues in the tracker but couldn't find any so opening this one - apologies if I missed one.

The lint suggests to use or_insert_with which makes the else part return out of the closure instead of out of the function. Because they expect different return types (the closure expects a String, the function expects an Option<&String> in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8b9c4d6dad5586827f5906400a480391), it leads to a compilation error.

Lint Name

clippy::map_entry

Reproducer

I tried this code (also available at https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8b9c4d6dad5586827f5906400a480391):

use std::collections::HashMap;

#[derive(Default)]
struct Repro {
    cache: HashMap<u64, String>,
}

impl Repro {
    fn calc(k: u64) -> Option<String> {
        if k == 1337 {
            Some("hello!".to_string())
        } else {
            None
        }
    }

    pub fn lookup(&mut self, k: u64) -> Option<&String> {
        if !self.cache.contains_key(&k) {
            let Some(foo) = Self::calc(k) else {
                return None;
            };

            self.cache.insert(k, foo);
        }

        self.cache.get(&k)
    }
}

fn main() {
    let mut repro = Repro::default();
    println!("{:?}", repro.lookup(1337));
    println!("{:?}", repro.lookup(1338));
}

I saw this happen:

warning: usage of `contains_key` followed by `insert` on a `HashMap`
  --> src/main.rs:18:9
   |
18 | /         if !self.cache.contains_key(&k) {
19 | |             let Some(foo) = Self::calc(k) else {
20 | |                 return None;
21 | |             };
22 | |
23 | |             self.cache.insert(k, foo);
24 | |         }
   | |_________^
   |
   = 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
   |
18 ~         self.cache.entry(k).or_insert_with(|| {
19 +             let Some(foo) = Self::calc(k) else {
20 +                 return None;
21 +             };
22 + 
23 +             foo
24 +         });
   |

Version

rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-pc-windows-msvc
release: 1.76.0
LLVM version: 17.0.6

Additional Labels

No response

@0vercl0k 0vercl0k 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 Mar 15, 2024
@bors bors closed this as completed in b5dcaae Mar 18, 2024
@0vercl0k
Copy link
Author

Thank you @y21 for fixing this 🙏

Cheers

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.

1 participant