From 9f283f571fa429e542969c5775178ad1b27dd9b5 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Wed, 20 Mar 2024 10:45:27 -0500 Subject: [PATCH] fix signal error message and add tests (#2118) --- packages/generational-box/src/error.rs | 43 +++++++-- packages/generational-box/src/owner.rs | 0 packages/generational-box/src/references.rs | 24 ++++- packages/generational-box/src/sync.rs | 30 +++---- packages/generational-box/src/unsync.rs | 31 +++---- packages/generational-box/tests/errors.rs | 99 +++++++++++++++++++++ packages/signals/examples/errors.rs | 75 ---------------- packages/signals/src/copy_value.rs | 2 + packages/signals/src/read.rs | 1 + packages/signals/src/write.rs | 3 + 10 files changed, 187 insertions(+), 121 deletions(-) delete mode 100644 packages/generational-box/src/owner.rs create mode 100644 packages/generational-box/tests/errors.rs delete mode 100644 packages/signals/examples/errors.rs diff --git a/packages/generational-box/src/error.rs b/packages/generational-box/src/error.rs index 9aef73901..1cba7e6a1 100644 --- a/packages/generational-box/src/error.rs +++ b/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.")?; diff --git a/packages/generational-box/src/owner.rs b/packages/generational-box/src/owner.rs deleted file mode 100644 index e69de29bb..000000000 diff --git a/packages/generational-box/src/references.rs b/packages/generational-box/src/references.rs index 236e1cc82..3589d6647 100644 --- a/packages/generational-box/src/references.rs +++ b/packages/generational-box/src/references.rs @@ -65,7 +65,7 @@ impl Drop for GenerationalRefBorrowInfo { pub struct GenerationalRefMut { pub(crate) inner: W, #[cfg(any(debug_assertions, feature = "debug_borrows"))] - pub(crate) borrow: GenerationalRefMutBorrowInfo, + pub(crate) borrow: GenerationalRefBorrowMutGuard, } impl> GenerationalRefMut { @@ -77,7 +77,7 @@ impl> GenerationalRefMut { 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 { +pub(crate) struct GenerationalRefBorrowMutGuard { + borrow_info: GenerationalRefMutBorrowInfo, +} + +#[cfg(any(debug_assertions, feature = "debug_borrows"))] +impl From 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.borrowed_from.borrowed_mut_at.write().take(); + self.borrow_info + .borrowed_from + .borrowed_mut_at + .write() + .take(); } } diff --git a/packages/generational-box/src/sync.rs b/packages/generational-box/src/sync.rs index 92eee44a4..6f7149324 100644 --- a/packages/generational-box/src/sync.rs +++ b/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 Storage for SyncStorage { ) -> Result, 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) { diff --git a/packages/generational-box/src/unsync.rs b/packages/generational-box/src/unsync.rs index ed0f791f3..dc609e72a 100644 --- a/packages/generational-box/src/unsync.rs +++ b/packages/generational-box/src/unsync.rs @@ -12,7 +12,6 @@ pub struct UnsyncStorage(RefCell>>); impl Storage for UnsyncStorage { fn try_read( &'static self, - #[cfg(any(debug_assertions, feature = "debug_ownership"))] at: crate::GenerationalRefBorrowInfo, ) -> Result, error::BorrowError> { @@ -26,20 +25,17 @@ impl Storage 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, }) } diff --git a/packages/generational-box/tests/errors.rs b/packages/generational-box/tests/errors.rs new file mode 100644 index 000000000..eaac414c8 --- /dev/null +++ b/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, +) -> ( + GenerationalRef>, + &'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, +) -> ( + GenerationalRefMut>, + &'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, &'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); +} diff --git a/packages/signals/examples/errors.rs b/packages/signals/examples/errors.rs deleted file mode 100644 index 9f1797c9d..000000000 --- a/packages/signals/examples/errors.rs +++ /dev/null @@ -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); - - 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>); - - if generation() < 4 { - needs_update(); - } - - rsx! { - if let Some(value) = signal() { - "{value:?}" - } else { - ReadDroppedSignalChild { parent_signal: signal } - } - } -} - -#[component] -fn ReadDroppedSignalChild(parent_signal: SyncSignal>>) -> Element { - let signal = use_signal_sync(|| 0); - - use_hook(move || parent_signal.set(Some(signal))); - - rsx! { "{signal}" } -} diff --git a/packages/signals/src/copy_value.rs b/packages/signals/src/copy_value.rs index b8fd5d859..962239373 100644 --- a/packages/signals/src/copy_value.rs +++ b/packages/signals/src/copy_value.rs @@ -121,12 +121,14 @@ impl> Readable for CopyValue { type Target = T; type Storage = S; + #[track_caller] fn try_read_unchecked( &self, ) -> Result, generational_box::BorrowError> { self.value.try_read() } + #[track_caller] fn peek_unchecked(&self) -> ReadableRef<'static, Self> { self.value.read() } diff --git a/packages/signals/src/read.rs b/packages/signals/src/read.rs index 8b01d2cd7..2f1aef085 100644 --- a/packages/signals/src/read.rs +++ b/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() } diff --git a/packages/signals/src/write.rs b/packages/signals/src/write.rs index cd8c91043..88fe81f85 100644 --- a/packages/signals/src/write.rs +++ b/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> that provides some convenience methods. pub trait WritableOptionExt: Writable> { /// 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 {