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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bafe406
UnsafePinned: update get() docs and signature to allow shared mutation
RalfJung Jun 7, 2025
bd0a81e
centralize aliasing rules discussion in UnsafeCell docs
RalfJung Jun 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/core/src/cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2170,7 +2170,7 @@ impl<T: ?Sized> UnsafeCell<T> {
/// 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`
/// or mutable aliases going on when casting to `&T`.
///
/// # Examples
///
Expand Down
19 changes: 11 additions & 8 deletions library/core/src/pin/unsafe_pinned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,14 @@ impl<T: ?Sized> UnsafePinned<T> {
ptr::from_mut(self) as *mut T
}

/// Get read-only access to the contents of a shared `UnsafePinned`.
/// Get mutable access to the contents of a shared `UnsafePinned`.
///
/// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
/// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
/// [`UnsafeCell`] if you also need interior mutability.
/// 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
Copy link
Contributor

@traviscross traviscross Jun 7, 2025 •
edited
Loading

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.

/// or mutable aliases going on when casting to `&T`.
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.

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

Copy link
Member

@workingjubilee workingjubilee Jun 7, 2025

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

@RalfJung RalfJung Jun 7, 2025

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.

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.

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

@RalfJung RalfJung Jun 7, 2025

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

Copy link
Member

@workingjubilee workingjubilee Jun 7, 2025

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.

///
/// All the usual caveats around mutation shared state apply, see [`UnsafeCell`].
Copy link
Member

@workingjubilee workingjubilee Jun 7, 2025

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`].

///
/// [`UnsafeCell`]: crate::cell::UnsafeCell
///
Expand All @@ -100,16 +103,16 @@ impl<T: ?Sized> UnsafePinned<T> {
///
/// unsafe {
/// let mut x = UnsafePinned::new(0);
/// let ptr = x.get(); // read-only pointer, assumes immutability
/// let ptr = x.get();
/// x.get_mut_unchecked().write(1);
/// ptr.read(); // UB!
/// assert_eq!(ptr.read(), 1);
/// }
/// ```
#[inline(always)]
#[must_use]
#[unstable(feature = "unsafe_pinned", issue = "125735")]
pub const fn get(&self) -> *const T {
ptr::from_ref(self) as *const T
pub const fn get(&self) -> *mut T {
self.value.get()
}

/// Gets an immutable pointer to the wrapped value.
Expand Down
Loading