Add non-unsafe .get_mut() for Unsafecell

[danielhenrymantilla]

• Tracking issue: Tracking Issue for unsafe_cell_get_mut #76943

As discussed in: https://internals.rust-lang.org/t/add-non-unsafe-get-mut-for-unsafecell/12407

• Rendered documentation

This PR tries to move the sound &mut UnsafeCell<T> -> &mut T projection that all the "downstream" constructions were already relying on, up to the root abstraction, where it rightfully belongs, and officially blessing it.

• this helps reduce the amount of unsafe snippets out there (c.f., the second commit of this PR: 5886c38)

The fact that this getter is now expose for UnsafeCell<T> itself, will also help convey the idea that UnsafeCell is not magical w.r.t. &mut accesses, contrary to what some people incorrectly think.

• Even the standard library itself at some point had such a confusion, c.f. this comment where there is a mention of multi-threaded (and thus shared) access despite dealing with exclusive references over unique ownership:

  rust/library/core/src/cell.rs

  Lines 498 to 499 in 59fb88d

  	pub fn get_mut(&mut self) -> &mut T {
  	// SAFETY: This can cause data races if called from a separate thread,

r? @RalfJung

[RalfJung]

Even the standard library itself at some point had such a confusion, c.f. this comment where there is a mention of multi-threaded (and thus shared) access despite dealing with exclusive references over unique ownership:

That seems like a different confusion, the comment after all explains that due to unique ownership there cannot be races. But yeah, the comment is confusing.

[RalfJung]

This PR tries to move the sound &mut UnsafeCell -> &mut T projection that all the "downstream" constructions were already relying on

Well, all standard library users. We don't know what other clients do outside the standard library.
There is a slight chance that some of them rely on the fact that UnsafeCell, even when exposed directly to safe code, does not permit safe code to do anything. But I think we can discuss this on the tracking issue.

[RalfJung]

Thanks. :) @bors r+ rollup

[bors]

📌 Commit 853a9c4a4596892d6f69b9ec7f49177dfabbf1ef has been approved by RalfJung

[RalfJung]

ah wait looks like you wanted to squash some thing first
@bors r-

[danielhenrymantilla]

Ok, I think we're getting there, @RalfJung 🙂 (thx for the review!).

Now that we have an example of "using UnsafeCell as un unchecked RefCell", would it make sense to also add an example of using UnsafeCell to implement some thread-safe-because-synchronized mutation abstraction? (The only problem would be to keep that example as small / minimal as possible 🤔)

[danielhenrymantilla]

I don't really know where to post the following, so here it goes:

Next steps / potential future PR(s)

Also, as a possible follow-up for this PR, I was thinking about something such as .assume_unique() and .assume_no_muts() (modulo bikeshed of course) since currently .get() serves a dual function of being able to yield &T and &mut T through its *mut T, thus leading to different patterns and thus different safety constraints, that may be non-obvious from the function calls. Indeed, compare:

unsafe {
    // SAFETY: within this scope there are no other references to `x`'s contents,
    // so ours is effectively unique.
    let p1_exclusive: &mut i32 = &mut *p1.get();
    *p1_exclusive += 27;
}

unsafe {
    // SAFETY: within this scope nobody expects to have exclusive access to `x`'s contents,
    // so we can have multiple shared accesses concurrently.
    let p2_shared: &i32 = &*p2.get();
    assert_eq!(*p2_shared, 42 + 27);
    let p1_shared: &i32 = &*p1.get();
    assert_eq!(*p1_shared, *p2_shared);
}

to:

unsafe {
    // SAFETY: within this scope there are no other references to `x`'s contents,
    // so ours is effectively unique.
    let p1_exclusive: &mut i32 = p1.assume_unique()
    *p1_exclusive += 27;
}

unsafe {
    // SAFETY: within this scope nobody expects to have exclusive access to `x`'s contents,
    // so we can have multiple shared accesses concurrently.
    let p2_shared: &i32 = p2.assume_no_muts();
    assert_eq!(*p2_shared, 42 + 27);
    let p1_shared: &i32 = p1.assume_no_muts();
    assert_eq!(*p1_shared, *p2_shared);
}

This would have the benefit of lifetime-bounding the obtained references, which is not a bad thing either.

Indeed, let's compare:

//! Misusage
let x = UnsafeCell::new(42);
let p1 = &x;
let at_ft_mut = unsafe { &mut *p1.get() };
let p2 = &mut x;
let at_ft_mut_2 = p2.get_mut();
mem::swap(at_ft_mut, at_ft_mut_2);

which compiles fine, to:

//! Misusage
let x = UnsafeCell::new(42);
let p1 = &x;
let at_ft_mut = unsafe { p1.assume_unique() };
let p2 = &mut x;
let at_ft_mut_2 = p2.get_mut();
mem::swap(at_ft_mut, at_ft_mut_2);

which fails with:

error[E0502]: cannot borrow `x` as mutable because it is also borrowed as immutable
  --> src/main.rs:9:14
   |
7  |     let p1 = &x;
   |              -- immutable borrow occurs here
8  |     let at_ft_mut = unsafe { p1.assume_unique() };
9  |     let p2 = &mut x;
   |              ^^^^^^ mutable borrow occurs here
10 |     let at_ft_mut_2 = p2.get_mut();
11 |     mem::swap(at_ft_mut, at_ft_mut_2);
   |               --------- immutable borrow later used here

[RalfJung]

Now that we have an example of "using UnsafeCell as un unchecked RefCell", would it make sense to also add an example of using UnsafeCell to implement some thread-safe-because-synchronized mutation abstraction?

Honestly, that feels like a bit too much for a doc comment. That sounds more like a Nomicon chapter.^^

Also, as a possible follow-up for this PR, I was thinking about something such as .assume_unique() and .assume_no_muts() (modulo bikeshed of course) since currently .get() serves a dual function of being able to yield &T and &mut T through its *mut T, thus leading to different patterns and thus different safety constraints, that may be non-obvious from the function calls.

Yes, that sounds very useful. :) (for a separate PR though)

[danielhenrymantilla]

Squashed 🙂 (I prefer not to force push while an interactive review is in process, since comments may get lost / track missing commits; now that that has been done, I can finally write the PR as two semantically meaningful commits: implement the method with the appropriate docs, and then simplify some unsafe { ...get() } calls to ...get_mut())

[RalfJung]

I prefer not to force push while an interactive review is in process

That is much appreciated. :) It also makes it easier for me to see what changed from revision to revision.

[RalfJung]

@bors r+ rollup

[bors]

📌 Commit 5886c38 has been approved by RalfJung

[a1phyr]

Is there a reason why UnsafeCell::get_mut was not implemented like this ?

fn get_mut(&mut self) -> &mut T {
    &mut self.value
}

[danielhenrymantilla]

Mostly an oversight 😅, I had been using an extension trait to implement this and thus tunnel-visioned on using the .get() API. Given that your implementation does not require unsafe, it will be better to use that one indeed: filed #78735.