clippy suggests map.entry().or_insert() when it shouldn't

[tspiteri]

use std::collections::HashMap;
fn expensive_can_fail(v: u32) -> Result<u32, ()> {
    if v == 0 { Err(()) } else { Ok(v) }
}
fn check(map: &mut HashMap<u32, u32>, v: u32) -> Result<(), ()> {
    if !map.contains_key(&v) {
        map.insert(v, expensive_can_fail(v)?);
    }
    Ok(())
}
fn main() {
    let mut map = HashMap::new();
    let _ = check(&mut map, 0);
}

In the above, clippy suggests map.entry(v).or_insert(expensive_can_fail(v)?), but that calls the expensive function even if it is not needed.

Normally you could use map.entry(v).or_insert_with(/*...*/) to call the function only when needed, but in this case, since expensive_can_fail(v)? can return, that doesn't work either. Maybe in this case clippy can suggest

    if let Entry::Vacant(o) = map.entry(v) {
        o.insert(expensive_can_fail(v)?);
    }

which avoids calling the expensive function needlessly and avoids the double key search.

[MichaelMauderer]

There is another case when this warning crops up when it should not: when the function that is called to produce the value that is inserted requires a borrow of an object owning the map.

For example:

fn fill_value(&mut self, key: u32) {
        if !self.map.contains_key(&key) {
            self.map
                .insert(key, self.make_something_expensive_with_self(key));
        }
    }

As far as I can see, this can't be rewritten with the entry API as the entry will keep a mutable reference to self, prohibiting the second borrow to self required for the function cal to make_something_expensive_with_self.

[kayleg]

I think there is a false positive with 2 different maps triggering this too:

fn map_issue() {
    let mut m1: HashMap<u32, bool> = HashMap::new();
    let mut m2 = HashMap::new();
    m2.insert(1, true);

    if m1.contains_key(&1) {
        m2.insert(1, false);
        m1.remove(&1);
    } else {
        let might_be_falsy = m2.remove(&1).unwrap();
        if !might_be_falsy {
            m2.insert(1, true);
            println!("It was falsy");
        } else {
            m2.insert(1, might_be_falsy);
        }
    }
}

Results in: consider using: m1.entry(1)

If you comment out the m2.insert lines in the contains_key else block it goes away, but the contains check is on m1.