Cell should provide a method to clone out the value

I known there is a reason only Copy is supported at the moment but I suppose the following code is valid:

let a = Rc::new(String::from("foo"));
let a = Cell::new(Rc::downgrade(&a));
let b = a.take();

a.set(b.clone());

Which effectively cloned out the value. If the above code is valid that mean Cell should be able to have the following method:

impl<T: Clone> {
    pub fn clone_value(&self) -> T;
}

What if the cell itself is set from inside the clone impl of the cell's content? You did get a exclusive and shared reference to the same data concurrently being alive. That is UB.

2 Likes

Sorry I don't see the picture here. Did you mean my above code is valid but we can't have a method to do this on Cell itself?

Consider this Clone implementation:

struct Inner(Option<Rc<Cell<Inner>>>);

impl Clone for Inner {
    fn clone(&self) -> Self {
        if let Some(inner) = &self.0 {
            // Creates a mutable referene to self if self is circular
            inner.set(Inner(None));
        }
        Inner(None)
    }
}


fn main() {
    let x = Rc::new(Cell::new(Inner(None)));
    x.set(Inner(Some(x.clone())));
    
    // this results in UB
    // Cell::clone_value(&*x)
}
1 Like

The whole idea behind Cell is that it can't give out a T& when you have a Cell<T>&. Which is exactly what calling T::clone from Cell<T>::clone_value would require.

7 Likes

Your code calls take, which requires Default and replaces the content of the cell with its result.

The method you can implement is hence this:

impl<T: Clone + Default> Cell<T> {
    pub fn clone_value(&self) -> T {
        let value = self.take();
        let cloned = value.clone();
        self.set(value);
        cloned
    }
}
3 Likes

Thanks for the example. I'm not sure if I understand correctly but its seem like this problem happens only if the cell contains the the value it is going to clone while cloning the value. That mean if we require Default as the example provided by @SkiFire13 it should be fine.

If this is UB, where is the unsafe code?

Or the point is that the required signature for Clone::clone_value (with just a T: Clone bound) can't be implemented without unsafe code?

Is this still UB with @SkiFire13's implementation? (with a T: Clone + Default bound)

@SkiFire13's version is safe, but if the clone implementation panics the Cell will be left with the default value and the original value will be lost - to fix that you'd also have to catch_unwind around the clone and make sure to restore the original value - I tried this in Rust Playground but this involves some AssertUnwindSafe that may be dubious.

No need for catch_unwind; you can do this with Drop. See PawnExt in pawn - Rust for a sketch of how this might look.

Using that library, cloning a Cell looks like let x = my_cell.pawn().clone();, which internally works by swapping in Default::default(), cloning the thing of which it now has ownership, and then the Drop on the temporary swaps the original back into my_cell again.

10 Likes

Very cool! Is there a proposal to add this to the stdlib?

I don't think it is important for the clone implementation to be unwind safe considering &Cell<T> is already not unwind safe.

It's a little demo I tossed together many years ago. I don't have any plans to propose it, personally.

Though it's interesting that this is the second time in a couple months someone's asked me that :stuck_out_tongue:

Ah yes, that's much nicer :slight_smile: