Tracking issue for {Rc, Arc}::get_mut_unchecked

[SimonSapin]

On Rc and Arc an new unsafe get_mut_unchecked method provides &mut T access without checking the reference count. Arc::get_mut involves multiple atomic operations whose cost can be non-trivial. Rc::get_mut is less costly, but we add Rc::get_mut_unchecked anyway for symmetry with Arc.

These can be useful independently, but they will presumably be typical when uninitialized constructors (tracked in #63291) are used.

An alternative with a safe API would be to introduce UniqueRc and UniqueArc types that have the same memory layout as Rc and Arc (and so zero-cost conversion to them) but are guaranteed to have only one reference. But introducing entire new types feels “heavier” than new constructors on existing types, and initialization of MaybeUninit<T> typically requires unsafe code anyway.

PR #62451 adds:

impl<T: ?Sized> Rc<T> { pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {…} }
impl<T: ?Sized> Arc<T> { pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {…} }

---

Open questions / issues

• Rename to get_unchecked_mut to match https://doc.rust-lang.org/std/?search=get_unchecked_mut ?
• Arc::get_mut_unchecked does not mention restrictions on drop #136322

These functions should be renamed to get_unchecked_mut for consistency with other similar APIs in the standard library. See for example: https://doc.rust-lang.org/nightly/std/pin/struct.Pin.html#method.get_unchecked_mut

[CAD97]

Are the safety docs for this correct? They currently say

Any other Arc or Weak pointers to the same allocation must not be dereferenced for the duration of the returned borrow. This is trivially the case if no such pointers exist, for example immediately after Arc::new.

This means that this is sound:

let mut arc = Arc::new(0);
let clone = Arc::clone(&arc);
unsafe {
    *Arc::get_mut_unchecked(&mut arc) = 1;
}
if *clone == 1 {
    println!("1"); // this branch is always taken
} else {
    println!("0");
}

I don't think this is necessarily incorrect (and Miri passes), but it should be noted that ArcInner doesn't put the data payload in an UnsafeCell, and it is always behind a ptr::NonNull. In fact, I've re-convinced myself that this is correct while writing this comment, but I think it could be more directly called out in the docs that this is sound. (It's subtle why.) This is new surface area to (A)Rc that isn't exposed on stable, and isn't replicated by a Unique(A)Rc type.

[SimonSapin]

As far as I understand, the absence of UnsafeCell is ok here because we’re not mutating through a &_ shared reference.

Can you say some more about what is subtle and how you conclude that this is sound?

[CAD97]

The main thing is that I previously considered an Arc to be "just" a shared box, where get_mut_unchecked exposes the falsehood of that understanding via allowing use of Arc for (completely unchecked) internal mutability.

I just found it nonobvious that Arc enables shared mutability, that's really it. Without this API it only exposes unshared mutability (of the payload).

[SimonSapin]

We previously had get_mut and make_mut that allowed mutability through Arc, though yes that is only with (runtime checking that) no sharing (is) happening.

[SimonSapin]

I initially documented this as safe when there is only one reference (no sharing), then Ralf pointed out this can be relaxed: #62451 (comment)

[CAD97]

I wrote a bit more about this over on irlo. TL;DR I really think making the RcBox/ArcInner types { strong: UnsafeCell<usize>, weak: UnsafeCell<usize>, value: UnsafeCell<T> } is a good change for clarity of implementation and avoiding problems like Rc::get_mut(Rc::from_raw(Rc::into_raw(_))) potentially being unsound.

[CAD97]

cc @RalfJung on this tracking issue since it plays into pointer provenance rules.

[RalfJung]

avoiding problems like Rc::get_mut(Rc::from_raw(Rc::into_raw(_))) potentially being unsound.

The fix for that, IMO, is to not go through deref in into_raw but use a raw ptr to the RcBox/ArcInner directly.

[CAD97]

https://internals.rust-lang.org/t/rc-and-internal-mutability/11463/13

get_mut_unchecked can cause a lifetime to be too short while still meeting the requirement of "no other rc types are 'used' over the mutation":

#![feature(get_mut_unchecked)]

use std::rc::Rc;

unsafe fn f<'a>(mut r: Rc<&'a str>, s: &'a str) {
    *Rc::get_mut_unchecked(&mut r) = &s;
}

fn main() {
    let x = Rc::new("Hello, world!");
    {
        let s = String::from("Replaced");
        unsafe { f(x.clone(), &s) };
    }
    println!("{}", x);
}

Found by @xfix

So either a "this doesn't do bad things with lifetimes" rule needs to be added, or it should go back to only allowing when get_mut would succeed.

---

Later edit: for a clearer picture of what is happening, see a version with the reference-of operator lifted. Changing it from &mut x to &mut x.clone() allows it to compile, as the cloned version can reduce its lifetime's length.