Issue with hashmap and fallible update

[robin]

Sorry for the title, I don't know how to properly sum-up my issue in a clear sentence.

I am using a HashMap as a cache. I want to be able to create the value if the key isn't already present in the HashMap. The following code works.

use std::collections::HashMap;
type ExpensiveData = f32;

struct Cache {
    cache: HashMap<u32, ExpensiveData>
}
impl Cache {
    fn get_value(&mut self, key: u32) -> &ExpensiveData {
        cache.entry(3).or_insert_with(|| 3.0);
    }
}

But unfortunately the constructor of ExpensiveData is fallible (the data is loaded from a file). I can't use the try operator inside the lambda (.or_insert_with(|| faillible_operation()?) isn't valid for obvious reason).
It feels that my only solution is to do two look-up each time I want to access my data to solve this issue.

fn faillible_operation(key: u32) -> std::Result<ExpensiveData, SomeError> {
    if rand() % 2 {
        Err(SomeError)
    } else {
        Ok(key as ExpensiveData)
    }
}

impl Cache {
    fn get_value(&mut self, key: u32) -> std::Result<&ExpensiveData, SomeError> {
        if cache.contains_key(&key) {
            cache.get(&key).unwrap() // unrelated note: I don't understand why `cache[&3]` returns a value and not a reference
        } else {
            let expensive_data: ExpensiveData = faillible_operation(key)?; // I can use the try operator here
            cache.entry(key).or_insert(expensive_data)
        }
    }
}

[cuviper]

You can match the entry yourself and do your computation, something like:

fn get_value(&mut self, key: u32) -> std::Result<&ExpensiveData, SomeError> {
    match cache.get(key) {
        Occupied(e) => Ok(e.get()),
        Vacant(e) => {
            let expensive_data: ExpensiveData = faillible_operation(key)?;
            Ok(e.insert(expensive_data))
        }
    }
}

[Heliozoa]

Just a small note, I was trying this out myself and this'll work but the Occupied branch needs to return e.into_mut() instead of e.get(), because get borrows from e which is owned by the function.

[cuviper]

Good catch -- clearly I didn't actually try it.

[robin]

impl Cache {
    pub fn get_value(&mut self, key: u32) -> Result<&ExpensiveData, SomeError> {
        match self.cache.entry(key) {
            Entry::Occupied(e) => Ok(e.into_mut()),
            Entry::Vacant(e) => {
                let expensive_data: ExpensiveData = faillible_operation(key)?;
                Ok(e.insert(expensive_data))
            }
        }
    }
}

I am still not fully convinced. It's much better than my initial version, but still really verbose and complex.

playground

[cuviper]

It's essentially the same code as the implementation of or_insert_with, just with added result handling.

[robin]

It adds 5 lines of pure boilerplate for a 3 lines function.

[cuviper]

I guess it's a matter of opinion, but I don't see this as boilerplate. Entry has some nice methods for convenience, but it makes sense to require more work if you stray from those boundaries, like wanting to deal with your fallible operation.

Maybe there could be a new Entry method dealing with Result in particular:

pub fn or_insert_with<F: FnOnce() -> V>(self, default: F) -> &'a mut V;

pub fn or_insert_with_result<F, E>(self, default: F) -> Result<&'a mut V, E>
where
    F: FnOnce() -> Result<V, E>;

But it would be more flexible for fallible operations to use Try, something like:

pub fn or_insert_with_try<F, E, R1, R2>(self, default: F) -> R2
where
    R1: Try<Ok = V, Error = E>,
    R2: Try<Ok = &'a mut V, Error = E>,
    F: FnOnce() -> R1;

I think this would be hard to actually use, because type inference can't tell that you want R1 and R2 to be the "same" Result type. They're not really the same since they have different type parameters, and I think we would need type constructors to make that really possible. So if type inference can't figure this out from surrounding context, you'll end up turbo-fishing these generic parameters.

I suspect the libs team would rather you just match the entry.

[mbrubeck]

The code can be slightly shortened, if length/complexity is the main concern:

fn get_value(&mut self, key: u32) -> Result<&ExpensiveData, SomeError> {
    Ok(match self.cache.entry(key) {
        Occupied(e) => e.into_mut(),
        Vacant(e) => e.insert(faillible_operation(key)?)
    })
}

[robin]

Indeed. I somehow didn't realised I could remove the temporary (that I introduced in an intemediate refactoring state).