Stabilize arc_new_cyclic by bdbai · Pull Request #90666 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation31 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

rustbot added the T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

label

Nov 7, 2021

r? @dtolnay

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

I like these!

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

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

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

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.

• 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.

• 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, }

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

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

}

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

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, }

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.

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. 😊

@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.

@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.

This comment has been minimized.

rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Dec 13, 2021

This comment has been minimized.

This comment has been minimized.

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

btw why did rustbot add the T-compiler tag?

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

@rfcbot resolve clarify doc example

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

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.

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

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

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?

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

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

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

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 :)

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.

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)

This will be merged soon.

@bors r+

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

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jan 22, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 22, 2022

Stabilize arc_new_cyclic

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 rust-lang#75861 .

@rustbot label +T-libs-api

This was referenced

Jan 22, 2022

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 23, 2022

Labels

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.