Stabilize arc_new_cyclic

[bdbai]

This stabilizes feature arc_new_cyclic as the implementation has been merged for one year and there is no unresolved questions. The FCP is not started yet.

Closes #75861 .

@rustbot label +T-libs-api

[rust-highfive]

r? @dtolnay

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

[dtolnay]

I like these!

@rust-lang/libs-api
@rfcbot fcp merge

impl<T> Rc<T> {
    pub fn new_cyclic(f: impl FnOnce(&Weak<T>) -> T) -> Rc<T>;
}

impl<T> Arc<T> {
    pub fn new_cyclic(f: impl FnOnce(&Weak<T>) -> T) -> Arc<T>;
}

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• clarify doc example resolved by Stabilize arc_new_cyclic #90666 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

I think the docs for this API leave a lot to be desired. I can't figure out what this API is really supposed to do or, more importantly, why or when I should (or shouldn't) use it. Here are some points of confusion that I have after looking at this for a few minutes:

• The name of the API includes the word "cyclic," but nothing in the docs mention it.
• I'm grateful for the existence of the example, but the example doesn't really help me understand what this routine does or why I would want to use it. It kind of seems like a "trivial" example to me, which is fine in some cases, but this API feels like it deserves something better.
• The docs mention a None value, but I don't quite grok this. I don't see any Option<T> anywhere, so it's not clear to me where this None value comes from.
• What happens if data_fn panics? Does a leak occur? Something worse? Should we specify the behavior of this routine if data_fn panics?

My suspicion is that this is a fine API to add, but I'd like to see the docs improved here a bit before signing off.

[yaahc]

• The docs mention a None value, but I don't quite grok this. I don't see any Option<T> anywhere, so it's not clear to me where this None value comes from.

It looks like the None is referring to calling upgrade on the Weak argument inside of the provided closure. Seems easy to clarify by turning the upgrade reference into a link to the relevant function on Weak.

[yaahc]

• I'm grateful for the existence of the example, but the example doesn't really help me understand what this routine does or why I would want to use it. It kind of seems like a "trivial" example to me, which is fine in some cases, but this API feels like it deserves something better.

I did some digging in the history of this API to see if I couldn't find any better explanations and found this comment which explicitly mentions being able to give out reference counted pointers to ones self. Perhaps the example just needs to additionally include a simple method on Foo, naming notwithstanding.

use std::sync::{Arc, Weak};

struct Foo {
    me: Weak<Foo>,
}

impl Foo {
    /// Construct a reference counted Foo.
    fn new() -> Arc<Self> {
        Arc::new_cyclic(|me| Foo {
            me: me.clone(),
        })
    }

    /// Return a reference counted pointer to Self.
    fn me(&self) -> Arc<Self> {
        self.me.upgrade()
    }
}

[yaahc]

@rfcbot reviewed
@rfcbot concern clarify doc example

[ogoffart]

That's similar to the current docs for Rc::new_cyclic: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.new_cyclic

[yaahc]

Fair point, we should definitely also make sure all the docs on Rc::new_cyclic and Arc::new_cyclic are consistent.

The necessary changes as I see them:

• Update Arc::new_cyclic docs to add the methods mentioned above
• Rename the Foo type in the Arc example to Gadget
• Add the second method from the methods mentioned above to the Rc example
• Update the description for both Arc and Rc so that the mention of upgrade becomes a link to Weak::upgrade

[bdbai]

Regarding the doc example, I would like to propose the following alternative:

use std::sync::{Arc, Weak};

struct Root {
    left: Leaf,
    right: Leaf,
}

struct Leaf {
    root: Weak<Root>,
}

let tree = Arc::new_cyclic(|me| {
    assert!(root.upgrade().is_none());
    Root {
        left: Leaf { root: me.clone() },
        right: Leaf { root: me.clone() },
    }
});

assert!(tree.left.root.upgrade().is_some());
assert!(Arc::ptr_eq(&tree.right.root.upgrade().unwrap(), &tree));

I noticed there was a section Breaking cycles with Weak in the documentation of Arc<T>, where a tree-like data structure was introduced as an example. I guess the doc example here is the right place to reflect that.

[bdbai]

The description was amended as follows:

/// Constructs a new Arc<T> using a closure data_fn that has access to
/// a weak reference to the constructing Arc<T>.
///
/// Generally, a structure circularly referencing itself, either directly or
/// indirectly, should not hold a strong reference to prevent a memory leak.
/// In data_fn, initialization of T can make use of the weak reference
/// by cloning and storing it inside T for use at a later time.
///
/// Since the new Arc<T> is not fully-constructed until
/// Arc<T>::new_cyclic returns, calling [upgrade] on the weak
/// reference inside data_fn will fail and result in a None value.
///
/// # Panics
/// If data_fn panics, the panic is propagated to the caller, and the
/// temporary [Weak<T>] is dropped normally.
///
/// # Example
/// ...
/// [upgrade]: Weak::upgrade

Since I am not a native English speaker, I would appreciate it if any suggestions could be made in order to make readers feel more idiomatic. 😊

[yaahc]

@bdbai I edited a couple of the verb tenses in your updated doc comment but otherwise everything in your example looked correct grammatically. Am I correct in understanding that you've not made the mentioned changes in the PR itself?

As for the example, I have a slight preference for the example that I suggested earlier since it seems closer to real world examples while still being relatively brief, where as I feel like the tree example you linked is a simplified form of what a real world tree structure would look like and is still a much longer example. That said, I'm fine with either example, so I'll leave it up to other libs team members to voice a stronger preference.

I would still like to see the Rc and Arc docs updated to match regardless of which example we go with.

[bdbai]

@yaahc

I edited a couple of the verb tenses in your updated doc comment but otherwise everything in your example looked correct grammatically.

Thanks!

Am I correct in understanding that you've not made the mentioned changes in the PR itself?

I failed to mention that the changes of the text description would be committed into the PR once it got corrected and approved. Sorry about that.

I would still like to see the Rc and Arc docs updated to match regardless of which example we go with.

Sure. I will update both while changing the codebase. For the sake of brevity, I would like to mention only one of them during discussion since the both does not differ in terms of this API.

[bdbai]

@yaahc The documentation in Arc::new_cyclic and Rc::new_cyclic has been updated.

btw why did rustbot add the T-compiler tag?

[yaahc]

@bdbai not sure what's up with the T-compiler label but the updates look great!

@rfcbot resolve clarify doc example

[bdbai]

@veber-alex fixed.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[wx-csy]

Can't wait for the stabilization to build data structures with circular references!

Currently, this function takes a closure to initialize the arc, but it operates badly with async programming. I think we can offer an API that returns a pair of (Initializer, Weak<T>), where the non-clonable Initializer can be used to initialize the arc and after that, the weak pointer can be promoted to a strong pointer. In this case, I can pass the initializer to an async block then do the initialization asynchronously.

[bdbai]

@wx-csy good point!
I suggest this feature may be generalized as ArcBuilder (or TempArc?) with the following API interface:

struct ArcBuilder<T> { ... }
impl<T> ArcBuilder<T> {
    fn new() -> Self;
    fn get_weak(&self) -> &Weak<T>;
    fn with_value(self, value: T) -> Arc<T>;
}

Once implemented, it will also address the need of fallible cyclic Arc creation mentioned here. Furthermore, interoperability with MaybeUninit can be added to it.

Since the pair of new_cyclic functions have been in the standard library for a while, reworking this feature may introduce breaking changes. @yaahc what do you think?

[m-ou-se]

That sounds like a discussion to be had on the tracking issue, before stabilizing this: #75861

[wx-csy]

@wx-csy good point! I suggest this feature may be generalized as ArcBuilder (or TempArc?) with the following API interface:

struct ArcBuilder<T> { ... }
impl<T> ArcBuilder<T> {
    fn new() -> Self;
    fn get_weak(&self) -> &Weak<T>;
    fn with_value(self, value: T) -> Arc<T>;
}

Once implemented, it will also address the need of fallible cyclic Arc creation mentioned here. Furthermore, interoperability with MaybeUninit can be added to it.

Since the pair of new_cyclic functions have been in the standard library for a while, reworking this feature may introduce breaking changes. @yaahc what do you think?

Well, I think it's fine to move forward stabilizing new-cyclic and just add ArcBuilder later. After that, we may mark new-cyclic as a shorthand for ArcBuilder.

Btw, we may additionally return an ArcBuilder when unwrapping the last strong pointer (e.g., in Arc::try_unwrap), so that we may reinitialize the arc within exactly the same allocation, though I've not yet found a real-world use case of this :)

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[m-ou-se]

Well, I think it's fine to move forward stabilizing new-cyclic and just add ArcBuilder later. After that, we may mark new-cyclic as a shorthand for ArcBuilder.

Sounds good.

Note that this ArcBuilder idea has some overlap with the ArcMut idea mentioned here: #63291 (comment)

[m-ou-se]

This will be merged soon.

@bors r+

[bors]

📌 Commit 00e191c has been approved by m-ou-se