Skip to content

Commit

Permalink
Rollup merge of rust-lang#96820 - r-raymond:master, r=cuviper
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JohnTitor authored Jun 22, 2022
2 parents 4415af5 + 048a801 commit c7aa270
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 5 deletions.
28 changes: 24 additions & 4 deletions library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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 })
}
Expand Down Expand Up @@ -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() }
}
}

Expand All @@ -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();
}
}
}
Expand All @@ -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();
}
Expand Down
14 changes: 13 additions & 1 deletion library/std/src/sync/rwlock/tests.rs
Original file line number Diff line number Diff line change
@@ -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};

Expand Down Expand Up @@ -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);
}
14 changes: 14 additions & 0 deletions src/test/codegen/noalias-rwlockreadguard.rs
Original file line number Diff line number Diff line change
@@ -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>) {}

0 comments on commit c7aa270

Please sign in to comment.