- Make proptest a conditional dependency

- Add multi-threaded tests for godot-cell
- Add blocking drop method for InaccessibleGuard
- Fix UB when InaccessibleGuard is blocking dropped in the wrong order in multi-threaded code

Author: lilizoey

.github/workflows/full-ci.yml

@@ -178,6 +178,20 @@ jobs:
       - name: "Test tree borrows"
         run: MIRIFLAGS="-Zmiri-tree-borrows" cargo miri test -p godot-cell
 
+  proptest:
+    name: proptest
+    runs-on: ubuntu-20.04
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: "Install Rust"
+        uses: ./.github/composite/rust
+
+      - name: "Compile tests"
+        run: cargo test -p godot-cell --features="proptest" --no-run
+
+      - name: "Test"
+        run: cargo test -p godot-cell --features="proptest"
 
   # For complex matrix workflow, see https://stackoverflow.com/a/65434401
   godot-itest:

godot-cell/Cargo.toml

@@ -3,5 +3,8 @@ name = "godot-cell"
 version = "0.1.0"
 edition = "2021"
 
-[dev-dependencies]
-proptest = "1.4.0"
+[features]
+proptest = ["dep:proptest"]
+
+[dependencies]
+proptest = { version = "1.4.0", optional = true }

godot-cell/src/borrow_state.rs

@@ -73,7 +73,7 @@ impl BorrowState {
     /// Set self as having reached an erroneous or unreliable state.
     ///
     /// Always returns [`BorrowStateErr::Poisoned`].
-    fn poison(&mut self, err: impl Into<String>) -> Result<(), BorrowStateErr> {
+    pub(crate) fn poison(&mut self, err: impl Into<String>) -> Result<(), BorrowStateErr> {
         self.poisoned = true;
 
         Err(BorrowStateErr::Poisoned(err.into()))
@@ -230,6 +230,10 @@ impl BorrowState {
         Ok(self.inaccessible_count)
     }
 
+    pub(crate) fn may_unset_inaccessible(&self) -> bool {
+        !self.has_accessible() && self.shared_count() == 0 && self.inaccessible_count > 0
+    }
+
     pub fn unset_inaccessible(&mut self) -> Result<usize, BorrowStateErr> {
         if self.has_accessible() {
             return Err("cannot set current reference as accessible when an accessible mutable reference already exists".into());
@@ -294,7 +298,7 @@ impl From<String> for BorrowStateErr {
     }
 }
 
-#[cfg(all(test, not(miri)))]
+#[cfg(all(test, feature = "proptest"))]
 mod proptests {
     use super::*;
     use proptest::{arbitrary::Arbitrary, collection::vec, prelude::*};

godot-cell/src/guards.rs

@@ -7,7 +7,7 @@
 
 use std::ops::{Deref, DerefMut};
 use std::ptr::NonNull;
-use std::sync::Mutex;
+use std::sync::{Mutex, MutexGuard};
 
 use crate::CellState;
 
@@ -69,8 +69,9 @@ impl<'a, T> Drop for RefGuard<'a, T> {
 
 /// Wraps a mutably borrowed value of type `T`.
 ///
-/// This prevents all other borrows of `value`, unless the `&mut` reference handed out from this guard is
-/// made inaccessible by a call to [`GdCell::make_inaccessible()`](crate::GdCell::make_inaccessible).
+/// This prevents all other borrows of `value` while this guard is accessible. To make this guard
+/// inaccessible, use [`GdCell::make_inaccessible()`](crate::GdCell::make_inaccessible) on a mutable
+/// reference handed out by this guard.
 #[derive(Debug)]
 pub struct MutGuard<'a, T> {
     state: &'a Mutex<CellState<T>>,
@@ -101,11 +102,11 @@ impl<'a, T> MutGuard<'a, T> {
     /// This is the case because:
     /// - [`GdCell`](super::GdCell) will not create any new references while this guard exists and is
     ///   accessible.
-    /// - When it is marked as inaccessible it is impossible to have any `&self` or `&mut self` references to
-    ///   this guard that can be used. Because we take in a `&mut self` reference with a lifetime `'a` and
-    ///   return an [`InaccessibleGuard`] with a lifetime `'b` where `'a: 'b` which ensure that the
-    ///   `&mut self` outlives that guard and cannot be used until the guard is dropped. And the rust
-    ///   borrow-checker will prevent any new references from being made.
+    /// - When it is made inaccessible it is impossible to have any `&self` or `&mut self` references to this
+    ///   guard that can be used. Because we take in a `&mut self` reference with a lifetime `'a` and return
+    ///   an [`InaccessibleGuard`] with a lifetime `'b` where `'a: 'b` which ensure that the `&mut self`
+    ///   outlives that guard and cannot be used until the guard is dropped. And the rust borrow-checker will
+    ///   prevent any new references from being made.
     /// - When it is made inaccessible, [`GdCell`](super::GdCell) will also ensure that any new references
     ///   are derived from this guard's `value` pointer, thus preventing `value` from being invalidated.
     pub(crate) unsafe fn new(
@@ -128,13 +129,23 @@ impl<'a, T> Deref for MutGuard<'a, T> {
         let count = self.state.lock().unwrap().borrow_state.mut_count();
         // This is just a best-effort error check. It should never be triggered.
         assert_eq!(
-            self.count, count,
-            "attempted to access the non-current mutable borrow. **this is a bug, please report it**"
+            self.count,
+            count,
+            "\
+            attempted to access a non-current mutable borrow of type: `{}`. \n\
+            current count: {}\n\
+            value pointer: {:p}\n\
+            attempted access count: {}\n\
+            **this is a bug, please report it**\
+            ",
+            std::any::type_name::<T>(),
+            self.count,
+            self.value,
+            count
         );
 
-        // SAFETY:
-        // It is safe to call `as_ref()` on value when we have a `&self` reference because of the safety
-        // invariants of `new`.
+        // SAFETY: It is safe to call `as_ref()` on value when we have a `&self` reference because of the
+        // safety invariants of `new`.
         unsafe { self.value.as_ref() }
     }
 }
@@ -144,8 +155,19 @@ impl<'a, T> DerefMut for MutGuard<'a, T> {
         let count = self.state.lock().unwrap().borrow_state.mut_count();
         // This is just a best-effort error check. It should never be triggered.
         assert_eq!(
-            self.count, count,
-            "attempted to access the non-current mutable borrow. **this is a bug, please report it**"
+            self.count,
+            count,
+            "\
+            attempted to access a non-current mutable borrow of type: `{}`. \n\
+            current count: {}\n\
+            value pointer: {:p}\n\
+            attempted access count: {}\n\
+            **this is a bug, please report it**\
+            ",
+            std::any::type_name::<T>(),
+            self.count,
+            self.value,
+            count
         );
 
         // SAFETY:
@@ -170,22 +192,29 @@ impl<'a, T> Drop for MutGuard<'a, T> {
 
 /// A guard that ensures a mutable reference is kept inaccessible until it is dropped.
 ///
-/// The current reference is stored in the guard and we push a new reference to `state` on creation. We then
-/// undo this upon dropping the guard.
+/// We store the current reference in the guard upon creation, and push a new reference to `state` on
+/// creation. When the guard is dropped, `state`'s pointer is reset to the original pointer.
 ///
-/// This ensure that any new references are derived from the new reference we pass in, and when this guard is
-/// dropped we reset it to the previous reference.
+/// This ensures that any new references are derived from the new reference we pass in, and when this guard
+/// is dropped the state is reset to what it was before, as if this guard never existed.
 #[derive(Debug)]
 pub struct InaccessibleGuard<'a, T> {
     state: &'a Mutex<CellState<T>>,
+    stack_depth: usize,
     prev_ptr: NonNull<T>,
 }
 
 impl<'a, T> InaccessibleGuard<'a, T> {
     /// Create a new inaccessible guard for `state`.
     ///
     /// Since `'b` must outlive `'a`, we cannot have any other references aliasing `new_ref` while this
-    /// guard exists.
+    /// guard exists. So this guard ensures that the guard that handed out `new_ref` is inaccessible while
+    /// this guard exists.
+    ///
+    /// Will error if:
+    /// - There is currently no accessible mutable borrow.
+    /// - There are any shared references.
+    /// - `new_ref` is not equal to the pointer in `state`.
     pub(crate) fn new<'b>(
         state: &'a Mutex<CellState<T>>,
         new_ref: &'b mut T,
@@ -205,16 +234,53 @@ impl<'a, T> InaccessibleGuard<'a, T> {
 
         guard.borrow_state.set_inaccessible()?;
         let prev_ptr = guard.get_ptr();
-        guard.set_ptr(new_ptr);
+        let stack_depth = guard.push_ptr(new_ptr);
+
+        Ok(Self {
+            state,
+            stack_depth,
+            prev_ptr,
+        })
+    }
 
-        Ok(Self { state, prev_ptr })
+    /// Single implementation of drop-logic for use in both drop implementations.
+    fn perform_drop(
+        mut state: MutexGuard<'_, CellState<T>>,
+        prev_ptr: NonNull<T>,
+        stack_depth: usize,
+    ) {
+        if state.stack_depth != stack_depth {
+            state
+                .borrow_state
+                .poison("cannot drop inaccessible guards in the wrong order")
+                .unwrap();
+        }
+        state.borrow_state.unset_inaccessible().unwrap();
+        state.pop_ptr(prev_ptr);
+    }
+
+    /// Drop self if possible, otherwise returns self again.
+    ///
+    /// Used currently in the mock-tests, as we need a thread safe way to drop self. Using the normal drop
+    /// logic may poison state, however it should not cause any UB either way.
+    #[doc(hidden)]
+    pub fn try_drop(self) -> Result<(), std::mem::ManuallyDrop<Self>> {
+        let manual = std::mem::ManuallyDrop::new(self);
+        let state = manual.state.lock().unwrap();
+        if !state.borrow_state.may_unset_inaccessible() || state.stack_depth != manual.stack_depth {
+            return Err(manual);
+        }
+        Self::perform_drop(state, manual.prev_ptr, manual.stack_depth);
+
+        Ok(())
     }
 }
 
 impl<'a, T> Drop for InaccessibleGuard<'a, T> {
     fn drop(&mut self) {
-        let mut state = self.state.lock().unwrap();
-        state.borrow_state.unset_inaccessible().unwrap();
-        state.set_ptr(self.prev_ptr);
+        // Default behavior of drop-logic simply panics and poisons the cell on failure. This is appropriate
+        // for single-threaded code where no errors should happen here.
+        let state = self.state.lock().unwrap();
+        Self::perform_drop(state, self.prev_ptr, self.stack_depth);
     }
 }