diff --git a/src/lib.rs b/src/lib.rs index 04b8b66..55f70c8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -354,9 +354,6 @@ macro_rules! self_cell { ) -> Self { use ::core::ptr::NonNull; - #[allow(unused)] - use $crate::unsafe_self_cell::MutBorrowDefaultTrait; - unsafe { // All this has to happen here, because there is not good way // of passing the appropriate logic into UnsafeSelfCell::new @@ -393,8 +390,6 @@ macro_rules! self_cell { dependent_ptr.write(dependent_builder(&*owner_ptr)); ::core::mem::forget(drop_guard); - $crate::_mut_borrow_lock!(owner_ptr, $Owner); - Self { unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new( joined_void_ptr, @@ -414,9 +409,6 @@ macro_rules! self_cell { ) -> ::core::result::Result { use ::core::ptr::NonNull; - #[allow(unused)] - use $crate::unsafe_self_cell::MutBorrowDefaultTrait; - unsafe { // See fn new for more explanation. @@ -444,8 +436,6 @@ macro_rules! self_cell { dependent_ptr.write(dependent); ::core::mem::forget(drop_guard); - $crate::_mut_borrow_lock!(owner_ptr, $Owner); - ::core::result::Result::Ok(Self { unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new( joined_void_ptr, @@ -468,9 +458,6 @@ macro_rules! self_cell { ) -> ::core::result::Result { use ::core::ptr::NonNull; - #[allow(unused)] - use $crate::unsafe_self_cell::MutBorrowDefaultTrait; - unsafe { // See fn new for more explanation. @@ -498,8 +485,6 @@ macro_rules! self_cell { dependent_ptr.write(dependent); ::core::mem::forget(drop_guard); - $crate::_mut_borrow_lock!(owner_ptr, $Owner); - ::core::result::Result::Ok(Self { unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new( joined_void_ptr, @@ -689,18 +674,4 @@ macro_rules! _impl_automatic_derive { }; } -#[doc(hidden)] -#[macro_export] -macro_rules! _mut_borrow_lock { - ($owner_ptr:expr, $Owner:ty) => {{ - let wrapper = ::core::mem::transmute::< - &$Owner, - &$crate::unsafe_self_cell::MutBorrowSpecWrapper<$Owner>, - >(&*$owner_ptr); - - // If `T` is `MutBorrow` will call `lock`, otherwise a no-op. - wrapper.lock(); - }}; -} - pub use unsafe_self_cell::MutBorrow; diff --git a/src/unsafe_self_cell.rs b/src/unsafe_self_cell.rs index be3d294..17ec58d 100644 --- a/src/unsafe_self_cell.rs +++ b/src/unsafe_self_cell.rs @@ -1,9 +1,10 @@ #![allow(clippy::needless_lifetimes)] -use core::cell::{Cell, UnsafeCell}; +use core::cell::UnsafeCell; use core::marker::PhantomData; use core::mem; use core::ptr::{drop_in_place, read, NonNull}; +use core::sync::atomic::{AtomicBool, Ordering}; extern crate alloc; @@ -255,7 +256,7 @@ impl JoinedCell { /// ``` pub struct MutBorrow { // Private on purpose. - is_locked: Cell, + is_locked: AtomicBool, value: UnsafeCell, } @@ -265,7 +266,7 @@ impl MutBorrow { // Use the Rust type system to model an affine type that can only go from unlocked -> locked // but never the other way around. Self { - is_locked: Cell::new(false), + is_locked: AtomicBool::new(false), value: UnsafeCell::new(value), } } @@ -280,12 +281,14 @@ impl MutBorrow { /// Will panic if called anywhere but in the dependent constructor. Will also panic if called /// more than once. pub fn borrow_mut(&self) -> &mut T { - if self.is_locked.get() { + // Ensure this function can only be called once. + // Relaxed should be fine, because only one thread could ever read `false` anyway, + // so further synchronization is pointless. + let was_locked = self.is_locked.swap(true, Ordering::Relaxed); + + if was_locked { panic!("Tried to access locked MutBorrow") } else { - // Ensure this function can only be called once. - self.is_locked.set(true); - // SAFETY: `self.is_locked` starts out as locked and can never be unlocked again, which // guarantees that this function can only be called once. And the `self.value` being // private ensures that there are no other references to it. @@ -299,36 +302,10 @@ impl MutBorrow { } } -// Provides the no-op for types that are not `MutBorrow`. -#[doc(hidden)] -pub unsafe trait MutBorrowDefaultTrait { - #[doc(hidden)] - fn lock(&self) {} -} - -unsafe impl MutBorrowDefaultTrait for T {} - -#[doc(hidden)] -#[repr(transparent)] -pub struct MutBorrowSpecWrapper(T); - -impl MutBorrowSpecWrapper { - #[doc(hidden)] - pub fn lock(&self) { - ::lock(&self.0); - } -} - -// unsafe trait to make sure users can't impl this without unsafe. -#[doc(hidden)] -pub unsafe trait MutBorrowSpecImpl { - #[doc(hidden)] - fn lock(&self); -} - -unsafe impl MutBorrowSpecImpl for MutBorrow { - #[doc(hidden)] - fn lock(&self) { - self.is_locked.set(true); - } -} +// SAFETY: The reasoning why it is safe to share `MutBorrow` across threads is as follows: The +// `AtomicBool` `is_locked` ensures that only ever exactly one thread can get access to the inner +// value. In that sense it works like a critical section, that begins when `borrow_mut()` is called +// and that ends when the outer `MutBorrow` is dropped. Once one thread acquired the unique +// reference through `borrow_mut()` no other interaction with the inner value MUST ever be possible +// while the outer `MutBorrow` is alive. +unsafe impl Sync for MutBorrow {} diff --git a/tests-extra/Cargo.lock b/tests-extra/Cargo.lock index 65b4c62..88c53a5 100644 --- a/tests-extra/Cargo.lock +++ b/tests-extra/Cargo.lock @@ -92,7 +92,7 @@ dependencies = [ [[package]] name = "self_cell" -version = "1.0.1" +version = "1.0.4" [[package]] name = "serde" diff --git a/tests-extra/src/lib.rs b/tests-extra/src/lib.rs index ca3a4fe..bb0a034 100644 --- a/tests-extra/src/lib.rs +++ b/tests-extra/src/lib.rs @@ -1,15 +1,14 @@ -#![allow(dead_code)] #![allow(unused_imports)] -use std::fs; -use std::process::Command; +use std::cell::RefCell; +use std::rc::Rc; use std::str; use crossbeam_utils::thread; use impls::impls; -use self_cell::self_cell; +use self_cell::{self_cell, MutBorrow}; #[allow(dead_code)] struct NotSend<'a> { @@ -55,6 +54,35 @@ fn not_sync() { assert!(!impls!(NotSendCell: Sync)); } +#[test] +fn mut_borrow_traits() { + type MutBorrowString = MutBorrow; + assert!(impls!(MutBorrowString: Send)); + assert!(impls!(MutBorrowString: Sync)); + + type MutBorrowRefCellString = MutBorrow>; + assert!(impls!(MutBorrowRefCellString: Send)); + assert!(impls!(MutBorrowRefCellString: Sync)); + + type MutBorrowRcString = MutBorrow>; + assert!(!impls!(MutBorrowRcString: Send)); + assert!(!impls!(MutBorrowRcString: Sync)); + + type MutStringRef<'a> = &'a mut String; + + self_cell!( + struct MutBorrowStringCell { + owner: MutBorrow, + + #[covariant] + dependent: MutStringRef, + } + ); + + assert!(impls!(MutBorrowStringCell: Send)); + assert!(impls!(MutBorrowStringCell: Sync)); +} + #[test] #[cfg(feature = "invalid_programs")] // Not supported by miri isolation. diff --git a/tests/self_cell.rs b/tests/self_cell.rs index 62a7de8..f3fd22b 100644 --- a/tests/self_cell.rs +++ b/tests/self_cell.rs @@ -811,25 +811,6 @@ fn mut_borrow_double_borrow() { let _mut_ref_b: &mut i32 = mut_borrow.borrow_mut(); } -#[test] -#[should_panic] -fn mut_borrow_lock_borrow() { - let mut_borrow = MutBorrow::new(45); - - { - let wrapper = unsafe { - std::mem::transmute::< - &MutBorrow, - &self_cell::unsafe_self_cell::MutBorrowSpecWrapper>, - >(&mut_borrow) - }; - - wrapper.lock(); - } - - let _: &mut i32 = mut_borrow.borrow_mut(); -} - type MutStringRef<'a> = &'a mut String; self_cell!( @@ -918,17 +899,6 @@ fn mut_borrow_try_new_or_recover() { assert_eq!(err, 678); } -type MutBorrowStringRef<'a> = &'a MutBorrow; - -self_cell!( - struct MutStringFullBorrowCell { - owner: MutBorrow, - - #[covariant] - dependent: MutBorrowStringRef, - } -); - #[test] #[should_panic] fn mut_borrow_new_borrow_mut() { @@ -936,36 +906,3 @@ fn mut_borrow_new_borrow_mut() { cell.borrow_owner().borrow_mut(); } - -#[test] -#[should_panic] -fn mut_borrow_new_late_borrow_mut() { - let cell = MutStringFullBorrowCell::new(MutBorrow::new("abc".into()), |owner| owner); - - cell.borrow_owner().borrow_mut(); -} - -#[test] -#[should_panic] -fn mut_borrow_try_new_late_borrow_mut() { - let cell = MutStringFullBorrowCell::try_new( - MutBorrow::new("abc".into()), - |owner| -> Result { std::result::Result::Ok(owner) }, - ) - .unwrap(); - - cell.borrow_owner().borrow_mut(); -} - -#[test] -#[should_panic] -fn mut_borrow_try_new_or_recover_late_borrow_mut() { - let cell = MutStringFullBorrowCell::try_new_or_recover( - MutBorrow::new("abc".into()), - |owner| -> Result { std::result::Result::Ok(owner) }, - ) - .map_err(|(_owner, err)| err) - .unwrap(); - - cell.borrow_owner().borrow_mut(); -}