Add Arc::new_with() function for constructing self-referential data structures by Diggsey · Pull Request #72443 · rust-lang/rust (original) (raw)

I started reviewing with a few nits (and a potential unsoundness with atomic ordering), but then, after thinking about it, I've realised that the whole Weak-as-input API is hard to make sound given how Weak operates: anybody can call .upgrade() on a Weak or a .clone() of it, which will call .inner().

And currently, .inner() does promote the NonNull<ArcInner<T>> to a &ArcInner<T>, thus asserting both:

• non-aliasing of data: T (c.f., @Diggsey post above),
• and in the future, that data is initialized. This incurs in the maintenance burden of having to be update the code if Rust "starts exploiting" that validity of the pointee is part of the validity invariant of a Rust reference (which it conservatively "officially" does: the very fact this currently isn't be a problem here is that the stdlib gets to know what isn't "exploited UB" yet).

So, instead, I suggest that, before this PR goes forward, Weak's .inner() be changed to return an Option<&'_ ArcInner<UnsafeCell<MaybeUninit<T>>> (or an Option<&'_ ArcInner<()>> with separate unsafe fns &data accessors)

• Otherwise, the whole new_with does indeed require infecting everything with UnsafeCells, and would be relying on Rust references being allowed to refer to uninitialized data 😬

This may look cumbersome, but it will lead to the rest of Arc's API to be at peace with Weaks being, well, a very weak pointer type, that only gets to assert anything about its data if and only if strong ≥ 1.

Then, we could slightly simplify and thus improve new_with, with something along these lines:

/// Constructs a new `Arc<T>` out of a closure that gets to have
/// `Weak` "back"-pointer to the to-be-returned `Arc`.
///
/// This is useful to create immutable graph-like structures, since
/// those may require back pointers.
///
/// Since the closure runs before such `Arc` is fully constructed,
/// the obtained `Weak` "dangles" while the closure is running:
/// any attempts to upgrade the `Weak` at that point will fail.
#[inline]
#[unstable(feature = "arc_new_with", issue = "none")]
pub fn new_with(data_fn: impl FnOnce(&'_ Weak<T>) -> T) -> Arc<T> {
    let weak = Weak {
        ptr: Box::into_raw_nonnull(box ArcInner {
            strong: 0.into(),
            weak: 1.into(),
            data: UnsafeCell::new(mem::MaybeUninit::<T>::uninit()),
        }).cast(),
    };
    let data = data_fn(&weak);
    let inner: &'_ ArcInner<UnsafeCell<MaybeUninit<T>>> = weak.inner().unwrap();
    unsafe {
        inner
            .data
            .get() // exclusive access is guaranteed by `strong == 0`
            .write(MaybeUninit::new(data))
        ;
        // `data` has been initialized and is no longer (directly) mutated
        // so we can finally allow other `Weak`s to be upgraded.
        inner.strong.store(1, atomic::Ordering::Release);
        // "transmute"-upgrade the `Weak` to an `Arc`
        Arc::from_inner(ManuallyDrop::new(weak).ptr)
    }
}