Add alloc::rc::UniqueRc

[eholk]

This PR implements UniqueRc as described in rust-lang/libs-team#90.

I've tried to stick to the API proposed there, incorporating the feedback from the ACP review. For now I've just implemented UniqueRc, but we'll want UniqueArc as well. I wanted to get feedback on this implementation first since the UniqueArc version should be mostly a copy/paste/rename job.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[eholk]

@rustbot author

There are a few issues that need fixed before this is ready for review.

[programmerjake]

to me, something named UniqueRc<T> should contain a T and implement DerefMut<Target = T> and basically be Rc but with the restriction that strong_count == 1 and Weak::upgrade doesn't yet work...

what you have here seems more like a UninitalizedRc or EmptyRc where it allocated space for a RcBox but didn't fill it in yet.

[joboet]

to me, something named UniqueRc<T> should contain a T and implement DerefMut<Target = T> and basically be Rc but with the restriction that strong_count == 1 and Weak::upgrade doesn't yet work...

what you have here seems more like a UninitalizedRc or EmptyRc where it allocated space for a RcBox but didn't fill it in yet.

With the UniqueRc<T> you propose, EmptyRc could be easily emulated by using UniqueRc<MaybeUninit<T>> and adding a write method to initialize it.

[m-ou-se]

to me, something named UniqueRc<T> should contain a T and implement DerefMut<Target = T> and basically be Rc but with the restriction that strong_count == 1 and Weak::upgrade doesn't yet work...

That's indeed what I meant in rust-lang/libs-team#90 (comment)

[eholk]

Ah, I see what you mean. I misinterpreted the original suggestion.

UniqueRc didn't seem like the right name for what I've implemented. While I was writing the documentation, something like PlaceholderRc<T> felt like a better name.

So with UniqueRc, creating a cyclic structure would look more like this, right?

struct Cyclic {
    this: Weak<Cyclic>,
}

let mut cycle = UniqueRc::new(Cyclic { this: Weak::new() };
cycle.this = UniqueRc::weak(cycle);

// convert to a normal `Rc`
let rc = UniqueRc::into_rc(cycle);

I'm curious why the Libs API team prefers this approach? I personally find something like PlaceholderRc or UninitializedRc more ergonomic, but I admit this is a matter of preference and I am likely overlooking other concerns around consistency with the rest of std, etc.

One nice property I see for PlaceholderRc is that it's pretty straightforward to implement Rc::new_cyclic in terms of PlaceholderRc. It's less obvious how to do this with UniqueRc, although @joboet's trick of using UniqueRc<MaybeUninit<T>> should work.

I appreciate the discussion here! One thing this has pointed out to me is that it's actually possible to implement this without depending on any internal details of Rc, so it could be done in an external crate (although it would require unsafe code, of course). In our usage of this, we had basically written our own RcBox and transmuted the contents of the Rc into that, which worked but was incredibly fragile. Given the realization we can do this without depending on internal details, I wonder if it makes more sense to try this as an external crate first in the interest of not growing std more than we need to?

[programmerjake]

for UniqueRc at least, it does require modifying Rc's internals in order to prevent Weak::upgrade from working until the UniqueRc has been converted into an Rc, otherwise you could have both shared and mutable references to T which is UB.

[eholk]

for UniqueRc at least, it does require modifying Rc's internals in order to prevent Weak::upgrade from working until the UniqueRc has been converted into an Rc

Oh, good point. Thanks for pointing that out!

[eholk]

I updated the PR to implement UniqueRc<T> as a thing that always holds an initialized T and provides a DerefMut impl so you can freely mutate its contents before converting it to an Rc.

I also changed all the functions to not have a self, since the pattern for Rc and such is to avoid adding new methods that could conflict with methods on the underlying T.

Finally, I renamed UniqueRc::weak to UniqueRc::downgrade for better symmetry with Rc.

[safinaskar]

I updated the PR to implement UniqueRc<T> as a thing that always holds an initialized T

I like your previous approach more. It is simpler to use

[programmerjake]

I updated the PR to implement UniqueRc<T> as a thing that always holds an initialized T

I like your previous approach more. It is simpler to use

imho both APIs are useful, both the EmptyRc (like in Rc::new_cyclic) and UniqueRc that implements DerefMut<Target = T> -- they have different use-cases, so for each use-case one and/or the other or neither is best suited for that use-case.

[eholk]

imho both APIs are useful, both the EmptyRc (like in Rc::new_cyclic) and UniqueRc that implements DerefMut<Target = T> -- they have different use-cases, so for each use-case one and/or the other or neither is best suited for that use-case.

EmptyRc seems like a good name for it. The nice thing is the two modes are pretty similar (you can implement EmptyRc as UniqueRc<MaybeUninit<T>>, so it doesn't seem like a big deal to include both.

[eholk]

I think this is ready now. It's tempting to add EmptyRc too, but I think it's better to save that for a later PR. This PR focuses on implementing UniqueRc as was suggested by the libs API team.

@rustbot ready

[Mark-Simulacrum]

Implementation looks fine to me. I think the ACP looks like it was positively received, so I'm inclined to say we should file a tracking issue and I can approve this PR -- the naming of the type feels unfortunate as mentioned in my comment, but I wouldn't block this over that.

[eholk]

I opened #112566 as the tracking issue for this feature.

[eholk]

@rustbot ready

[riking]

Please check API consistency with the Rust-for-Linux UniqueArc https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/sync/arc.rs#L502-L578

[programmerjake]

Please check API consistency with the Rust-for-Linux UniqueArc https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/sync/arc.rs#L502-L578

imho API consistency with Rc<T> should take priority over Linux's UniqueArc where there's a conflict.

[Mark-Simulacrum]

@rustbot author

r=me with commits squashed

[Mark-Simulacrum]

I think we can revise API as needed in future PRs since this is unstable (e.g., for renaming or for kernel API consistencies).

[eholk]

Please check API consistency with the Rust-for-Linux UniqueArc https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/sync/arc.rs#L502-L578

I added this under open questions in the tracking issue (#112566). Thanks for pointing this out to me, it's good to know of another use case for something like this.

[eholk]

@bors r=@Mark-Simulacrum

[bors]

📌 Commit 53003cd has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 53003cd with merge 14803bd...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 14803bd to master...

[rust-timer]

Finished benchmarking commit (14803bd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-3.4%, -1.0%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.9%	[-3.4%, -1.0%]	10

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.668s -> 655.501s (-0.33%)

[RalfJung]

Why does create_gadget return an Option?

[eholk]

The example evolved a bit from my first version of it. Originally I wanted to show that you could use this to initialize a circular data structure while the initialization phase is fallible, which is something that the existing new_cyclic function doesn't handle well. Unfortunately, there's no fallibility left in this example so the Option is probably more of a distraction.