Skip to content

Implement ptr_as_ref_unchecked #122492

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 1 commit into from
May 3, 2024
Merged

Implement ptr_as_ref_unchecked #122492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 48 additions & 0 deletions library/core/src/ptr/const_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,54 @@ impl<T: ?Sized> *const T {
if self.is_null() { None } else { unsafe { Some(&*self) } }
}

/// Returns a shared reference to the value behind the pointer.
/// If the pointer may be null or the value may be uninitialized, [`as_uninit_ref`] must be used instead.
/// If the pointer may be null, but the value is known to have been initialized, [`as_ref`] must be used instead.
///
/// [`as_ref`]: #method.as_ref
/// [`as_uninit_ref`]: #method.as_uninit_ref
///
/// # Safety
///
/// When calling this method, you have to ensure that all of the following is true:
///
/// * The pointer must be properly aligned.
///
/// * It must be "dereferenceable" in the sense defined in [the module documentation].
///
/// * The pointer must point to an initialized instance of `T`.
///
/// * You must enforce Rust's aliasing rules, since the returned lifetime `'a` is
/// arbitrarily chosen and does not necessarily reflect the actual lifetime of the data.
Comment on lines +382 to +383
Copy link
Member

Choose a reason for hiding this comment

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

I realize this is copied from elsewhere but it has been copied so often with subtle changes of the context around it that by being a copy of a copy of a copy it reads fairly jarringly in this new case. I would prefer it was more explicit to explain that you, the caller of this unsafe fn, define the 'a lifetime by calling this (you may pass an explicit generic via turbofish, or it will be inferred). You are making a claim that you will uphold it for that long, and "arbitrarily chosen" makes it sound more... impersonal.

All of those requirements, except mutation (which is stated in this clause) are stated previously, so perhaps it would be better to move the non-mutation clause into its own bullet. Then it can simply say "all of the previous rules".

Copy link
Member

@RalfJung RalfJung Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In practice this is almost always chosen by inference, which makes it equivalent to 'static, so "arbitrarily chosen" is fairly close to the truth. Whatever new wording we use here should preserve that sentiment.

/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
Comment on lines +384 to +385
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly it is still illegal to mutate &'a Inner if I use this to obtain &'a Inner directly, instead of &'a UnsafeCell<Inner>. It might be better to describe this as "except via pointers obtained by UnsafeCell::get and other functions on UnsafeCell." Again, I understand this has already been copied a zillion times and you didn't originate this.

///
/// This applies even if the result of this method is unused!
/// (The part about being initialized is not yet fully decided, but until
/// it is, the only safe approach is to ensure that they are indeed initialized.)
///
/// [the module documentation]: crate::ptr#safety
///
/// # Examples
///
/// ```
/// #![feature(ptr_as_ref_unchecked)]
/// let ptr: *const u8 = &10u8 as *const u8;
///
/// unsafe {
/// println!("We got back the value: {}!", ptr.as_ref_unchecked());
/// }
Comment on lines +399 to +401
Copy link
Member

Choose a reason for hiding this comment

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

A lot of doctests don't really test anything because they're no_run or they only have prints like this one. Perhaps this could test something simple like

    assert!(10.eq(ptr.as_ref_unchecked()));

/// ```
// FIXME: mention it in the docs for `as_ref` and `as_uninit_ref` once stabilized.
#[unstable(feature = "ptr_as_ref_unchecked", issue = "122034")]
#[rustc_const_unstable(feature = "const_ptr_as_ref", issue = "91822")]
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we should be associating this with const-stability of this set of fn instead of just landing these as const stable when we land them? Can you make sure the two issues are crosslinked and mention that as an option?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we shouldn't associate this with const_ptr_as_ref, so I'm adjusting that in #130164.

When a const fn is still #[unstable], it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

#[inline]
#[must_use]
pub const unsafe fn as_ref_unchecked<'a>(self) -> &'a T {
// SAFETY: the caller must guarantee that `self` is valid for a reference
unsafe { &*self }
}

/// Returns `None` if the pointer is null, or else returns a shared reference to
/// the value wrapped in `Some`. In contrast to [`as_ref`], this does not require
/// that the value has to be initialized.
Expand Down
103 changes: 103 additions & 0 deletions library/core/src/ptr/mut_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,57 @@ impl<T: ?Sized> *mut T {
if self.is_null() { None } else { unsafe { Some(&*self) } }
}

/// Returns a shared reference to the value behind the pointer.
/// If the pointer may be null or the value may be uninitialized, [`as_uninit_ref`] must be used instead.
/// If the pointer may be null, but the value is known to have been initialized, [`as_ref`] must be used instead.
///
/// For the mutable counterpart see [`as_mut_unchecked`].
///
/// [`as_ref`]: #method.as_ref
/// [`as_uninit_ref`]: #method.as_uninit_ref
/// [`as_mut_unchecked`]: #method.as_mut_unchecked
///
/// # Safety
///
/// When calling this method, you have to ensure that all of the following is true:
///
/// * The pointer must be properly aligned.
///
/// * It must be "dereferenceable" in the sense defined in [the module documentation].
///
/// * The pointer must point to an initialized instance of `T`.
///
/// * You must enforce Rust's aliasing rules, since the returned lifetime `'a` is
/// arbitrarily chosen and does not necessarily reflect the actual lifetime of the data.
/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
///
/// This applies even if the result of this method is unused!
/// (The part about being initialized is not yet fully decided, but until
/// it is, the only safe approach is to ensure that they are indeed initialized.)
///
/// [the module documentation]: crate::ptr#safety
///
/// # Examples
///
/// ```
/// #![feature(ptr_as_ref_unchecked)]
/// let ptr: *mut u8 = &mut 10u8 as *mut u8;
///
/// unsafe {
/// println!("We got back the value: {}!", ptr.as_ref_unchecked());
/// }
/// ```
// FIXME: mention it in the docs for `as_ref` and `as_uninit_ref` once stabilized.
Comment on lines +375 to +416
Copy link
Member

Choose a reason for hiding this comment

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

My comments from before apply down the line here.

#[unstable(feature = "ptr_as_ref_unchecked", issue = "122034")]
#[rustc_const_unstable(feature = "const_ptr_as_ref", issue = "91822")]
#[inline]
#[must_use]
pub const unsafe fn as_ref_unchecked<'a>(self) -> &'a T {
// SAFETY: the caller must guarantee that `self` is valid for a reference
unsafe { &*self }
}

/// Returns `None` if the pointer is null, or else returns a shared reference to
/// the value wrapped in `Some`. In contrast to [`as_ref`], this does not require
/// that the value has to be initialized.
Expand Down Expand Up @@ -693,6 +744,58 @@ impl<T: ?Sized> *mut T {
if self.is_null() { None } else { unsafe { Some(&mut *self) } }
}

/// Returns a unique reference to the value behind the pointer.
/// If the pointer may be null or the value may be uninitialized, [`as_uninit_mut`] must be used instead.
/// If the pointer may be null, but the value is known to have been initialized, [`as_mut`] must be used instead.
///
/// For the shared counterpart see [`as_ref_unchecked`].
///
/// [`as_mut`]: #method.as_mut
/// [`as_uninit_mut`]: #method.as_uninit_mut
/// [`as_ref_unchecked`]: #method.as_mut_unchecked
///
/// # Safety
///
/// When calling this method, you have to ensure that all of the following is true:
///
/// * The pointer must be properly aligned.
///
/// * It must be "dereferenceable" in the sense defined in [the module documentation].
///
/// * The pointer must point to an initialized instance of `T`.
///
/// * You must enforce Rust's aliasing rules, since the returned lifetime `'a` is
/// arbitrarily chosen and does not necessarily reflect the actual lifetime of the data.
/// In particular, while this reference exists, the memory the pointer points to must
/// not get mutated (except inside `UnsafeCell`).
///
/// This applies even if the result of this method is unused!
/// (The part about being initialized is not yet fully decided, but until
/// it is, the only safe approach is to ensure that they are indeed initialized.)
///
/// [the module documentation]: crate::ptr#safety
Comment on lines +747 to +776
Copy link
Member

Choose a reason for hiding this comment

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

My comments from before etc.

///
/// # Examples
///
/// ```
/// #![feature(ptr_as_ref_unchecked)]
/// let mut s = [1, 2, 3];
/// let ptr: *mut u32 = s.as_mut_ptr();
/// let first_value = unsafe { ptr.as_mut_unchecked() };
/// *first_value = 4;
/// # assert_eq!(s, [4, 2, 3]);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we hiding the assert_eq! here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Genuine copypasta
https://github.com/rust-lang/rust/pull/122492/files#diff-d0f76807985a6a8aa06787f524381d47762ce707757608c3f538b59abeebb0faL684

Copy link
Member

Choose a reason for hiding this comment

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

ah, delicious italian food

/// println!("{s:?}"); // It'll print: "[4, 2, 3]".
/// ```
// FIXME: mention it in the docs for `as_mut` and `as_uninit_mut` once stabilized.
#[unstable(feature = "ptr_as_ref_unchecked", issue = "122034")]
#[rustc_const_unstable(feature = "const_ptr_as_ref", issue = "91822")]
#[inline]
#[must_use]
pub const unsafe fn as_mut_unchecked<'a>(self) -> &'a mut T {
// SAFETY: the caller must guarantee that `self` is valid for a reference
unsafe { &mut *self }
}

/// Returns `None` if the pointer is null, or else returns a unique reference to
/// the value wrapped in `Some`. In contrast to [`as_mut`], this does not require
/// that the value has to be initialized.
Expand Down