Auto merge of rust-lang#96820 - r-raymond:master, r=cuviper

Make RwLockReadGuard covariant

Hi, first time contributor here, if anything is not as expected, please let me know.

`RwLockReadGoard`'s type constructor is invariant. Since it behaves like a smart pointer to an immutable reference, there is no reason that it should not be covariant. Take e.g.

```
fn test_read_guard_covariance() {
    fn do_stuff<'a>(_: RwLockReadGuard<'_, &'a i32>, _: &'a i32) {}
    let j: i32 = 5;
    let lock = RwLock::new(&j);
    {
        let i = 6;
        do_stuff(lock.read().unwrap(), &i);
    }
    drop(lock);
}
```
where the compiler complains that &i doesn't live long enough. If `RwLockReadGuard` is covariant, then the above code is accepted because the lifetime can be shorter than `'a`.

In order for `RwLockReadGuard` to be covariant, it can't contain a full reference to the `RwLock`, which can never be covariant (because it exposes a mutable reference to the underlying data structure). By reducing the data structure to the required pieces of `RwLock`, the rest falls in place.

If there is a better way to do a test that tests successful compilation, please let me know.

Fixes rust-lang#80392

library/std/src/sync/rwlock.rs

@@ -4,6 +4,7 @@ mod tests;
 use crate::cell::UnsafeCell;
 use crate::fmt;
 use crate::ops::{Deref, DerefMut};
+use crate::ptr::NonNull;
 use crate::sync::{poison, LockResult, TryLockError, TryLockResult};
 use crate::sys_common::rwlock as sys;
 
@@ -101,7 +102,12 @@ unsafe impl<T: ?Sized + Send + Sync> Sync for RwLock<T> {}
 #[stable(feature = "rust1", since = "1.0.0")]
 #[clippy::has_significant_drop]
 pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
-    lock: &'a RwLock<T>,
+    // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a
+    // `Ref` argument doesn't hold immutability for its whole scope, only until it drops.
+    // `NonNull` is also covariant over `T`, just like we would have with `&T`. `NonNull`
+    // is preferable over `const* T` to allow for niche optimization.
+    data: NonNull<T>,
+    inner_lock: &'a sys::MovableRwLock,
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
@@ -511,12 +517,21 @@ impl<T> From<T> for RwLock<T> {
 }
 
 impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> {
+    /// Create a new instance of `RwLockReadGuard<T>` from a `RwLock<T>`.
+    // SAFETY: if and only if `lock.inner.read()` (or `lock.inner.try_read()`) has been
+    // successfully called from the same thread before instantiating this object.
     unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockReadGuard<'rwlock, T>> {
-        poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { lock })
+        poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard {
+            data: NonNull::new_unchecked(lock.data.get()),
+            inner_lock: &lock.inner,
+        })
     }
 }
 
 impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> {
+    /// Create a new instance of `RwLockWriteGuard<T>` from a `RwLock<T>`.
+    // SAFETY: if and only if `lock.inner.write()` (or `lock.inner.try_write()`) has been
+    // successfully called from the same thread before instantiating this object.
     unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockWriteGuard<'rwlock, T>> {
         poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard })
     }
@@ -555,7 +570,8 @@ impl<T: ?Sized> Deref for RwLockReadGuard<'_, T> {
     type Target = T;
 
     fn deref(&self) -> &T {
-        unsafe { &*self.lock.data.get() }
+        // SAFETY: the conditions of `RwLockGuard::new` were satisfied when created.
+        unsafe { self.data.as_ref() }
     }
 }
 
@@ -564,22 +580,25 @@ impl<T: ?Sized> Deref for RwLockWriteGuard<'_, T> {
     type Target = T;
 
     fn deref(&self) -> &T {
+        // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
         unsafe { &*self.lock.data.get() }
     }
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<T: ?Sized> DerefMut for RwLockWriteGuard<'_, T> {
     fn deref_mut(&mut self) -> &mut T {
+        // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
         unsafe { &mut *self.lock.data.get() }
     }
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<T: ?Sized> Drop for RwLockReadGuard<'_, T> {
     fn drop(&mut self) {
+        // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when created.
         unsafe {
-            self.lock.inner.read_unlock();
+            self.inner_lock.read_unlock();
         }
     }
 }
@@ -588,6 +607,7 @@ impl<T: ?Sized> Drop for RwLockReadGuard<'_, T> {
 impl<T: ?Sized> Drop for RwLockWriteGuard<'_, T> {
     fn drop(&mut self) {
         self.lock.poison.done(&self.poison);
+        // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
         unsafe {
             self.lock.inner.write_unlock();
         }

library/std/src/sync/rwlock/tests.rs

@@ -1,6 +1,6 @@
 use crate::sync::atomic::{AtomicUsize, Ordering};
 use crate::sync::mpsc::channel;
-use crate::sync::{Arc, RwLock, TryLockError};
+use crate::sync::{Arc, RwLock, RwLockReadGuard, TryLockError};
 use crate::thread;
 use rand::{self, Rng};
 
@@ -245,3 +245,15 @@ fn test_get_mut_poison() {
         Ok(x) => panic!("get_mut of poisoned RwLock is Ok: {x:?}"),
     }
 }
+
+#[test]
+fn test_read_guard_covariance() {
+    fn do_stuff<'a>(_: RwLockReadGuard<'_, &'a i32>, _: &'a i32) {}
+    let j: i32 = 5;
+    let lock = RwLock::new(&j);
+    {
+        let i = 6;
+        do_stuff(lock.read().unwrap(), &i);
+    }
+    drop(lock);
+}

src/test/codegen/noalias-rwlockreadguard.rs

@@ -0,0 +1,14 @@
+// compile-flags: -O -C no-prepopulate-passes -Z mutable-noalias=yes
+
+#![crate_type = "lib"]
+
+use std::sync::{RwLock, RwLockReadGuard};
+
+// Make sure that `RwLockReadGuard` does not get a `noalias` attribute, because
+// the `RwLock` might alias writes after it is dropped.
+
+// CHECK-LABEL: @maybe_aliased(
+// CHECK-NOT: noalias
+// CHECK-SAME: %_data
+#[no_mangle]
+pub unsafe fn maybe_aliased(_: RwLockReadGuard<'_, i32>, _data: &RwLock<i32>) {}

src/test/debuginfo/rwlock-read.rs

@@ -15,11 +15,8 @@
 //
 // cdb-command:dx r
 // cdb-check:r                [Type: std::sync::rwlock::RwLockReadGuard<i32>]
-// cdb-check:    [...] lock             : [...] [Type: std::sync::rwlock::RwLock<i32> *]
-//
-// cdb-command:dx r.lock->data,d
-// cdb-check:r.lock->data,d   : 0 [Type: core::cell::UnsafeCell<i32>]
-// cdb-check:    [<Raw View>]     [Type: core::cell::UnsafeCell<i32>]
+// cdb-check:    [...] data             : NonNull([...]: 0) [Type: core::ptr::non_null::NonNull<i32>]
+// cdb-check:    [...] inner_lock       : [...] [Type: std::sys_common::rwlock::MovableRwLock *]
 
 #[allow(unused_variables)]