Skip to content

Commit

Permalink
Rollup merge of #80771 - thomcc:nonnull-refmut, r=dtolnay
Browse files Browse the repository at this point in the history
Make NonNull::as_ref (and friends) return refs with unbound lifetimes

# Rationale:

1. The documentation for all of these functions claims that this is what the functions already do, as they all come with this comment:

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

    So I think it's just a bug that they weren't this way already. Note that had it not been for this part, I wouldn't be making this PR, so if we decide we won't take this change, I'll follow it up with a docs PR to fix this.

2. This is how the equivalent raw pointer functions behave.

    They also take `self` and not `&self`/`&mut self`, but that can't be changed compatibly at this point. This is the next best thing.

3. Without this fix, often code that uses these methods will find it has to expand the lifetime of the result.

    (I can't speak for others but even in unsafe-heavy code, needing to do this unexpectedly is a huge red flag -- if Rust thinks something should have a specific lifetime, I assume it's for a reason)

### Can this cause existing code to be unsound?

I'm confident this can't cause new unsoundness since the reference exists for at most its lifetime, but you get a borrow checker error if you do something that would require/allow the reference to exist past its lifetime.

Additionally, the aliasing rules of a reference only applies while the reference exists.

This *must* be the case, as it is required by the rules used by safe code. (That said, the documentation in this file sort of contradicts it, but I think it's just ambiguity between the lifetime `'a` in `&'a T` and lifetime of the `&'a T` reference itself...)

We are increasing the lifetime of these references, but they should already have hard bounds on that lifetime, or they'd have borrow checker errors.

(CC ``@RalfJung`` because I have gone and done the mistake where I say something definitive about aliasing in Rust which is honestly outside the group of things I should make definitive comments about).

# Caveats

1. This is insta-stable (except for on the unstable functions ofc). I don't think there's any other alternative.

2. I don't believe this is a breaking change in practice. In theory someone could be assigning `NonNull::as_ref` to a function pointer of type `fn(&NonNull<T>) -> &T`. Now they'd need to use a slightly different function pointer type which is (probably) incompatible. This seems pathological, but I guess crater could be used if there are concerns.

3. This has no tests. The old version didn't either that I saw. I could add some stuff that fails to compile without it, if that would be useful.

4. Sometimes the NLL borrow checker gives up and decides lifetimes live till the end of the scope, as opposed to the range where they're used. If this change can cause this to happen more, then my soundness rationale is wrong, and it's likely breaking.

    In practice this seems super unlikely.

Anyway. That was a lot of typing.

Fixes #80183
  • Loading branch information
Dylan-DPC committed Mar 22, 2021
2 parents e9398bc + cce258b commit ad8aa18
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl<T: Sized> NonNull<T> {
/// [the module documentation]: crate::ptr#safety
#[inline]
#[unstable(feature = "ptr_as_uninit", issue = "75402")]
pub unsafe fn as_uninit_ref(&self) -> &MaybeUninit<T> {
pub unsafe fn as_uninit_ref<'a>(&self) -> &'a MaybeUninit<T> {
// SAFETY: the caller must guarantee that `self` meets all the
// requirements for a reference.
unsafe { &*self.cast().as_ptr() }
Expand Down Expand Up @@ -142,7 +142,7 @@ impl<T: Sized> NonNull<T> {
/// [the module documentation]: crate::ptr#safety
#[inline]
#[unstable(feature = "ptr_as_uninit", issue = "75402")]
pub unsafe fn as_uninit_mut(&mut self) -> &mut MaybeUninit<T> {
pub unsafe fn as_uninit_mut<'a>(&mut self) -> &'a mut MaybeUninit<T> {
// SAFETY: the caller must guarantee that `self` meets all the
// requirements for a reference.
unsafe { &mut *self.cast().as_ptr() }
Expand Down Expand Up @@ -244,7 +244,7 @@ impl<T: ?Sized> NonNull<T> {
/// [the module documentation]: crate::ptr#safety
#[stable(feature = "nonnull", since = "1.25.0")]
#[inline]
pub unsafe fn as_ref(&self) -> &T {
pub unsafe fn as_ref<'a>(&self) -> &'a T {
// SAFETY: the caller must guarantee that `self` meets all the
// requirements for a reference.
unsafe { &*self.as_ptr() }
Expand Down Expand Up @@ -280,7 +280,7 @@ impl<T: ?Sized> NonNull<T> {
/// [the module documentation]: crate::ptr#safety
#[stable(feature = "nonnull", since = "1.25.0")]
#[inline]
pub unsafe fn as_mut(&mut self) -> &mut T {
pub unsafe fn as_mut<'a>(&mut self) -> &'a mut T {
// SAFETY: the caller must guarantee that `self` meets all the
// requirements for a mutable reference.
unsafe { &mut *self.as_ptr() }
Expand Down Expand Up @@ -427,7 +427,7 @@ impl<T> NonNull<[T]> {
/// [valid]: crate::ptr#safety
#[inline]
#[unstable(feature = "ptr_as_uninit", issue = "75402")]
pub unsafe fn as_uninit_slice(&self) -> &[MaybeUninit<T>] {
pub unsafe fn as_uninit_slice<'a>(&self) -> &'a [MaybeUninit<T>] {
// SAFETY: the caller must uphold the safety contract for `as_uninit_slice`.
unsafe { slice::from_raw_parts(self.cast().as_ptr(), self.len()) }
}
Expand Down Expand Up @@ -488,7 +488,7 @@ impl<T> NonNull<[T]> {
/// ```
#[inline]
#[unstable(feature = "ptr_as_uninit", issue = "75402")]
pub unsafe fn as_uninit_slice_mut(&self) -> &mut [MaybeUninit<T>] {
pub unsafe fn as_uninit_slice_mut<'a>(&self) -> &'a mut [MaybeUninit<T>] {
// SAFETY: the caller must uphold the safety contract for `as_uninit_slice_mut`.
unsafe { slice::from_raw_parts_mut(self.cast().as_ptr(), self.len()) }
}
Expand Down

0 comments on commit ad8aa18

Please sign in to comment.