From 95da8673d76bb824c2fb57376cb6b254b14eb15e Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Fri, 6 Sep 2024 18:15:07 +0200 Subject: [PATCH] Model unlocked -> locked via unlocked by construction This is less code, less run-time work and conceptually harder to miss-use. Win-win. --- src/lib.rs | 20 -------------------- src/unsafe_self_cell.rs | 27 ++++++--------------------- tests/self_cell.rs | 41 ++++++++--------------------------------- 3 files changed, 14 insertions(+), 74 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 96433e0..04b8b66 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -385,8 +385,6 @@ macro_rules! self_cell { // Move owner into newly allocated space. owner_ptr.write(owner); - $crate::_mut_borrow_unlock!(owner_ptr, $Owner); - // Drop guard that cleans up should building the dependent panic. let drop_guard = $crate::unsafe_self_cell::OwnerAndCellDropGuard::new(joined_ptr); @@ -437,8 +435,6 @@ macro_rules! self_cell { // Move owner into newly allocated space. owner_ptr.write(owner); - $crate::_mut_borrow_unlock!(owner_ptr, $Owner); - // Drop guard that cleans up should building the dependent panic. let mut drop_guard = $crate::unsafe_self_cell::OwnerAndCellDropGuard::new(joined_ptr); @@ -493,8 +489,6 @@ macro_rules! self_cell { // Move owner into newly allocated space. owner_ptr.write(owner); - $crate::_mut_borrow_unlock!(owner_ptr, $Owner); - // Drop guard that cleans up should building the dependent panic. let mut drop_guard = $crate::unsafe_self_cell::OwnerAndCellDropGuard::new(joined_ptr); @@ -695,20 +689,6 @@ macro_rules! _impl_automatic_derive { }; } -#[doc(hidden)] -#[macro_export] -macro_rules! _mut_borrow_unlock { - ($owner_ptr:expr, $Owner:ty) => {{ - let wrapper = ::core::mem::transmute::< - &mut $Owner, - &mut $crate::unsafe_self_cell::MutBorrowSpecWrapper<$Owner>, - >(&mut *$owner_ptr); - - // If `T` is `MutBorrow` will call `unlock`, otherwise a no-op. - wrapper.unlock(); - }}; -} - #[doc(hidden)] #[macro_export] macro_rules! _mut_borrow_lock { diff --git a/src/unsafe_self_cell.rs b/src/unsafe_self_cell.rs index 895a1f5..be3d294 100644 --- a/src/unsafe_self_cell.rs +++ b/src/unsafe_self_cell.rs @@ -262,8 +262,10 @@ pub struct MutBorrow { impl MutBorrow { /// Constructs a new `MutBorrow`. pub fn new(value: T) -> Self { + // 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(true), + is_locked: Cell::new(false), value: UnsafeCell::new(value), } } @@ -284,9 +286,9 @@ impl MutBorrow { // Ensure this function can only be called once. self.is_locked.set(true); - // SAFETY: `self.is_locked` starts out as locked and can only be unlocked with `unsafe` - // functions, 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. + // 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. unsafe { &mut *self.value.get() } } } @@ -300,9 +302,6 @@ impl MutBorrow { // Provides the no-op for types that are not `MutBorrow`. #[doc(hidden)] pub unsafe trait MutBorrowDefaultTrait { - #[doc(hidden)] - unsafe fn unlock(&mut self) {} - #[doc(hidden)] fn lock(&self) {} } @@ -314,11 +313,6 @@ unsafe impl MutBorrowDefaultTrait for T {} pub struct MutBorrowSpecWrapper(T); impl MutBorrowSpecWrapper { - #[doc(hidden)] - pub unsafe fn unlock(&mut self) { - ::unlock(&mut self.0); - } - #[doc(hidden)] pub fn lock(&self) { ::lock(&self.0); @@ -328,20 +322,11 @@ impl MutBorrowSpecWrapper { // unsafe trait to make sure users can't impl this without unsafe. #[doc(hidden)] pub unsafe trait MutBorrowSpecImpl { - #[doc(hidden)] - unsafe fn unlock(&mut self); - #[doc(hidden)] fn lock(&self); } unsafe impl MutBorrowSpecImpl for MutBorrow { - // SAFETY: The caller MUST only ever call this once. - #[doc(hidden)] - unsafe fn unlock(&mut self) { - self.is_locked.set(false); - } - #[doc(hidden)] fn lock(&self) { self.is_locked.set(true); diff --git a/tests/self_cell.rs b/tests/self_cell.rs index 2413daf..62a7de8 100644 --- a/tests/self_cell.rs +++ b/tests/self_cell.rs @@ -783,24 +783,17 @@ fn cell_mem_size() { } #[test] -#[should_panic] fn mut_borrow_new_borrow() { - let mut_borrow = MutBorrow::new(45); - let _ = mut_borrow.borrow_mut(); + let mut_borrow = MutBorrow::new("abc".to_string()); + + let string_ref: &mut String = mut_borrow.borrow_mut(); + string_ref.pop(); + assert_eq!(string_ref, "ab"); } #[test] fn mut_borrow_mutate() { - let mut mut_borrow = MutBorrow::new(45); - - unsafe { - let wrapper = std::mem::transmute::< - &mut MutBorrow, - &mut self_cell::unsafe_self_cell::MutBorrowSpecWrapper>, - >(&mut mut_borrow); - - wrapper.unlock(); - } + let mut_borrow = MutBorrow::new(45); let mut_ref: &mut i32 = mut_borrow.borrow_mut(); @@ -812,16 +805,7 @@ fn mut_borrow_mutate() { #[test] #[should_panic] fn mut_borrow_double_borrow() { - let mut mut_borrow = MutBorrow::new(45); - - unsafe { - let wrapper = std::mem::transmute::< - &mut MutBorrow, - &mut self_cell::unsafe_self_cell::MutBorrowSpecWrapper>, - >(&mut mut_borrow); - - wrapper.unlock(); - } + let mut_borrow = MutBorrow::new(45); let _mut_ref_a: &mut i32 = mut_borrow.borrow_mut(); let _mut_ref_b: &mut i32 = mut_borrow.borrow_mut(); @@ -830,16 +814,7 @@ fn mut_borrow_double_borrow() { #[test] #[should_panic] fn mut_borrow_lock_borrow() { - let mut mut_borrow = MutBorrow::new(45); - - unsafe { - let wrapper = std::mem::transmute::< - &mut MutBorrow, - &mut self_cell::unsafe_self_cell::MutBorrowSpecWrapper>, - >(&mut mut_borrow); - - wrapper.unlock(); - } + let mut_borrow = MutBorrow::new(45); { let wrapper = unsafe {