Skip to content

UnsafePinned: update get() docs and signature to allow shared mutation #142162

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 9, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 7, 2025

Follow-up to #140638, making get consistent with the fact that there's an UnsafeCell inside this type now by returning *mut T instead of *const T.

Cc @rust-lang/libs-api
Tracking issue: #125735

@rustbot
Copy link
Collaborator

rustbot commented Jun 7, 2025

r? @workingjubilee

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

@rustbot rustbot 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 Jun 7, 2025
Comment on lines 91 to 94
/// This can be cast to a pointer of any kind.
/// Ensure that the access is unique (no active references, mutable or not)
/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. This would be better, I'd suggest, mechanically wrapped.

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer semantic linebreaks, since the primary way this will be read is in the actual compiled documentation form and so the internal format of our docs is best-off organized around tracking history.

I don't usually quibble much over it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use auto-wrapping almost everywhere, and semantic wrapping doesn't go well with out 100 column limit, so I think I'll re-wrap this.

Comment on lines 93 to 94
/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest wording here along the lines of "...ensure that there are no ongoing mutations or live mutable references when casting to &T."

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW this paragraph is copied verbatim from UnsafeCell::get. I was hoping we could just reuse it without spending a lot of time on editing.^^

Comment on lines 92 to 93
/// Ensure that the access is unique (no active references, mutable or not)
/// when casting to `&mut T`, and ensure that there are no mutations
Copy link
Contributor

@traviscross traviscross Jun 7, 2025

Choose a reason for hiding this comment

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

Since the second half of this, about casting to &T, makes note about ensuring there are no ongoing foreign writes (i.e. even if no active references exist), perhaps the first half should as well, since I'd expect casting to &mut T to have this same precondition.

/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`.
///
/// All the usual caveats around mutation shared state apply, see [`UnsafeCell`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// All the usual caveats around mutation shared state apply, see [`UnsafeCell`].
/// All the usual caveats around mutation of shared state apply, see [`UnsafeCell`].

Comment on lines 91 to 94
/// This can be cast to a pointer of any kind.
/// Ensure that the access is unique (no active references, mutable or not)
/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can apply the deduplicating technique of #141609 or if it's even worth it here? Probably not, it's just a few lines.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 7, 2025

I've replaced the paragraph in question with a reference to the UnsafeCell docs, which already have a much better discussion of the aliasing rules.

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 8, 2025 via email

@bors
Copy link
Collaborator

bors commented Jun 8, 2025

📌 Commit bd0a81e has been approved by traviscross

It is now in the queue for this repository.

@bors 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 Jun 8, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 8, 2025
…iscross

UnsafePinned: update get() docs and signature to allow shared mutation

Follow-up to rust-lang#140638, making `get` consistent with the fact that there's an `UnsafeCell` inside this type now by returning `*mut T` instead of `*const T`.

Cc `@rust-lang/libs-api`
Tracking issue: rust-lang#125735
bors added a commit that referenced this pull request Jun 8, 2025
Rollup of 11 pull requests

Successful merges:

 - #140774 (Affirm `-Cforce-frame-pointers=off` does not override)
 - #141001 (Make NonZero<char> possible)
 - #141700 (Atomic intrinsics : use const generic ordering, part 2)
 - #142008 (const-eval error: always say in which item the error occurred)
 - #142053 (Add new Tier-3 targets: `loongarch32-unknown-none*`)
 - #142089 (Replace all uses of sysroot_candidates with get_or_default_sysroot)
 - #142108 (compiler: Add track_caller to AbiMapping::unwrap)
 - #142132 (`tests/ui`: A New Order [6/N])
 - #142162 (UnsafePinned: update get() docs and signature to allow shared mutation)
 - #142171 (`tests/ui`: A New Order [7/N])
 - #142179 (store `target.min_global_align` as an `Align`)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

?
@bors r-
@bors r=workingjubilee,traviscross

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 8, 2025
@bors
Copy link
Collaborator

bors commented Jun 8, 2025

📌 Commit bd0a81e has been approved by workingjubilee,traviscross

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 8, 2025
…ingjubilee,traviscross

UnsafePinned: update get() docs and signature to allow shared mutation

Follow-up to rust-lang#140638, making `get` consistent with the fact that there's an `UnsafeCell` inside this type now by returning `*mut T` instead of `*const T`.

Cc `@rust-lang/libs-api`
Tracking issue: rust-lang#125735
bors added a commit that referenced this pull request Jun 9, 2025
Rollup of 12 pull requests

Successful merges:

 - #141803 (Remove rustc's notion of "preferred" alignment AKA `__alignof`)
 - #142053 (Add new Tier-3 targets: `loongarch32-unknown-none*`)
 - #142089 (Replace all uses of sysroot_candidates with get_or_default_sysroot)
 - #142108 (compiler: Add track_caller to AbiMapping::unwrap)
 - #142132 (`tests/ui`: A New Order [6/N])
 - #142162 (UnsafePinned: update get() docs and signature to allow shared mutation)
 - #142171 (`tests/ui`: A New Order [7/N])
 - #142179 (store `target.min_global_align` as an `Align`)
 - #142183 (Added test for 30904)
 - #142194 (Remove all unused feature gates from the compiler)
 - #142199 (Do not free disk space in the `mingw-check-tidy` job)
 - #142210 (Run `mingw-check-tidy` on auto builds)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit faab021 into rust-lang:master Jun 9, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 9, 2025
rust-timer added a commit that referenced this pull request Jun 9, 2025
Rollup merge of #142162 - RalfJung:unsafe-pinned-get, r=workingjubilee,traviscross

UnsafePinned: update get() docs and signature to allow shared mutation

Follow-up to #140638, making `get` consistent with the fact that there's an `UnsafeCell` inside this type now by returning `*mut T` instead of `*const T`.

Cc ``@rust-lang/libs-api``
Tracking issue: #125735
@RalfJung RalfJung deleted the unsafe-pinned-get branch June 9, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant 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.

5 participants