fix signal error message and add tests (#2118)

packages/generational-box/src/error.rs

@@ -2,7 +2,7 @@ use std::error::Error;
 use std::fmt::Debug;
 use std::fmt::Display;
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 /// An error that can occur when trying to borrow a value.
 pub enum BorrowError {
     /// The value was dropped.
@@ -22,7 +22,7 @@ impl Display for BorrowError {
 
 impl Error for BorrowError {}
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 /// An error that can occur when trying to borrow a value mutably.
 pub enum BorrowMutError {
     /// The value was dropped.
@@ -46,12 +46,23 @@ impl Display for BorrowMutError {
 impl Error for BorrowMutError {}
 
 /// An error that can occur when trying to use a value that has been dropped.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq)]
 pub struct ValueDroppedError {
     #[cfg(any(debug_assertions, feature = "debug_ownership"))]
     pub(crate) created_at: &'static std::panic::Location<'static>,
 }
 
+impl ValueDroppedError {
+    /// Create a new `ValueDroppedError`.
+    #[allow(unused)]
+    pub fn new(created_at: &'static std::panic::Location<'static>) -> Self {
+        Self {
+            #[cfg(any(debug_assertions, feature = "debug_ownership"))]
+            created_at,
+        }
+    }
+}
+
 impl Display for ValueDroppedError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str("Failed to borrow because the value was dropped.")?;
@@ -64,12 +75,23 @@ impl Display for ValueDroppedError {
 impl std::error::Error for ValueDroppedError {}
 
 /// An error that can occur when trying to borrow a value that has already been borrowed mutably.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq)]
 pub struct AlreadyBorrowedMutError {
     #[cfg(any(debug_assertions, feature = "debug_borrows"))]
     pub(crate) borrowed_mut_at: &'static std::panic::Location<'static>,
 }
 
+impl AlreadyBorrowedMutError {
+    /// Create a new `AlreadyBorrowedMutError`.
+    #[allow(unused)]
+    pub fn new(borrowed_mut_at: &'static std::panic::Location<'static>) -> Self {
+        Self {
+            #[cfg(any(debug_assertions, feature = "debug_borrows"))]
+            borrowed_mut_at,
+        }
+    }
+}
+
 impl Display for AlreadyBorrowedMutError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str("Failed to borrow because the value was already borrowed mutably.")?;
@@ -82,12 +104,23 @@ impl Display for AlreadyBorrowedMutError {
 impl std::error::Error for AlreadyBorrowedMutError {}
 
 /// An error that can occur when trying to borrow a value mutably that has already been borrowed immutably.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct AlreadyBorrowedError {
     #[cfg(any(debug_assertions, feature = "debug_borrows"))]
     pub(crate) borrowed_at: Vec<&'static std::panic::Location<'static>>,
 }
 
+impl AlreadyBorrowedError {
+    /// Create a new `AlreadyBorrowedError`.
+    #[allow(unused)]
+    pub fn new(borrowed_at: Vec<&'static std::panic::Location<'static>>) -> Self {
+        Self {
+            #[cfg(any(debug_assertions, feature = "debug_borrows"))]
+            borrowed_at,
+        }
+    }
+}
+
 impl Display for AlreadyBorrowedError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str("Failed to borrow mutably because the value was already borrowed immutably.")?;

packages/generational-box/src/references.rs

@@ -65,7 +65,7 @@ impl Drop for GenerationalRefBorrowInfo {
 pub struct GenerationalRefMut<W> {
     pub(crate) inner: W,
     #[cfg(any(debug_assertions, feature = "debug_borrows"))]
-    pub(crate) borrow: GenerationalRefMutBorrowInfo,
+    pub(crate) borrow: GenerationalRefBorrowMutGuard,
 }
 
 impl<T, R: DerefMut<Target = T>> GenerationalRefMut<R> {
@@ -77,7 +77,7 @@ impl<T, R: DerefMut<Target = T>> GenerationalRefMut<R> {
         Self {
             inner,
             #[cfg(any(debug_assertions, feature = "debug_borrows"))]
-            borrow,
+            borrow: borrow.into(),
         }
     }
 }
@@ -105,8 +105,24 @@ pub struct GenerationalRefMutBorrowInfo {
 }
 
 #[cfg(any(debug_assertions, feature = "debug_borrows"))]
-impl Drop for GenerationalRefMutBorrowInfo {
-    fn drop(&mut self) {
-        self.borrowed_from.borrowed_mut_at.write().take();
+pub(crate) struct GenerationalRefBorrowMutGuard {
+    borrow_info: GenerationalRefMutBorrowInfo,
+}
+
+#[cfg(any(debug_assertions, feature = "debug_borrows"))]
+impl From<GenerationalRefMutBorrowInfo> for GenerationalRefBorrowMutGuard {
+    fn from(borrow_info: GenerationalRefMutBorrowInfo) -> Self {
+        Self { borrow_info }
+    }
+}
+
+#[cfg(any(debug_assertions, feature = "debug_borrows"))]
+impl Drop for GenerationalRefBorrowMutGuard {
+    fn drop(&mut self) {
+        self.borrow_info
+            .borrowed_from
+            .borrowed_mut_at
+            .write()
+            .take();
     }
 }

packages/generational-box/src/sync.rs

@@ -73,10 +73,7 @@ impl AnyStorage for SyncStorage {
             .map(|inner| GenerationalRefMut {
                 inner,
                 #[cfg(any(debug_assertions, feature = "debug_borrows"))]
-                borrow: crate::GenerationalRefMutBorrowInfo {
-                    borrowed_from: borrow.borrowed_from,
-                    created_at: borrow.created_at,
-                },
+                borrow,
             })
     }
 
@@ -147,20 +144,17 @@ impl<T: Sync + Send + 'static> Storage<T> for SyncStorage {
     ) -> Result<Self::Mut<'static, T>, error::BorrowMutError> {
         let write = self.0.write();
 
-        RwLockWriteGuard::try_map(write, |any| any.as_mut()?.downcast_mut())
-            .map_err(|_| {
-                error::BorrowMutError::Dropped(ValueDroppedError {
-                    #[cfg(any(debug_assertions, feature = "debug_ownership"))]
-                    created_at: at.created_at,
-                })
-            })
-            .map(|guard| {
-                GenerationalRefMut::new(
-                    guard,
-                    #[cfg(any(debug_assertions, feature = "debug_ownership"))]
-                    at,
-                )
-            })
+        match RwLockWriteGuard::try_map(write, |any| any.as_mut()?.downcast_mut()) {
+            Ok(guard) => Ok(GenerationalRefMut::new(
+                guard,
+                #[cfg(any(debug_assertions, feature = "debug_ownership"))]
+                at,
+            )),
+            Err(_) => Err(error::BorrowMutError::Dropped(ValueDroppedError {
+                #[cfg(any(debug_assertions, feature = "debug_ownership"))]
+                created_at: at.created_at,
+            })),
+        }
     }
 
     fn set(&self, value: T) {

packages/generational-box/src/unsync.rs

@@ -12,7 +12,6 @@ pub struct UnsyncStorage(RefCell<Option<Box<dyn std::any::Any>>>);
 impl<T: 'static> Storage<T> for UnsyncStorage {
     fn try_read(
         &'static self,
-
         #[cfg(any(debug_assertions, feature = "debug_ownership"))]
         at: crate::GenerationalRefBorrowInfo,
     ) -> Result<Self::Ref<'static, T>, error::BorrowError> {
@@ -26,20 +25,17 @@ impl<T: 'static> Storage<T> for UnsyncStorage {
             error::BorrowError::AlreadyBorrowedMut(error::AlreadyBorrowedMutError {})
         })?;
 
-        Ref::filter_map(borrow, |any| any.as_ref()?.downcast_ref())
-            .map_err(|_| {
-                error::BorrowError::Dropped(error::ValueDroppedError {
-                    #[cfg(any(debug_assertions, feature = "debug_ownership"))]
-                    created_at: at.created_at,
-                })
-            })
-            .map(|guard| {
-                GenerationalRef::new(
-                    guard,
-                    #[cfg(any(debug_assertions, feature = "debug_ownership"))]
-                    at,
-                )
-            })
+        match Ref::filter_map(borrow, |any| any.as_ref()?.downcast_ref()) {
+            Ok(guard) => Ok(GenerationalRef::new(
+                guard,
+                #[cfg(any(debug_assertions, feature = "debug_ownership"))]
+                at,
+            )),
+            Err(_) => Err(error::BorrowError::Dropped(error::ValueDroppedError {
+                #[cfg(any(debug_assertions, feature = "debug_ownership"))]
+                created_at: at.created_at,
+            })),
+        }
     }
 
     fn try_write(
@@ -136,10 +132,7 @@ impl AnyStorage for UnsyncStorage {
             .map(|inner| GenerationalRefMut {
                 inner,
                 #[cfg(any(debug_assertions, feature = "debug_borrows"))]
-                borrow: crate::GenerationalRefMutBorrowInfo {
-                    borrowed_from: borrow.borrowed_from,
-                    created_at: borrow.created_at,
-                },
+                borrow,
             })
     }
 

packages/generational-box/tests/errors.rs

@@ -0,0 +1,99 @@
+use std::cell::{Ref, RefMut};
+
+use generational_box::{
+    AlreadyBorrowedError, AlreadyBorrowedMutError, AnyStorage, BorrowError, BorrowMutError,
+    GenerationalBox, GenerationalRef, GenerationalRefMut, Owner, UnsyncStorage, ValueDroppedError,
+};
+
+#[track_caller]
+fn read_at_location(
+    value: GenerationalBox<i32>,
+) -> (
+    GenerationalRef<Ref<'static, i32>>,
+    &'static std::panic::Location<'static>,
+) {
+    let location = std::panic::Location::caller();
+    let read = value.read();
+    (read, location)
+}
+
+#[track_caller]
+fn write_at_location(
+    value: GenerationalBox<i32>,
+) -> (
+    GenerationalRefMut<RefMut<'static, i32>>,
+    &'static std::panic::Location<'static>,
+) {
+    let location = std::panic::Location::caller();
+    let write = value.write();
+    (write, location)
+}
+
+#[track_caller]
+fn create_at_location(
+    owner: &Owner,
+) -> (GenerationalBox<i32>, &'static std::panic::Location<'static>) {
+    let location = std::panic::Location::caller();
+    let value = owner.insert(1);
+    (value, location)
+}
+
+#[test]
+fn read_while_writing_error() {
+    let owner = UnsyncStorage::owner();
+    let value = owner.insert(1);
+
+    let (write, location) = write_at_location(value);
+
+    assert_eq!(
+        value.try_read().err(),
+        Some(BorrowError::AlreadyBorrowedMut(
+            AlreadyBorrowedMutError::new(location)
+        ))
+    );
+    drop(write);
+}
+
+#[test]
+fn read_after_dropped_error() {
+    let owner = UnsyncStorage::owner();
+    let (value, location) = create_at_location(&owner);
+    drop(owner);
+    assert_eq!(
+        value.try_read().err(),
+        Some(BorrowError::Dropped(ValueDroppedError::new(location)))
+    );
+}
+
+#[test]
+fn write_while_writing_error() {
+    let owner = UnsyncStorage::owner();
+    let value = owner.insert(1);
+
+    let (write, location) = write_at_location(value);
+
+    assert_eq!(
+        value.try_write().err(),
+        Some(BorrowMutError::AlreadyBorrowedMut(
+            AlreadyBorrowedMutError::new(location)
+        ))
+    );
+    drop(write);
+}
+
+#[test]
+fn write_while_reading_error() {
+    let owner = UnsyncStorage::owner();
+    let value = owner.insert(1);
+
+    let (read, location) = read_at_location(value);
+
+    assert_eq!(
+        value.try_write().err(),
+        Some(BorrowMutError::AlreadyBorrowed(AlreadyBorrowedError::new(
+            vec![location]
+        )))
+    );
+
+    drop(read);
+}

packages/signals/examples/errors.rs

@@ -1,75 +0,0 @@
-use dioxus::prelude::*;
-
-fn main() {
-    launch(app);
-}
-
-#[derive(Clone, Copy)]
-enum ErrorComponent {
-    Read,
-    ReadMut,
-    ReadDropped,
-}
-
-fn app() -> Element {
-    let mut error = use_signal(|| None as Option<ErrorComponent>);
-
-    rsx! {
-        match error() {
-            Some(ErrorComponent::Read) => rsx! { Read {} },
-            Some(ErrorComponent::ReadMut) => rsx! { ReadMut {} },
-            Some(ErrorComponent::ReadDropped) => rsx! { ReadDropped {} },
-            None => rsx! {
-                button { onclick: move |_| error.set(Some(ErrorComponent::Read)), "Read" }
-                button { onclick: move |_| error.set(Some(ErrorComponent::ReadMut)), "ReadMut" }
-                button { onclick: move |_| error.set(Some(ErrorComponent::ReadDropped)), "ReadDropped"}
-            }
-        }
-    }
-}
-
-#[component]
-fn Read() -> Element {
-    let signal = use_signal_sync(|| 0);
-
-    let _write = signal.write_unchecked();
-    let _read = signal.read();
-
-    unreachable!()
-}
-
-#[component]
-fn ReadMut() -> Element {
-    let signal = use_signal_sync(|| 0);
-
-    let _read = signal.read();
-    let _write = signal.write_unchecked();
-
-    unreachable!()
-}
-
-#[component]
-fn ReadDropped() -> Element {
-    let signal = use_signal_sync(|| None as Option<SyncSignal<i32>>);
-
-    if generation() < 4 {
-        needs_update();
-    }
-
-    rsx! {
-        if let Some(value) = signal() {
-            "{value:?}"
-        } else {
-            ReadDroppedSignalChild { parent_signal: signal }
-        }
-    }
-}
-
-#[component]
-fn ReadDroppedSignalChild(parent_signal: SyncSignal<Option<SyncSignal<i32>>>) -> Element {
-    let signal = use_signal_sync(|| 0);
-
-    use_hook(move || parent_signal.set(Some(signal)));
-
-    rsx! { "{signal}" }
-}

packages/signals/src/copy_value.rs

@@ -121,12 +121,14 @@ impl<T: 'static, S: Storage<T>> Readable for CopyValue<T, S> {
     type Target = T;
     type Storage = S;
 
+    #[track_caller]
     fn try_read_unchecked(
         &self,
     ) -> Result<ReadableRef<'static, Self>, generational_box::BorrowError> {
         self.value.try_read()
     }
 
+    #[track_caller]
     fn peek_unchecked(&self) -> ReadableRef<'static, Self> {
         self.value.read()
     }

packages/signals/src/read.rs

@@ -67,6 +67,7 @@ pub trait Readable {
     /// Get a reference to the value without checking the lifetime. This will subscribe the current scope to the signal.
     ///
     /// NOTE: This method is completely safe because borrow checking is done at runtime.
+    #[track_caller]
     fn read_unchecked(&self) -> ReadableRef<'static, Self> {
         self.try_read_unchecked().unwrap()
     }

packages/signals/src/write.rs

@@ -52,6 +52,7 @@ pub trait Writable: Readable {
     /// Get a mutable reference to the value without checking the lifetime. This will update any subscribers.
     ///
     /// NOTE: This method is completely safe because borrow checking is done at runtime.
+    #[track_caller]
     fn write_unchecked(&self) -> WritableRef<'static, Self> {
         self.try_write_unchecked().unwrap()
     }
@@ -114,11 +115,13 @@ pub trait Writable: Readable {
 /// An extension trait for Writable<Option<T>> that provides some convenience methods.
 pub trait WritableOptionExt<T: 'static>: Writable<Target = Option<T>> {
     /// Gets the value out of the Option, or inserts the given value if the Option is empty.
+    #[track_caller]
     fn get_or_insert(&mut self, default: T) -> WritableRef<'_, Self, T> {
         self.get_or_insert_with(|| default)
     }
 
     /// Gets the value out of the Option, or inserts the value returned by the given function if the Option is empty.
+    #[track_caller]
     fn get_or_insert_with(&mut self, default: impl FnOnce() -> T) -> WritableRef<'_, Self, T> {
         let is_none = self.read().is_none();
         if is_none {