Skip to content

add CloneFromCell and Cell::get_cloned - #145685

Merged
bors merged 1 commit into
rust-lang:masterfrom
Qelxiros:cell_get_cloned
Oct 4, 2025
Merged

add CloneFromCell and Cell::get_cloned#145685
bors merged 1 commit into
rust-lang:masterfrom
Qelxiros:cell_get_cloned

Conversation

@Qelxiros

@QelxirosQelxiros commented Aug 20, 2025

Copy link
Copy Markdown
Contributor

Tracking issue: #145329

@rustbot

Copy link
Copy Markdown
Collaborator

r? @tgross35

rustbot has assigned @tgross35.
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

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2025
Comment threadlibrary/alloc/src/rc.rs Outdated
Comment threadlibrary/core/src/cell.rs Outdated
Comment threadlibrary/core/src/cell.rs Outdated
Comment threadlibrary/core/src/cell.rs Outdated
Comment threadlibrary/core/src/cell.rs Outdated
@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2025
@QelxirosQelxiros changed the title add CopyFromClone and Cell::get_clonedadd CloneFromCopy and Cell::get_clonedAug 27, 2025
@rust-log-analyzer

This comment has been minimized.

@Qelxiros

Copy link
Copy Markdown
ContributorAuthor

@rustbot ready (unless the CI fails)

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 3, 2025
Comment threadlibrary/core/src/cell.rs Outdated
Comment on lines +716 to +724
/// Types that can be cloned from a copy of the value.
///
/// # Safety
///
/// Implementing this trait is safe for any type that can be safely bitwise copied.
#[unstable(feature = "cell_get_cloned", issue = "145329")]
// Allow potential overlapping implementations in user code
#[marker]
pub unsafe trait CloneFromCopy: Clone {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs and safety comment don't quite cover it; moves are bitwise copies, and of course that can be cloned as well as the original. Copy really just means that a bitwise copy doesn't invalidate the original. I'm not sure what the exact preconditions are here though, in most cases isn't this just something like Freeze + Unpin?

I'm sure I am missing something, cc @programmerjake

@programmerjakeprogrammerjakeSep 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea is that if you have &Cell<T> it's unsound to call T::clone on that reference for generic T since it could contain references to the same &Cell<T> which allows it to modify T while a &T was passed to clone. So, instead, we do a bitwise copy of T to a another memory location that isn't aliased and call T::clone on that, making sure to forget the bitwise copy using something like ManuallyDrop. not all types are safe to just outright bitwise copy while the original is still valid (e.g. doing a bitwise copy on T = Cell<Option<String>> allows you to drop the String twice by taking it from the copy and from the original Cell<Cell<Option<String>>>).

@programmerjakeprogrammerjakeSep 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T: CloneFromCopy means the following code is sound:

pubfnclone_from_copy<T:CloneFromCopy>(v:&Cell<T>) -> T{let copy = unsafe{ManuallyDrop::new(ptr::read(v).into_inner())};T::clone(&copy)}

@tgross35

Copy link
Copy Markdown
Member

To put the above into preconditions,

@rustbot author

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2025
@rustbot

Copy link
Copy Markdown
Collaborator

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

@Qelxiros

Copy link
Copy Markdown
ContributorAuthor

@rustbot ready

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2025
Comment threadlibrary/core/src/cell.rs Outdated
///
/// # Safety
///
/// Implementing this trait is safe for any type that can be safely bitwise copied while the

@programmerjakeprogrammerjakeSep 8, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I wasn't very clear before: it's important to not drop the bitwise copy, otherwise it's not safe to impl for Rc and other reference-counted types:

fnf(v:&Cell<Rc<u8>>) -> Rc<u8>whereRc<u8>:CloneFromCopy,{// Safety: this is unsound, but CloneFromCopy's current wording implies this is fine:let copy = unsafe{ ptr::read(v.as_ptr())};let retval = copy.clone();drop(copy);
retval
}fncause_use_after_free(){let a = Cell::new(Rc::new(0u8));let b = f(&a);// f clones once and drops once so reference count is 1drop(a);// reference count is now 0, backing memory is freeddbg!(b);// use after free, this is UB}

@tgross35

tgross35 commented Sep 8, 2025

Copy link
Copy Markdown
Member

(I'll effectively delegate this review to Jake)

@rustbot author

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2025
@Qelxiros

Copy link
Copy Markdown
ContributorAuthor

@rustbot ready

@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 12, 2025
@QelxirosQelxiros changed the title add CloneFromCopy and Cell::get_clonedadd CloneFromCell and Cell::get_clonedOct 1, 2025
@tgross35

Copy link
Copy Markdown
Member

Thanks Jacob for reviewing.

@bors r=programmerjake,tgross35 rollup

@bors

bors commented Oct 3, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit 08b4641 has been approved by programmerjake,tgross35

It is now in the queue for this repository.

@borsbors 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 Oct 3, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
@ZalatharZalathar mentioned this pull request Oct 4, 2025
bors added a commit that referenced this pull request Oct 4, 2025
Rollup of 14 pull requests
Successful merges:
- #142670 (Document fully-qualified syntax in `as`' keyword doc)
- #144908 (Fix doctest output json)
- #145685 (add CloneFromCell and Cell::get_cloned)
- #146330 (Bump unicode_data and printables to version 17.0.0)
- #146451 (Fix atan2 inaccuracy in documentation)
- #146479 (add mem::conjure_zst)
- #147190 (std: `sys::net` cleanups)
- #147245 (only replace the intended comma in pattern suggestions)
- #147251 (Do not assert that a change in global cache only happens when concurrent)
- #147269 (Add regression test for 123953)
- #147277 (Extract common logic for iterating over features)
- #147280 (Return to needs-llvm-components being info-only)
- #147292 (Respect `-Z` unstable options in `rustdoc --test`)
- #147300 (Add xtensa arch to object file creation)
r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
@ZalatharZalathar mentioned this pull request Oct 4, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
@ZalatharZalathar mentioned this pull request Oct 4, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
@ZalatharZalathar mentioned this pull request Oct 4, 2025
bors added a commit that referenced this pull request Oct 4, 2025
Rollup of 14 pull requests
Successful merges:
- #142670 (Document fully-qualified syntax in `as`' keyword doc)
- #145685 (add CloneFromCell and Cell::get_cloned)
- #146330 (Bump unicode_data and printables to version 17.0.0)
- #146451 (Fix atan2 inaccuracy in documentation)
- #146479 (add mem::conjure_zst)
- #146874 (compiler: Hint at multiple crate versions if trait impl is for wrong ADT )
- #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos)
- #147190 (std: `sys::net` cleanups)
- #147251 (Do not assert that a change in global cache only happens when concurrent)
- #147280 (Return to needs-llvm-components being info-only)
- #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting)
- #147309 (Add documentation about unwinding to wasm targets)
- #147315 (bless autodiff batching test)
- #147323 (Fix top level ui tests check in tidy)
r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
bors added a commit that referenced this pull request Oct 4, 2025
Rollup of 11 pull requests
Successful merges:
- #142670 (Document fully-qualified syntax in `as`' keyword doc)
- #145685 (add CloneFromCell and Cell::get_cloned)
- #146330 (Bump unicode_data and printables to version 17.0.0)
- #146451 (Fix atan2 inaccuracy in documentation)
- #146479 (add mem::conjure_zst)
- #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos)
- #147190 (std: `sys::net` cleanups)
- #147251 (Do not assert that a change in global cache only happens when concurrent)
- #147280 (Return to needs-llvm-components being info-only)
- #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting)
- #147315 (bless autodiff batching test)
r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 4, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
bors added a commit that referenced this pull request Oct 4, 2025
Rollup of 10 pull requests
Successful merges:
- #142670 (Document fully-qualified syntax in `as`' keyword doc)
- #145685 (add CloneFromCell and Cell::get_cloned)
- #146330 (Bump unicode_data and printables to version 17.0.0)
- #146451 (Fix atan2 inaccuracy in documentation)
- #146479 (add mem::conjure_zst)
- #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos)
- #147190 (std: `sys::net` cleanups)
- #147251 (Do not assert that a change in global cache only happens when concurrent)
- #147280 (Return to needs-llvm-components being info-only)
- #147315 (bless autodiff batching test)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 47bd38c into rust-lang:masterOct 4, 2025
10 checks passed
@rustbotrustbot added this to the 1.92.0 milestone Oct 4, 2025
rust-timer added a commit that referenced this pull request Oct 4, 2025
Rollup merge of #145685 - Qelxiros:cell_get_cloned, r=programmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: #145329
github-actionsBot pushed a commit to model-checking/verify-rust-std that referenced this pull request Oct 9, 2025
…mmerjake,tgross35
add CloneFromCell and Cell::get_cloned
Tracking issue: rust-lang#145329
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Qelxiros@rustbot@rust-log-analyzer@tgross35@programmerjake@ais523@bors