Add non-`unsafe` `get_mut` for `UnsafeCell`

impl<T : ?Sized> UnsafeCell<T> {
    pub
    fn get_mut (self: &'_ mut UnsafeCell<T>) -> &'_ mut T
    {
        unsafe {
            // Safety: outer `&mut` guarantees lack of concurrency.
            &mut *self.get()
        }
    }
}

basically there is no reason not to have it (UnsafeCell is a wrapper that only interacts with &UnsafeCell inputs: an owned or exclusive UnsafeCell-ed wrapper is a no-op, much like Pin in a Pin<impl Deref[Mut]<Target = impl Unpin>>).

On the contrary, the fact there is no such get_mut() may lead some people into wrongly thinking that UnsafeCell is special even w.r.t. to &mut access.

This is a simple, straight-forward API, that can reduce the amount of unsafe code out there: whenever one sees a .get() call on an UnsafeCell, they have to think about the soundness of such potentially concurrent or even racy access, and it currently happens even when there is an outer &mut on the UnsafeCell, when it shouldn't be the case.

8 Likes

Most of the types built on UnsafeCell have their own get_mut already, but have to use unsafe to implement it. They could all be simplified with UnsafeCell::get_mut.

3 Likes

Nitpick: I think the implementation should be &mut *self.get() (note the deref).

1 Like

My nomicon knowledge is a bit shaky. Is this allowed?

let v = UnsafeCell::new(0u32);
let y: &mut u32 = {
    let used: &UnsafeCell<_> = &v;
    unsafe { &mut *used.get() }
};
let z: &mut UnsafeCell<u32> = y;
// if this were allowed, `z.get_mut()` here would
// definitely be unsound. but is creating `z`
// allowed in the first place?

If so, it wouldn't necessarily be a problem. But it'd probably be worth adding as a note in the documentation that calling get_mut isn't always going to be sound depending on what else your code does with other unsafe accesses.

You could invent similar scenarios from any pointer, regardless of cells -- I'd say the error lies in the open-ended lifetime of that reference.

Another variant I would use a lot is this:

unsafe fn get_mut(&self) -> &mut T { &mut *self.get() }

This is particularly useful as most other methods take mutable references and the whole (*a.b.get()).do_something() dance is quite awkard.

The question is, which variant should get the name get_mut? :stuck_out_tongue:

1 Like

I wondered if there was a pointer method that would make chains easier -- there's only as_mut<'a>(self) -> Option<&'a mut T>, which also says:

If you are sure the pointer can never be null and are looking for some kind of as_mut_unchecked that returns the &mut T instead of Option<&mut T> , know that you can dereference the pointer directly.

... which doesn't help you.

1 Like

I agree, that sounds like a reasonable argument.

2 Likes

Your variant could be named assume_mut (or assume_exclusive), by analogy with MaybeUninit::assume_init; after all, in invoking the method, you assume no other reference exists, and that assumption deserves some scrutiny when auditing the code for correctness. With this name, the method is going to immediately draw you attention as the dangerous operation in the unsafe block; this wouldn’t be the case with a name as, ahem, unassuming as get_mut.

4 Likes

Based on the discussion around fixing the unsoundness of self-referential generators (which extends to all intrusive Pin based data structures) it seems to me that having this safe method would be incompatible with using UnsafeCell to suppress the noalias annotations; unless by "more like UnsafeCell" there @RalfJung meant "more like a new wrapper type similar to UnsafeCell which suppresses noalias"?

That's what I meant, yes -- basically something that suppresses aliasing assumptions like noalias even for mutable references. The details of this are still entirely unclear.

When discussing about this with @Nemo157, it seems like there is no place across the documentation / reference etc. that explicitly states that UnsafeCell is not magical w.r.t. &mut access, even though its "children" (Cell, RefCell, etc.) all do offer the equivalent .get_mut() unchecked construct.

That is an argument in favor of:

Indeed, while the lack of documentation in that regard can be easily fixed, code > documentation hints at it being more robust to actually set that property in stone with this suggested method.

I'd thus like to push this idea further, but I can't remember the right approach for this case: does adding a method require an RFC, or can I just go and submit a PR w/ an associated tracking issue for a new unstable feature, and have the discussion happen there?

I'd say this is small enough to go ahead and PR.

Agreed. Make a PR and we can do a T-lang FCP in there.