Added RefCell methods update and update_in_place

[peterjoel]

These two small methods help you protect yourself from accidentally mutably borrowing from a RefCell twice. This can happen easily when you need to update the data in a RefCell, when the new value depends on the old data.

The two variants are for ergonomic reasons, as you may have a method already of one form or the other. Or mutating the data in place may be more efficient sometimes.

Example usage with methods:

struct S {
   s: u32
}

impl S {
    fn new(n: u32) -> S {
        S { s: n }
    }
    fn incremented(&self) -> S {
        S { s: self.s + 1 }
    }
    fn increment(&mut self) {
        self.s += 1;
    }
}


fn main() {
    let rc = RefCell::new(S::new(0));
    
    println!("{:?}", rc.borrow().s); // 0
    
    rc.update(S::incremented);
    rc.update_in_place(S::increment);
    
    println!("{:?}", rc.borrow().s); // 2
}

As a slightly contrived example of a problem case, the following code panics because the first borrow is still alive when the next one is requested:

use std::cell::RefCell;

struct C {
    initial_val: u32,
    val: RefCell<u32>
}

impl C {
    fn new(n: u32) -> C {
        C { val: RefCell::new(n), initial_val: n }
    }
    
    fn reset(&self){
        let mut v = self.val.borrow_mut();
        *v = self.initial_val;
    }
    
    fn tick(&self){
        let mut v = self.val.borrow_mut();
        if *v > 0 {
            *v -= 1;
        }
        
        if *v == 0 {
            self.reset(); // problem: reset() also mutably borrows self.val
        }
    }
}

fn main() {
    let n = 3u32;
    let c = C::new(n);
    c.tick(); // panics if n < 2
}

This bug can be fixed by putting the borrow in its own block, so that it's dropped before it's needed again. But that not always obvious and might not be fixed if this only happens in an edge case or triggered by an untested error condition.

Using update_in_place, there is no possibility to make this mistake:

    fn tick(&self){
        self.val.update_in_place(|v| if *v > 0 {
            *v -= 1;
        });
        if *self.val.borrow() == 0 {
            self.reset();
        }
    }

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR @peterjoel! I'll cc @rust-lang/libs here as well in case others would like to weigh in.

Personally, though, I feel that these APIs don't belong on RefCell itself. The cell types benefit from a minimalistic API to be quite clear about their API usage, and I've always felt that trying to extend this with more helper methods and such waters down the original API. There's really no end to possible helper functions and such we could add.

It's true, though, that borrow scopes can often be a problem. Do explicit scopes not work out well in your use case? Do you find that they lose out in ergonomics?

[peterjoel]

It's partly ergonomics. In the example above, the code is shorter and (I think) easier to read. It also doesn't introduce a new binding into the current block scope.

I've been stung a few times where I've written code that works, and the borrow is in a small scope, but a future change grows the scope. The borrow and drop move further away from each other, which can be dangerous, especially when other methods could also borrow the same data. Expressing an update in a closure makes it inconvenient to let the block grow large, reducing the chance of this type of bug.

Obviously it's not foolproof - borrowing the value again from inside the closure will still cause a panic. But I feel like it's easier to spot and less likely to happen.

[aturon]

@alexcrichton @peterjoel

I have pretty mixed feelings about this PR. I understand the motivation, but I worry about a couple things:

• For simple cases, working with borrow_mut directly is more ergonomic.
• But for more complex cases, you "should" use these methods to be safe.

In other words, it seems like a hard line to draw for API recommendation. I think this is another way of making @alexcrichton's point about the current minimalistic API for RefCell, which gives you a lot of clarity about how to use it.

I also feel like, if you are worried enough about the issue to use these methods, you could just as well use explicit blocks today and get basically the same benefits.

Finally, while RefCell borrow errors are not caught at compile time, in my experience they almost always show up with even minimal testing.

On the whole, I think I'm personally not inclined to take this PR at this time.

[peterjoel]

In other words, it seems like a hard line to draw for API recommendation.

The line is probably that you should use update when the new value needs to depend on the old value. Otherwise, you can can safely do the assignment in one line, without introducing any new bindings that aren't immediately dropped:

*ref_cell.borrow_mut() = new_value;

[peterjoel]

@aturon @alexcrichton I think I'm happy with your comment to not merge this, and to keep it in a 3rd party crate. The methods can be added to RefCell via traits (which is how I'd previously used them), and made available in a "safer" wrapper object, which only exposes these and similar methods.

[alexcrichton]

Ok in that case I'm going to close this, but thanks regardless for the PR @peterjoel!