Remove unnecessary lock code and switch to AtomicBool

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<Self, Err> {
             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<Self, ($Owner, Err)> {
             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;

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<Owner, Dependent> JoinedCell<Owner, Dependent> {
 /// ```
 pub struct MutBorrow<T> {
     // Private on purpose.
-    is_locked: Cell<bool>,
+    is_locked: AtomicBool,
     value: UnsafeCell<T>,
 }
 
@@ -265,7 +266,7 @@ impl<T> MutBorrow<T> {
         // 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<T> MutBorrow<T> {
     /// 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<T> MutBorrow<T> {
     }
 }
 
-// Provides the no-op for types that are not `MutBorrow`.
-#[doc(hidden)]
-pub unsafe trait MutBorrowDefaultTrait {
-    #[doc(hidden)]
-    fn lock(&self) {}
-}
-
-unsafe impl<T> MutBorrowDefaultTrait for T {}
-
-#[doc(hidden)]
-#[repr(transparent)]
-pub struct MutBorrowSpecWrapper<T>(T);
-
-impl<T: MutBorrowSpecImpl> MutBorrowSpecWrapper<T> {
-    #[doc(hidden)]
-    pub fn lock(&self) {
-        <T as MutBorrowSpecImpl>::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<T> MutBorrowSpecImpl for MutBorrow<T> {
-    #[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<T: Send> Sync for MutBorrow<T> {}

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<String>;
+    assert!(impls!(MutBorrowString: Send));
+    assert!(impls!(MutBorrowString: Sync));
+
+    type MutBorrowRefCellString = MutBorrow<RefCell<String>>;
+    assert!(impls!(MutBorrowRefCellString: Send));
+    assert!(impls!(MutBorrowRefCellString: Sync));
+
+    type MutBorrowRcString = MutBorrow<Rc<String>>;
+    assert!(!impls!(MutBorrowRcString: Send));
+    assert!(!impls!(MutBorrowRcString: Sync));
+
+    type MutStringRef<'a> = &'a mut String;
+
+    self_cell!(
+        struct MutBorrowStringCell {
+            owner: MutBorrow<String>,
+
+            #[covariant]
+            dependent: MutStringRef,
+        }
+    );
+
+    assert!(impls!(MutBorrowStringCell: Send));
+    assert!(impls!(MutBorrowStringCell: Sync));
+}
+
 #[test]
 #[cfg(feature = "invalid_programs")]
 // Not supported by miri isolation.

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<i32>,
-                &self_cell::unsafe_self_cell::MutBorrowSpecWrapper<MutBorrow<i32>>,
-            >(&mut_borrow)
-        };
-
-        wrapper.lock();
-    }
-
-    let _: &mut i32 = mut_borrow.borrow_mut();
-}
-
 type MutStringRef<'a> = &'a mut String;
 
 self_cell!(
@@ -918,54 +899,10 @@ fn mut_borrow_try_new_or_recover() {
     assert_eq!(err, 678);
 }
 
-type MutBorrowStringRef<'a> = &'a MutBorrow<String>;
-
-self_cell!(
-    struct MutStringFullBorrowCell {
-        owner: MutBorrow<String>,
-
-        #[covariant]
-        dependent: MutBorrowStringRef,
-    }
-);
-
 #[test]
 #[should_panic]
 fn mut_borrow_new_borrow_mut() {
     let cell = MutStringCell::new(MutBorrow::new("abc".into()), |owner| owner.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<MutBorrowStringRef, ()> { 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<MutBorrowStringRef, ()> { std::result::Result::Ok(owner) },
-    )
-    .map_err(|(_owner, err)| err)
-    .unwrap();
-
-    cell.borrow_owner().borrow_mut();
-}