Tracking Issue for `cell_get_cloned`

[frank-king]

Feature gate: #![feature(cell_get_cloned)]

This is a tracking issue for Cell::get_cloned for T: CloneFromCopy (e.g. Rc<T>, Arc<T>, etc.).

ACP: rust-lang/libs-team#572

Public API

/// Safety: can call `Self::clone(&*ManuallyDrop::new(ptr::read(ptr_to_self)))`
#[marker]
pub unsafe trait CloneFromCell: Clone {}

// Can't do this, unsound
// unsafe impl<T: Copy> CloneFromCell for T {}

unsafe impl<T: ?Sized> CloneFromCell for Rc<T> {}
unsafe impl<T: ?Sized> CloneFromCell for Arc<T> {}
unsafe impl<T: ?Sized> CloneFromCell for rc::Weak<T> {}
unsafe impl<T: ?Sized> CloneFromCell for sync::Weak<T> {}

// basically all std types that don't have indirection:
unsafe impl<T: CloneFromCell, const N: usize> CloneFromCell for [T; N] {}
unsafe impl<T: CloneFromCell> CloneFromCell for Option<T> {}
unsafe impl<T: CloneFromCell, E: CloneFromCell> CloneFromCell for Result<T, E> {}
// ...

impl<T: CloneFromCell> Cell<T> {
    pub fn get_cloned(&self) -> Self;
}

Steps / History

(Remember to update the S-tracking-* label when checking boxes.)

• Implementation: add CloneFromCell and Cell::get_cloned #145685
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Should this remain get_cloned or move to the Clone impl for Cell? Add Cell::get for Rc-like shallow-copying types libs-team#572 (comment)
• Concern: the soundness conditions here are extremely tricky to get right. We need to have a warning about this.
• The soundness requirement on CloneFromCopy as stated above is likely wrong.

cc @rust-lang/libs-api @rust-lang/lang

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[theemathas]

unsafe impl<T: Copy> CloneFromCopy for T {}

This impl seems problematic. Cell::get_cloned has to call T::clone, which may run arbitrary user code, even for Copy types.

[frank-king]

unsafe impl<T: Copy> CloneFromCopy for T {}

This impl seems problematic. Cell::get_cloned has to call T::clone, which may run arbitrary user code, even for Copy types.

T is copied (via ptr::read) before calling T::clone, and thus there's no reference to the copied T, so T::clone cannot do harm to it.

[ais523]

/// Safety: can call `Self::clone(&*ManuallyDrop::new(ptr::read(ptr_to_self)))`
#[marker]
pub unsafe trait CloneFromCopy: Clone {}

…

impl<T: CloneFromCopy> Cell<T> {
    pub fn get_cloned(&self) -> Self;
}

I believe that, given the stated safety condition, this is unsound.

Box<T> where T: Clone is an example of a type which can, according to the stated safety condition, implement CloneFromCopy (assume either that the special case that makes the copy itself UB gets removed, or that you are using a MyBox<T> that doesn't have such a case). The reason is that a copy of a Box is just a pointer to the contents of the box (like the original is), and you can clone via the copy of the pointer just as easily as you can clone via the original pointer.

However, cloning a Box<T> can change the value of a Cell<Box<T>> (if T has a sufficiently perverse clone implementation). Doing that won't cause the copy of the pointer to be dropped – but it could cause the allocation that it points to to be dropped (e.g. the Clone implementation could take the Box from the Cell by swapping another Box into its place, then drop the Box it took, causing the copy of the Box to now be pointing to deallocated memory and subsequently causing a use-after-free).

The correct safety requirement is unrelated to the ability to clone from a copy: rather, the requirement we want is that cloning does not write to any cells in a way that could cause the value inside a Cell<T> to be modified, moved out of the cell, or dropped. Cloning from a copy (rather than the original) doesn't actually solve any of these problems: modifying the value could drop something that the copy of the value references, as could dropping it, and moving it out of the cell makes it possible to modify or drop it without the cell being involved. However, once you enforce that more stringent safety requirement, there's no longer a need to actually do the copy out of the cell any more! So CloneFromCopy is probably the wrong trait to be using here – it should be some sort of CloneFromCell that asserts that the cloning operation doesn't change cells in a problematic way.

(Some context: I've recently been doing a study into what Rust would look like if "create a new value by copying the bits of an existing value, even if the type isn't Copy" were supported by the language and type system as a first-class operation. One of the main conclusions was that to be usable, the copies generally have to have a lifetime – otherwise, you can't guarantee that things that the original value referenced will still be alive by the time you try to use the copy, and thus you can't soundly use them via the copy. That made me realise that the API written here doesn't work, because it doesn't specify a lifetime for how long the copy remains valid, and if you could guarantee that the copy were still valid you could just as easily guarantee that the Cell still had contents that could safely be used directly. Note that without such a lifetime, even Rc can't be cloned safely from a copy: the original Rc that you copied might have been dropped since, and it might have been the last reference.)

[frank-king]

cloning does not write to any cells in a way that could cause the value inside a Cell to be modified, moved out of the cell, or dropped.

I strongly agree with you. As from the ACPs #572 and #648, we started from the same point that we want Cell<Rc<T>> be assessed by clone (or by get), and besides Rc<T>, I've just thinking of more general cases, so I proposed a trait CellGet (now the CloneFromCopy) that may apply to types similar to Rc<T>. The current statement of the safety property is not accurate enough and I really appreciate your feedback.

Is your statement an informal sketch or do you already have done some verification or proof?

One of the main conclusions was that to be usable, the copies generally have to have a lifetime – otherwise, you can't guarantee that things that the original value referenced will still be alive by the time you try to use the copy, and thus you can't soundly use them via the copy.

What an interesting conclusion. It sounds just like "borrowing" with auto-deref?

[ais523]

Is your statement an informal sketch or do you already have done some verification or proof?

It's based on the "cell permission" model, in which each cell is conceptually tied to a "permission object" (you can tie multiple cells to the same object but don't have to), and you can't read the cell unless you could safely shared-borrow its permission object, and can't write the cell unless you could safely exclusive-borrow its permission object. I haven't personally proven the model itself to be correct, but there is related work, which does contain proofs, e.g. ghost-cell is effectively an implementation of the cell permission model and the theory behind it has been formally proven (although I'm not sure whether or not there's a proof that the implementation actually matches the theory).

The basic idea is that in order to read inside a cell, you have to shared-borrow its permission object, so you can't then do things that require an exclusive reference (write/move) to that same cell as that would require a conflicting borrow. It's difficult to express "the same cell" in the type system, but you can broaden it to "cells of the same type" and get a safety argument that way. I'd consider it more of an informal argument than anything – a formal model exists, but it hasn't been formally proven to be relevant in this context, and the way the model is applied to the situation hasn't been formally proven to be correct either.

Thinking about what I just wrote, we might not need to explicitly ban drops; you can't drop the cell you're cloning from, because get_cloned takes a shared reference to the cell which prevents it being dropped, and if the cell isn't dropped you can't drop its contents without first overwriting them or moving them out (thus, banning moves and writes is enough).

One of the main conclusions was that to be usable, the copies generally have to have a lifetime – otherwise, you can't guarantee that things that the original value referenced will still be alive by the time you try to use the copy, and thus you can't soundly use them via the copy.

What an interesting conclusion. It sounds just like "borrowing" with auto-deref?

It's definitely related – the primary difference is that the thing that is being borrowed doesn't necessarily need to exist in memory in a usable form at all (e.g. it might be in a register, or might be a field of a #[repr(packed)] struct and thus unreferenceable due to being misaligned). It's pretty similar to a shared reference, but based on values rather than addresses (thus there isn't a pointer behind it, like there would be for a reference, meaning that you can't implement the Deref trait: all you have is a bit pattern).

[tgross35]

Seeing some iterations of the implementation PR, I think CloneFromCopy may not be the best name because it seems to indicate some relation to Copy. However, (1) it doesn't apply to everything Copy (which of course implies Clone), and (2) it applies to many things that have nothing to do with Copy.

Unhelpfully I don't have any great suggestions. PartialPureClone crossed my mind because Clone must be free of side effects for the local type (i.e. the Rc struct itself) but it may have limited side effects via indirection (i.e. the Rc counters). Or FreezeClone for similar reasons.

[programmerjake]

I think CloneFromCell is a pretty good name because it literally describes the operation we're trying to do, also because if you have a &Cell<T> you can also obtain a &Cell<Field> for each field of T (edit: stuff in parenthesis turns out to not work: #145329 (comment) this includes stuff like Deref but for like going from &Cell<Box<U>> to &Cell<U> or &Cell<&mut U> to &Cell<U>).

[programmerjake]

(edit: turns out to not work: #145329 (comment))

so maybe CloneFromCell could just be a safe trait, and then the implementers would use unsafe internally if necessary (which for most normal types would become unnecessary when rust gets projections &Cell<T> to &Cell<Field> and stuff like Box<T>: DerefCell)

pub trait CloneFromCell {
    // can be self instead of this if Cell<T>: Receiver<Target = T>
    fn clone_from_cell(this: &Cell<Self>) -> Self;
}

// future stuff not part of this tracking issue:

impl<T: ?Sized> Receiver for Cell<T> {
    type Target = T;
}

pub trait DerefCell: Deref {
    fn deref_cell(self: &Cell<Self>) -> &Cell<Self::Target>;
}

impl<T: ?Sized> DerefCell for Box<T> { ... }
impl<T> DerefCell for Vec<T> { ... }
impl DerefCell for String { ... }
...