stabilize {Rc, Arc}::is_unique

[malezjaa]

View all comments

Stabilization report:

Closes #138938

{Arc,Rc}::is_unique, is a function that allows determining whether there is exactly one strong count and zero weak counts to an Arc/Rc.

Implementation history

Implementation: #138939

Public api

// alloc::rc
impl<T> Rc<T> {
    pub fn is_unique(this: &Self) -> bool;
}

// alloc::sync
impl<T> Arc<T> {
    pub fn is_unique(this: &Self) -> bool;
}

@rustbot modify labels: +T-libs

This is a pretty small feature, so opening this to hopefully get a FCP started.

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[malezjaa]

r? libs

[clarfonthey]

It was already assigned to libs, but, sure.

Is there a reason why this API isn't offered on Rc too? That seems like an important detail to determine before stabilisation.

[malezjaa]

I don't think there was any. The ACP only proposed it for Arc, and Rc::is_unique was proposed by @tgross35 in the tracking issue, but never really implemented.

I can add it, but I'm not sure if it belongs in this PR.

[clarfonthey]

I mean, I don't like the precedent of stabilising asymmetric impls on Arc that could go on Rc too, which is maybe a sign we dont want to stabilise this yet.

[malezjaa]

I assume that Rc variant should be on its own tracking issue and ACP?

[clarfonthey]

I would say that unless there's significant reason to make things different on Arc and Rc then new features should apply to both. We don't want to stabilise separately.

[clarfonthey]

Once #163489 merges and this is rebased on top of that, I think it's okay to FCP merge, since Rc's functionality already existed, just wasn't public. If you could update the PR description also, that would help.

Thank you!

[clarfonthey]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[malezjaa]

@rustbot ready

Rebased and updated :D

[clarfonthey]

@rfcbot merge libs

[rust-rfcbot]

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

• @ChrisDenton
• @Mark-Simulacrum
• @cuviper
• @dtolnay
• @joshtriplett
• @nia-e
• @the8472
• @thomcc

No concerns currently listed.

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!

cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[2ndDerivative]

Can I suggest adding a comment for the obvious time-of-check to time-of-use bug to use the option in ::get_mut directly instead (or similar to the comment on strong_count)? I know for people dealing a lot with concurrency this should be obvious but yeah.

Or maybe one can do that in a clippy lint

[clarfonthey]

The point of FCP is for everyone to comment, not just libs members, so, you absolutely can make that suggestion. We do specifically take care to make sure the atomics will work appropriately, but yes, that should be mentioned.

Note that regardless, thread safety will be upheld, but there are cases where allocating is less than desired.

(Also, I think we can just make this a prereq for merging, not block the FCP, but would be happy if anyone wants to propose a change in this PR or elsewhere.)

[cuviper]

In cases like the doc suggestion that is_unique may justify get_mut_unchecked, the latter requires &mut Arc. So you can reason locally that no other clone is created between that TOCTOU check and use.

[clarfonthey]

Yeah, I guess it does clarify that "no clones occur in between" and that is effectively pointing out the TOCTOU issue.

So maybe the docs are okay as-is, but if anyone has a concrete suggestion, that seems reasonable. We also just did a release so we have time after this lands on nightly to fix docs before it ends up in a release.

[tgross35]

The last example does use *const i32 rather than get_mut_unchecked, which does have the clone in between. For a concrete suggestion, perhaps:

... Thus, the following code is valid:

[code]

Either of the following changes would make the example unsound:

* Using `get_mut` to create a mutable reference which is kept around while reading via `pointer`
* Cloning or dropping `arc` between checking `is_unique` and accessing `pointer`

(thought I had a third bullet point but now I can't remember what)

So maybe the docs are okay as-is, but if anyone has a concrete suggestion, that seems reasonable. We also just did a release so we have time after this lands on nightly to fix docs before it ends up in a release.

I think it's pretty much always worth making sure the docs are in a state we're happy with before actually pressing the merge button on stabilization. Once the "this must be done to get X feature I want" motivation goes away, the number of people actually interested in doing such a docs update gets a lot closer to 0 :) and there's typically no need to rush the stabilization PR.