Introduce a new type MoveCell<T> in std::cell

[nox]

No description provided.

[nox]

Cc @Amanieu

[Amanieu]

I think I like this RFC better considering that MoveCell can't implement a lot of traits (Debug, Eq, anything that requires &self) that Cell implements.

[ticki]

I would alter take to move the cell, instead of leaving Default::default() in its place.

Another method, which could be useful, is clone_out, which clones the inner value.

[nox]

clone_out is unsound because it would call the clone method from behind the inner UnsafeCell.

I don't understand what you mean about moving the cell for take. Do you mean consuming it?

Edit: s/unsafe/unsound/

[Amanieu]

You should add into_inner to consume the MoveCell and get its contents.

[ticki]

@Amanieu My point is, do that with take.

[Amanieu]

@ticki But on every other type the take method moves out of the original object and then resets it to a default state. It wouldn't make sense to break this convention here.

[alexcrichton]

@SimonSapin I recall when talking about this in person you mentioned wanting this before but ended up abandoning the idea in favor of a custom and specialized implementation. Could you elaborate that here as well? (I'm also forgetting the details...)

[glaebhoerl]

I think I like this RFC better considering that MoveCell can't implement a lot of traits (Debug, Eq, anything that requires &self) that Cell implements.

The difference there can be simply between Cell<T> and Cell<T> where T: Copy. That's what conditional impls are for after all. I don't really see a reason for this to be a separate type instead of extending Cell.

[nox]

I don't really see a reason for this to be a separate type instead of extending Cell.

Because documenting such a double usage type is a pain.

[nagisa]

There is barely anything to document. You make a Cell<T>::move(t: T), you have a Cell<T: !Copy> and cannot use any of the Cell<T: Copy> methods. The compiler will emit an error. All that’s left to make a MoveCell happen is a type MoveCell<T> = Cell<T>; in your own code if you want it.

[nox]

How do you explain in prose to people that sometimes it copies, as it has always done, and that sometimes it doesn't? And that PartialEq and friends work only in the former case? This has been discussed multiple times both IRL and on IRC, let's not bikeshed this again to the infinite. See also the discussion on #1651.

[SimonSapin]

@alexcrichton I meant this:

impl<T> MoveCell<Option<T>> {
    #[inline]
    pub fn is_none(&self) -> bool {
        unsafe {
            (*self.0.get()).is_none()
        }
    }

    #[inline]
    pub fn take(&self) -> Option<T> {
        unsafe {
            (*self.0.get()).take()
        }
    }
}

impl<T> MoveCell<Option<Weak<T>>> {
    #[inline]
    pub fn upgrade(&self) -> Option<Rc<T>> {
        unsafe {
            match *self.0.get() {
                Some(ref weak) => weak.upgrade(),
                None => None,
            }
        }
    }
}

impl<T> MoveCell<Option<Rc<T>>> {
    /// Return `Some` if this `Rc` is the only strong reference count,
    /// even if there are weak references.
    #[inline]
    pub fn take_if_unique_strong(&self) -> Option<Rc<T>> {
        unsafe {
            match *self.0.get() {
                None => None,
                Some(ref rc) if Rc::strong_count(rc) > 1 => None,
                // Not borrowing the `Rc<T>` here
                // as we would be invalidating that borrow while it is outstanding:
                Some(_) => self.take(),
            }
        }
    }
}

Coherence rules would not allow me to write these impls with a MoveCell defined in the standard library (and they’re specific-purpose enough that they don’t necessarily make sense to define in the standard library). But now that I think about it, I could define extension traits that use as_unsafe_cell.

By the way, the key to these method’s safety is that they’re known not to exercise the cell’s interior mutability (e.g. through reference cycles) while a &T reference to the inside of the cell exists. Another example is:

impl<T> MoveCell<T> where T: WellBehavedClone {
    #[inline]
    pub fn clone_inner(&self) -> T {
        unsafe {
            (*self.0.get()).clone()
        }
    }
}

/**
    A Clone impl that will not access the cell again through reference cycles,
    which would introduce mutable aliasing.
    Incorrect example:
    ```rust
    struct Evil(Box<u32>, Rc<MoveCell<Option<Evil>>>);
    impl Clone for Evil {
        fn clone(&self) -> Self {
            mem::drop(self.1.take());  // Mess with the "other" node, which might be `self`.
            Evil(
                self.0.clone(),  // use after free!
                Rc::new(MoveCell::new(None))
            )
        }
    }
    unsafe impl WellBehavedClone for Evil {}  // Wrong.
    let a = Rc::new(MoveCell::new(None));
    a.set(Some(Evil(Box::new(5), a.clone())));  // Make a reference cycle.
    a.clone_inner();
    ```
*/
pub unsafe trait WellBehavedClone: Clone {}
unsafe impl<T> WellBehavedClone for Rc<T> {}
unsafe impl<T> WellBehavedClone for Weak<T> {}
unsafe impl<T> WellBehavedClone for Option<T> where T: WellBehavedClone {}

In this case T::clone can have arbitrary behavior as far as this module is concern, so we add a bound on an unsafe trait.

[alexcrichton]

Thanks for the clarification @SimonSapin! Seems like a good impetus for perhaps stabilizing as_unsafe_cell on other types as well.

[nagisa]

How do you explain in prose to people that sometimes it copies, as it has always done, and that sometimes it doesn't?

“Copies when the stuff inside the cell is copyable. In case its not, you’re limited to only these methods which move.”

---

I feel like I’m more in favour #1651 (but not very strongly). Interior mutability is quite an advanced topic already and I don’t find impl<T: Copy> /* ... for */ Cell<T> (CopyCell) vs impl<T> /* ... for */ Cell<T> (MoveCell) to be that complex of a distinction to understand.

[alexcrichton]

@rfcbot fcp close

I personally feel that this is a bit too niche of a type to fit into the standard library. We already have a somewhat dizzying suite of FooCell types in the standard library, and this would cognitively be another one to worry about which is actually only applicable in some niche situations.

Most users can get by alright with RefCell, and only in the very rare scenario that the borrow flag cpu usage or memory footprint is high would you want to reach for this. That, compounded with the fact that this can easily be a crate on crates.io makes me think that this doesn't quite belong in the standard library yet.

I'm curious to hear what others think though!

[rfcbot]

Team member @alexcrichton has proposed to close this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nox]

@alexcrichton Fair enough. What needs to be done to move mitochondria to the nursery?

[Manishearth]

(The reason I think the nursery is a good idea is that we can advertise it from the std::cell docs. We can do that even if it isn't in the nursery, but we want the docs to link to long-term maintained "semi-official" solutions, and that's sort of what the nursery does)

[alexcrichton]

I would personally feel like it's a bit premature to move to the nursery, basically for the same reasons as before where this doesn't seem to be a widespread issue in the sense that we need to raise awareness and bless a crate (some of the purposes of the nursery)

[Manishearth]

Are we okay with linking to this in the docs without it being in the nursery?

[aturon]

I agree with @alexcrichton: for the vast majority of users, the overhead of RefCell is negligible.

@Manishearth

Can you explain why you think it's important to raise global awareness of this library? RefCell seems perfectly adequate for most people.

[Manishearth]

I've seen this crop up multiple times, that's all -- it feels like it fills a hole in std::cell.

[aturon]

@Manishearth

Can you elaborate on what, specifically, is coming up? In particular, why isn't RefCell enough?

[Manishearth]

It's basically:

• People want to avoid the overhead
• Being able to have a cell type for non-copy types in FFI (since you need cells in FFI a lot) that is layout-compatible with T. (This needs Specify #[repr(transparent)] #1758 to be future-proof, but it works now with Cell<T> and UnsafeCell<T>)

[aturon]

I'm pretty skeptical of the overhead issue (except for very rare cases like Servo where RefCell could impose a large memory footprint overhead due to widespread use). Would be a lot more convincing with some evidence.

The FFI point is more interesting -- it wasn't mentioned in the RFC's motivation. Can you give a more detailed example or two showing how/why this gets used there?

[codyps]

except for very rare cases like Servo

Is there something about the design of Servo that makes you expect that no other crate would do similar things? Or is there another reason you think that Servo is a rare case?

[SimonSapin]

In Servo we’d use MoveCell instead of RefCell in cases where each node of something contains a few cells, and there can be many nodes so saving one word of memory per cell ends up being significant. I don’t think Servo is that exceptional in this respect, there’s probably other applications that handle lots of data and are memory-constrained.

But I also think it’s fine for MoveCell not to be in the standard library. If you get to the point where MoveCell can bring significant memory savings to your app, you can afford to add one dependency line to your Cargo.toml file.

[alexcrichton]

I personally feel that Servo's use case quite well motivates the existence of MoveCell. That is, from a technical side of things, it totally makes sense to exist.

That being said, being a dependency of Servo does not imply that something should be in the standard library. As @SimonSapin mentioned once you can really see the wins of this optimization adding one dependency is very small.

[Manishearth]

The FFI point is this: When talking with C, you often need to turn off noalias semantics because C might mutate things. Some values (especially C-managed ones) might need C-like aliasing semantics (mutation+sharing allowed). If they have subfields which you wish to deal with from the Rust side, you need to make at least those fields mutable via interior mutability. RefCell isn't a solution since it has an extra field, but UnsafeCell, Cell, and MoveCell are.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

[jbj]

Doesn't MoveCell have an important advantage over RefCell in that borrow checking happens at compile time instead of run time? Every use of RefCell is a potential source of panic that must be inspected for correctness at every step in the evolution of the code.

[alexcrichton]

@jbj right yeah that's the advantage of using Cell rather than just having RefCell. There's clearly utility to MoveCell proposed here, solving a problem neither Cell or RefCell are solving, but this is mostly a question of whether it belongs in the standard library or not.

[Diggsey]

I think it's worth having in the standard library because it's fairly common to require inner mutability of elements in a large array, where RefCell imposes a reasonably large space overhead. Being in std::cell makes it a lot more discoverable, as I'm sure most people are not even aware that rust has a safe, zero-cost way to achieve this.

[alexcrichton]

My feeling is the same as @aturon's. Which is to say that yes MoveCell has a clear advantage, but it's not worth the cognitive overhead. Although interior mutability may be fairly common, it's very rare that the overhead of a the reference count in RefCell really matters. When it does come up then reaching for a crate on crates.io should be reasonable.

[aturon]

The FCP has elapsed. At this point, the libs team consensus continues to be that while this functionality is worthwhile in general, it doesn't yet merit adding an additional Cell type to std, and can be provided in crates.io for the time being.

That said, the related RFC #1651 would also address the motivations of this RFC, but do so without adding an entirely new type. There's not yet a clear consensus on that RFC, but continued discussion should probably happen there.

Thanks all!