Remove borrow in Ref getter

Is there a way to get around the extra .borrow() call here? Something kinda like the HashMap entry API?

I tried different variations, but kept coming back to either not being able to get a Ref from a RefMut or borrowing while the mutable borrow is still alive

struct Foo {
    pub ref_cell: RefCell<Option<String>>
}

impl Foo {
    fn get(&self) -> Ref<'_, String> {
        // if it's None, set it
        if self.ref_cell.borrow().is_none() {
            *self.ref_cell.borrow_mut() = Some(String::from("hello world"));
        }
        
        // either way, get it
        Ref::map(self.ref_cell.borrow(), |inner| inner.as_ref().unwrap())
    }
}

What's the exact problem? Your code compiled without change, as seen here:

Problem is there are two .borrow() calls in the case of the RefCell having a value already, and I would like to eliminate one

Got it. You can use your borrow_mut call for the conditional as well, but then you have to make sure to drop it before you take the immutable borrow.

I don't think a RefCell borrow costs much at all. Sure it's not free because it's checking the status, but unlike a HashMap's Entry that needs to perform a hash and search, RefCell's borrow is just checking a fixed structure.

2 Likes

you mean like this? It does help a bit, thanks, but there's still the problem of the extra borrow in case of Some

impl Foo {
    fn get(&self) -> Ref<'_, String> {
        // if it's None, set it
        {
            let mut lock = self.ref_cell.borrow_mut();
            
            if lock.as_ref().is_none() {
                *lock = Some(String::from("hello world"));
            }
        }
        
        // either way, get it
        Ref::map(self.ref_cell.borrow(), |inner| inner.as_ref().unwrap())
    }
}

Good point! Almost definitely a silly optimization... but still just kinda curious if it's possible

You can drop it either by creating a new scope that ends before the .borrow() call:

{
    let mut borrow = self.ref_cell.borrow_mut();
    if borrow.is_none() {
        *borrow = Some(String::from("hello world"));
    }
}
Ref::map(self.ref_cell.borrow(), |inner| inner.as_ref().unwrap())

Or by ending explicitly dropping the RefMut before taking a shared borrow.

let mut borrow = self.ref_cell.borrow_mut();
if borrow.is_none() {
    *borrow = Some(String::from("hello world"));
}
drop(borrow);
Ref::map(self.ref_cell.borrow(), |inner| inner.as_ref().unwrap())

The reason your current version works is that the immutable borrow is never assigned to a variable, so its lifetime ends as soon as the if condition is evaluated.

This can also be written as:

*lock = lock.take().or_else(|| Some(String::from("hello world")));
1 Like

getting into very micro optimization territory, but isn't that a bit more expensive, i.e. assigning its inner value to itself unnecessarily?

The optimizer shouldn't have any trouble figuring out what's going on here. If you have any doubts, you can check the generated assembly for the two version (using release mode) at the rust playground.

1 Like

You can also make the borrow_mut temporary to a single statement using one of the provided methods on Option:

self.ref_cell
    .borrow_mut()
    .get_or_insert_with(|| String::from("hello world"));
Ref::map(self.ref_cell.borrow(), |inner| inner.as_ref().unwrap())
1 Like

BTW, you're reinventing OnceCell, so the solution may be to use that instead.

5 Likes

In my use case, I need to also update the inner ref_cell sometimes.