Clarify and restrict when {Arc,Rc}::get_unchecked_mut is allowed.

[zachs18]

(Tracking issue for {Arc,Rc}::get_unchecked_mut: #63292)

(I'm using Rc in this comment, but it applies for Arc all the same).

As currently documented, Rc::get_unchecked_mut can lead to unsoundness when multiple Rc/Weak pointers to the same allocation exist. The current documentation only requires that other Rc/Weak pointers to the same allocation "must not be dereferenced for the duration of the returned borrow". This can lead to unsoundness in (at least) two ways: variance, and Rc<str>/Rc<[u8]> aliasing. (playground link).

This PR changes the documentation of Rc::get_unchecked_mut to restrict usage to when all Rc<T>/Weak<T> have the exact same T (including lifetimes). I believe this is sufficient to prevent unsoundness, while still allowing get_unchecked_mut to be called on an aliased Rc as long as the safety contract is upheld by the caller.

Alternatives

• A less strict, but still sound alternative would be to say that the caller must only write values which are valid for all aliased Rc/Weak inner types. (This was mentioned in the tracking issue). This may be too complicated to clearly express in the documentation.
• A more strict alternative would be to say that there must not be any aliased Rc/Weak pointers, i.e. it is required that get_mut would return Some(_). (This was also mentioned in the tracking issue). There is at least one codebase that this would cause to become unsound (here, where additional locking is used to ensure unique access to an aliased Rc<T>; I saw this because it was linked on the tracking issue).

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[Kixunil]

TBH, I don't like allowing implicit shared mutability via Rc at all. People who need it should explicitly use Rc<UnsafeCell<T>> Then among other things we wouldn't have to explain these things for a bunch of types in std and just have it in the UnsafeCell doc.

[joshtriplett]

cc @RalfJung: does this look reasonable to you?

[RalfJung]

This sounds reasonable, but might be good to get @CAD97 or @SimonSapin to take a look, who are more familiar with Rc/Arc internals than I am.

[CAD97]

I'm also iffy at this point on whether we should be treating Rc as exposing its (unchecked) internal mutability.

However, I do agree that this is sufficient to close the documented soundness hole, and that any looser use should probably be using UnsafeCell.

The only additional case I'd consider allowing intrinsically is when some Rcs are dyn Trait to the same concrete type (including lifetimes).

---

So to be explicit: this is a clear improvement to the current documentation/guarantees.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[RalfJung]

r? libs

[SimonSapin]

Given how finicky they are to use correctly, at this point I’d be in favor of removing these functions entirely and perhaps replace them with UniqueArc / UniqueRc types that would represent "Arc/Rc with strong_count == 1 && weak_count == 0", implement DerefMut, and have zero-cost conversion to Arc/Rc

[Mark-Simulacrum]

@bors r+ rollup

This seems OK and we have some sign-offs from other folks, we can iterate on other designs (e.g., UniueArc/Rc) later.

[bors]

📌 Commit 734f724 has been approved by Mark-Simulacrum

It is now in the queue for this repository.