Implement Copy and Clone for UnsafeCell<T>

[jamesmunns]

This change implements Copy and Clone for UnsafeCell<T>, if T implements Copy and/or Clone.

I am opening this as a PR, rather than as an RFC, because I am unsure whether this falls under the category of "Nontrivial trait impl (because all trait impls are insta-stable)" or "Implementing an "obvious" trait like Clone/Debug/etc", and the guide suggests to do whatever is easier. I am happy to close this PR and write a full RFC, if necessary.

I believe this is sound, as UnsafeCell<T> does not provide public access to self.value, and the accessor methods get() and set() always refer to item contained within self. Therefore if the contained item can be cloned or copied, I believe the containing Cell should also be able to copied or cloned.

The motivation for this change is to allow the use of UnsafeCell<T> in static arrays, using the [T; N] syntax. For this syntax to work, T must be Copy. For example, if we wanted to make an array of [UnsafeCell<u16>; 128], it would be necessary to fully type out the initializer 128 times, which is not desirable, and difficult to do at global scope with a const_fn or macro.

As of now, the #[stable] attribute is incorrect, and must be fixed before merging. I would also appreciate guidance on how to fill this out.

[rust-highfive]

r? @aidanhs

(rust_highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:39] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:39] tidy error: /checkout/src/libcore/cell.rs:1515: TODO is deprecated; use FIXME
[00:04:39] tidy error: /checkout/src/libcore/cell.rs:1523: TODO is deprecated; use FIXME
[00:04:41] some tidy checks failed
[00:04:41] 
[00:04:41] 
[00:04:41] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:41] 
[00:04:41] 
[00:04:41] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:41] Build completed unsuccessfully in 0:00:46
[00:04:41] Build completed unsuccessfully in 0:00:46
[00:04:41] Makefile:79: recipe for target 'tidy' failed
[00:04:41] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:15218e6c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0d80238b:start=1539966618169778301,finish=1539966618175015462,duration=5237161
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:08414379
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:09e8d5fc
travis_time:start:09e8d5fc
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0dbfad3c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[jamesmunns]

The Travis build failed because I had a // TODO noting that the #[stable] attr is incorrect. I will wait for some feedback before I fix this, or let me know if it is better to fix first.

[cramertj]

cc #25053

[jamesmunns]

Thanks for the context @cramertj ! Open to new input, though it seems this is still in needs-decision land. I doubt my explanation will provide new clarity, but perhaps it will spark new discussion :)

[TimNN]

Ping from triage @aidanhs / @rust-lang/libs: This PR requires your review.

[SimonSapin]

The [UnsafeCell<T>; N] initializer use case is compelling, but I’m really uncertain regarding the soundness of these impls in the general case. Note that UnsafeCell does not have get and set methods, only Cell does. Cell is based on the principle of forbidding inner references, RefCell on runtime access tracking, etc.

Similarly UnsafeCell is based on the principle that every access is unsafe as it goes through a raw pointer. It is the responsibility of the user (or wrapping library) to consider the soundness of each access. Clone creates a "hole" in this principle as provides inner references &T to arbitrary code T::clone, without unsafe.

[withoutboats]

To restate the concern: adding these impls would imply that projecting &UnsafeCell<T> to &T is safe if T: Clone. Questions to me seem:

• In the abstract, are we fine with saying this is safe?
• Has anyone in practice already relied on the assumption that this is not safe (e.g. by exposing &UnsafeCell<T> in an API where it would be unsafe to project that to &T)?

I'd also add that a slightly more conservative variant would be to only implement Clone for UnsafeCell<T> where T: Copy. In that case, we know that the operation being performed on &T is restricted to a memcpy. I don't know how much this resolves safety concerns though.

cc @RalfJung

[RalfJung]

To restate the concern: adding these impls would imply that projecting &UnsafeCell to &T is safe if T: Clone. Questions to me seem:

The reason we do this is that this is the only way to make UnsafeCell Copy, right?

I think in the abstract, this is fine. UnsafeCell<T> is actually no different from T itself in terms of invariants, which should justify these operations. Anyone wrapping a newtype around UnsafeCell then has it in their hands whether to expose Clone, and it is up to them to justify that.

Has anyone in practice already relied on the assumption that this is not safe (e.g. by exposing &UnsafeCell in an API where it would be unsafe to project that to &T)?

This is possible in theory, but note that this applies almost any time we add a new trait impl, or really any safe operation for an existing type in libstd: In theory there might be code out there relying on this operation not to exist. The mere existence of this possibility shouldn't stop us.

I am not sure if there is a good way to find out if this ever happened. UnsafeCell seems like an unlikely type to expose in an interface though.

[RalfJung]

In terms of motivation, what happened to the plan of impl<T: Copy> Copy for Cell<T>? I guess that would also be complicated by the fact that Copy then also has to implement Clone...

[SimonSapin]

Although we deprecated and then removed it (in favor of as_ptr), we used to have RefCell::as_unsafe_cell(&self) -> &UnsafeCell<T>. Combined with UnsafeCell<T>: Clone, this could be used to have &mut T (through borrow_mut()) and &T separately to the same value at the same time.

If projection form &UnsafeCell<T> to &T is safe, then users of UnsafeCell must keep it private and not have a method like RefCell::as_unsafe_cell (which so far was fine soundness-wise, if not particularly useful).

Changing the soundness requirements of an API that has been stable since Rust 1.0 is what makes me uncomfortable.

---

However, having Clone for UnsafeCell<T> require T: Copy and be implemented as a copy (not calling arbitrary code) resolves this concern, I think.

[RalfJung]

This shouldn't call clone(), that is still unknown code executing.

It should make a Copy instead.

[jamesmunns]

Is there an explict way to call copy? or is this acceptable?

#[stable(feature = "unsafe_cell_copy_clone_conservative", since = "1.31.0")]
impl<T: Clone + Copy> Clone for UnsafeCell<T> {
    fn clone(&self) -> UnsafeCell<T> {
        UnsafeCell::new(self.value)
    }
}

[RalfJung]

Yes that should do it.

[RalfJung]

Together with a comment why we are not calling clone().

[jamesmunns]

@RalfJung impl'd in 67bcac6, thanks!

[SimonSapin]

Since Self: Copy the body of this method can even be just *self, though this is equivalent.

[jamesmunns]

I've implemented the suggestion from @withoutboats, as this still addresses my original use case, and seems to have some consensus as being "more correct".

Additionally, once the discussion on whether to do this is complete, I would be happy to add inline documentation that captures the points made in this thread, and what guarantees need to be made for correct usage of these new impls, but I will wait until the discussion has stabilized.

[alexcrichton]

For users which expose &UnsafeCell<T> (like we almost did for RefCell), wouldn't this Copy implementation still be unsound for them? For example with RefCell<T> you could do:

let x = RefCell::new(2);
let mut y = x.borrow_mut();
let z: &mut u32 = &mut *y;
let copy = *x.as_unsafe_cell(); // reads from `z`, but not through `z`

[RalfJung]

@alexcrichton Yeah, as_unsafe_cell outright assumes that UnsafeCell will never be Copy, so making it Copy is unsound. Or rather, as_unsafe_cell is unsound if you ask me.

[SimonSapin]

as_unsafe_cell is not unsound today, nor since Rust 1.0. We are proposing to make it so now, and IMO that qualifies as a breaking change.

I think I’d prefer to introduce a new type (straw man name: SuperUnsafeCell) that could be Copy and even Sync (#53639 (comment)). That new type would be the lang item, and both UnsafeCell and Cell would be based on it.

[alexcrichton]

Yes my point was the same as @SimonSapin how regardless of whether or not as_unsafe_cell exists today this is technically a breaking change in the unsoundness guarantees.

I agree with @SimonSapin as well though that we could create a new type for this, although I might be more partial to the name UnsafeCellCopy rather than SuperUnsafeCell :)

[SimonSapin]

Yeah that name was not an actual proposal :) It was only intended as a way to refer to it by name rather than paraphrase for the purpose of this discussion.

Maybe not have Copy in the name though, if we want to also make it Sync. Though the soundness of being both Copy and Sync and have internal mutability is dubious, so maybe this new type should be unsafe to construct?

pub struct YoloCell<T: ?Sized> /* still not a serious name proposal */ {
    pub value: T,
    _private: (),  // prevent construction by struct literal
}

impl<T: ?Sized> Copy for YoloCell<T> {}
unsafe impl<T: ?Sized> Sync for YoloCell<T> {}

impl<T> YoloCell<T> {
    pub unsafe const fn new(value: T) -> Self {
        Self { value, _private: () }
    }
}

Back to naming, I was thinking of this type as the (new) fundamental basis for interior mutability, and UnsafeCell could be re-branded as "all accesses to the value require unsafe".

[RalfJung]

In your proposal, would UnsafeCell then get deprecated? I think it should, it would otherwise just be confusing.

---

I think at some point we have to figure out a general guidelines for how to handle things that are "accidentally safe". To what extend do we commit to guarantees that never really were meant as such?

One consequence of our conservative approach to extending libstd and compiler functionality is that there will always be things that happen to be impossible at some point not because of a design decision, but because we didn't want to make the decision to make it possible yet. Thus many new features will technically be breaking changes, but only if unsafe code authors were "too aggressive" in exploiting "happy accidents" of the then-current libstd/rustc. Ironically, the more careful we are with new features, the more we draw ourselves into this corner!

One example of that in the past would be the introduction of unions, which broke (as in "made unsound") some GC crates because they provided another way to not drop things. I see no fundamental difference between that situation and this, except that this time we actually have a realistic chance of avoiding the breakage. (Which is a good enough reason; if the policy is "we are fine breaking such accidental guarantees if maintaining them is not possible with reasonable effort while still advancing the language" that'd be a fine outcome.)

As another example, consider the fact that const fn is currently extremely restricted in what can do (e.g. no heap allocations, no writing to &mut, no unsafe operations, no casting pointers to integers). Yet we absolutely do not intend that to be a guarantee that const fn will never do that. Still, I could imagine unsafe code relying on these limitations (and I think that would be no less reasonable than relying on UnsafeCell being non-Copy), so technically extending const fn is a breaking change.

This also includes implementing basically any trait for any type, because it is conceivable that unsafe code relies on certain traits to rule out certain types. (In fact this is exactly what happens here: Unsafe code, for no good reason other than "that's how it currently is", relied on Copy ruling out UnsafeCell.)

[SimonSapin]

UnsafeCell could be deprecated if replaced, but I’m not sure it should. It’s not particularly flawed for cases where it works today. (Cases that don’t need Copy.)

FWIW at this point, this new cell type is just an idea and I’m trying to figure out "out loud" how it would work and what the consequences would be. I’m not claiming (yet) this is definitely the direction we should go.

---

I think this is a valid concern in general, but I’m not convinced it applies here. I believe this aspect of UnsafeCell was very much intentional. The name alone strongly indicates this IMO, and the docs also support it:

(Emphasis added)

UnsafeCell<T> is a type that wraps some T and indicates unsafe interior operations on the wrapped type.

The UnsafeCell API itself is technically very simple: it gives you a raw pointer *mut T to its contents. It is up to you as the abstraction designer to use that raw pointer correctly.

I read this as strongly implying “it only gives you a raw pointer”.

[TimNN]

Ping from triage! Could someone summarize how this PR can move forward? Does it require some team decision or FCP?

Edit Or RFC?

[cramertj]

I think this is a longstanding enough conversation with enough fine details to get worked out that it should move into an RFC, even if just a small one. With that in mind, I'm going to close this PR.

[SimonSapin]

@jamesmunns We have an accepted RFC to allow repeated array initializer syntax with a non-Copy item type if the expression is const: #49147. Would that remove the motivation for having Cell: Copy, or would there also be some other use case?

In #20813 (comment) some have previously argued that Cell should not be Copy to prevent accidental copies in cases someone might have meant to share a cell and use interior mutability.

[RalfJung]

some have previously argued that Cell should not be Copy to prevent accidental copies in cases someone might have meant to share a cell and use interior mutability.

That sounds like something diagnostics might be able to help with?

[SimonSapin]

Diagnostics happen with compiler errors, right? This is a case of a valid program having potentially unintended meaning. But it’s hard for the compiler to guess intent, and I assume that “use .clone() instead of implicit copy” was the way to express intent implied by that comment I linked.

[jamesmunns]

@SimonSapin yes, I believe this addresses my needs, and I think that #49147 is a better solution IMO. Thanks for following up!