From 469a7ac0afc4d63768ab21fb692f0f4008bfe084 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Tue, 11 Jul 2023 21:41:52 +0200 Subject: [PATCH 01/11] better diagnostics for use_shared_state --- Cargo.toml | 3 +- packages/hooks/Cargo.toml | 1 + packages/hooks/src/use_shared_state.rs | 78 ++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d69a74828..4f6561e4d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ futures-util = { version = "0.3", default-features = false } rustc-hash = "1.1.0" wasm-bindgen = "0.2.87" html_parser = "0.7.0" +thiserror = "1.0.30" # This is a "virtual package" # It is not meant to be published, but is used so "cargo run --example XYZ" works properly @@ -112,9 +113,9 @@ rand = { version = "0.8.4", features = ["small_rng"] } tokio = { version = "1.16.1", features = ["full"] } reqwest = { version = "0.11.9", features = ["json"] } fern = { version = "0.6.0", features = ["colored"] } -thiserror = "1.0.30" env_logger = "0.10.0" simple_logger = "4.0.0" +thiserror = { workspace = true } [profile.release] opt-level = 3 diff --git a/packages/hooks/Cargo.toml b/packages/hooks/Cargo.toml index 6a3205c68..198cdb74e 100644 --- a/packages/hooks/Cargo.toml +++ b/packages/hooks/Cargo.toml @@ -14,6 +14,7 @@ keywords = ["dom", "ui", "gui", "react"] dioxus-core = { workspace = true } futures-channel = { workspace = true } log = { workspace = true } +thiserror = { workspace = true } [dev-dependencies] diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index 3a3a4d639..c994e4817 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -121,30 +121,96 @@ pub struct UseSharedState { pub(crate) inner: Rc>>, } +#[derive(thiserror::Error, Debug)] +pub enum UseSharedStateError { + #[error("{type_name} is already borrowed, so it cannot be borrowed mutably.")] + AlreadyBorrowed { + source: core::cell::BorrowMutError, + type_name: &'static str, + }, + #[error("{type_name} is already borrowed mutably, so it cannot be borrowed anymore.")] + AlreadyBorrowedMutably { + source: core::cell::BorrowError, + type_name: &'static str, + }, +} + +pub type UseSharedStateResult = Result; + impl UseSharedState { /// Notify all consumers of the state that it has changed. (This is called automatically when you call "write") pub fn notify_consumers(&self) { self.inner.borrow_mut().notify_consumers(); } + /// Try reading the shared state + pub fn try_read(&self) -> UseSharedStateResult> { + self.inner + .try_borrow() + .map_err(|source| UseSharedStateError::AlreadyBorrowedMutably { + source, + type_name: std::any::type_name::(), + }) + .map(|value| Ref::map(value, |inner| &inner.value)) + } /// Read the shared value pub fn read(&self) -> Ref<'_, T> { - Ref::map(self.inner.borrow(), |inner| &inner.value) + match self.try_read() { + Ok(value) => value, + Err(message) => panic!( + "Reading the shared state failed: {}\n({:?})", + message, message + ), + } } + /// Try writing the shared state + pub fn try_write(&self) -> UseSharedStateResult> { + self.inner + .try_borrow_mut() + .map_err(|source| UseSharedStateError::AlreadyBorrowed { + source, + type_name: std::any::type_name::(), + }) + .map(|mut value| { + value.notify_consumers(); + RefMut::map(value, |inner| &mut inner.value) + }) + } /// Calling "write" will force the component to re-render /// /// // TODO: We prevent unncessary notifications only in the hook, but we should figure out some more global lock pub fn write(&self) -> RefMut<'_, T> { - let mut value = self.inner.borrow_mut(); - value.notify_consumers(); - RefMut::map(value, |inner| &mut inner.value) + match self.try_write() { + Ok(value) => value, + Err(message) => panic!( + "Writing to shared state failed: {}\n({:?})", + message, message + ), + } } - /// Allows the ability to write the value without forcing a re-render + /// Tries writing the value without forcing a re-render + pub fn try_write_silent(&self) -> UseSharedStateResult> { + self.inner + .try_borrow_mut() + .map_err(|source| UseSharedStateError::AlreadyBorrowed { + source, + type_name: std::any::type_name::(), + }) + .map(|value| RefMut::map(value, |inner| &mut inner.value)) + } + + /// Writes the value without forcing a re-render pub fn write_silent(&self) -> RefMut<'_, T> { - RefMut::map(self.inner.borrow_mut(), |inner| &mut inner.value) + match self.try_write_silent() { + Ok(value) => value, + Err(message) => panic!( + "Writing to shared state silently failed: {}\n({:?})", + message, message + ), + } } } From f159779af4b33e9d686f610b76e7ae1b7a62c9eb Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Tue, 11 Jul 2023 22:54:24 +0200 Subject: [PATCH 02/11] make thiserror a workspace dependency everywhere else --- Cargo.toml | 2 +- packages/cli/Cargo.toml | 2 +- packages/desktop/Cargo.toml | 2 +- packages/dioxus/Cargo.toml | 2 +- packages/fullstack/Cargo.toml | 2 +- packages/hooks/src/use_shared_state.rs | 2 ++ packages/liveview/Cargo.toml | 2 +- packages/router/Cargo.toml | 2 +- packages/ssr/Cargo.toml | 2 +- 9 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4f6561e4d..13e8ee2ab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,7 +76,7 @@ futures-util = { version = "0.3", default-features = false } rustc-hash = "1.1.0" wasm-bindgen = "0.2.87" html_parser = "0.7.0" -thiserror = "1.0.30" +thiserror = "1.0.40" # This is a "virtual package" # It is not meant to be published, but is used so "cargo run --example XYZ" works properly diff --git a/packages/cli/Cargo.toml b/packages/cli/Cargo.toml index 7e5e60eed..5fa8827b1 100644 --- a/packages/cli/Cargo.toml +++ b/packages/cli/Cargo.toml @@ -10,7 +10,7 @@ license = "MIT/Apache-2.0" [dependencies] # cli core clap = { version = "4.2", features = ["derive"] } -thiserror = "1.0.30" +thiserror = { workspace = true } wasm-bindgen-cli-support = "0.2" colored = "2.0.0" diff --git a/packages/desktop/Cargo.toml b/packages/desktop/Cargo.toml index 05c746a52..c537aaed8 100644 --- a/packages/desktop/Cargo.toml +++ b/packages/desktop/Cargo.toml @@ -17,7 +17,7 @@ dioxus-hot-reload = { workspace = true, optional = true } serde = "1.0.136" serde_json = "1.0.79" -thiserror = "1.0.30" +thiserror = { workspace = true } log = { workspace = true } wry = { version = "0.28.0" } futures-channel = { workspace = true } diff --git a/packages/dioxus/Cargo.toml b/packages/dioxus/Cargo.toml index b6bfe6536..fedf65894 100644 --- a/packages/dioxus/Cargo.toml +++ b/packages/dioxus/Cargo.toml @@ -33,7 +33,7 @@ futures-util = { workspace = true } log = { workspace = true } rand = { version = "0.8.4", features = ["small_rng"] } criterion = "0.3.5" -thiserror = "1.0.30" +thiserror = { workspace = true } env_logger = "0.10.0" tokio = { workspace = true, features = ["full"] } # dioxus-edit-stream = { workspace = true } diff --git a/packages/fullstack/Cargo.toml b/packages/fullstack/Cargo.toml index a6a1d29fe..ef3337c67 100644 --- a/packages/fullstack/Cargo.toml +++ b/packages/fullstack/Cargo.toml @@ -34,7 +34,7 @@ http = { version = "0.2.9", optional = true } log = { workspace = true } once_cell = "1.17.1" -thiserror = "1.0.40" +thiserror = { workspace = true } tokio = { workspace = true, features = ["full"], optional = true } object-pool = "0.5.4" anymap = "0.12.1" diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index c994e4817..a5762523e 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -153,6 +153,7 @@ impl UseSharedState { }) .map(|value| Ref::map(value, |inner| &inner.value)) } + /// Read the shared value pub fn read(&self) -> Ref<'_, T> { match self.try_read() { @@ -177,6 +178,7 @@ impl UseSharedState { RefMut::map(value, |inner| &mut inner.value) }) } + /// Calling "write" will force the component to re-render /// /// diff --git a/packages/liveview/Cargo.toml b/packages/liveview/Cargo.toml index 28a4e4a06..210b72b53 100644 --- a/packages/liveview/Cargo.toml +++ b/packages/liveview/Cargo.toml @@ -9,7 +9,7 @@ description = "Build server-side apps with Dioxus" license = "MIT/Apache-2.0" [dependencies] -thiserror = "1.0.38" +thiserror = { workspace = true } log = { workspace = true } slab = { workspace = true } futures-util = { workspace = true, default-features = false, features = [ diff --git a/packages/router/Cargo.toml b/packages/router/Cargo.toml index 7015c3243..2a0afbc20 100644 --- a/packages/router/Cargo.toml +++ b/packages/router/Cargo.toml @@ -30,7 +30,7 @@ wasm-bindgen = { workspace = true, optional = true } js-sys = { version = "0.3", optional = true } gloo-events = { version = "0.1.1", optional = true } log = { workspace = true } -thiserror = "1.0.30" +thiserror = { workspace = true } futures-util = { workspace = true } serde = { version = "1", optional = true } serde_urlencoded = { version = "0.7.1", optional = true } diff --git a/packages/ssr/Cargo.toml b/packages/ssr/Cargo.toml index 98e127be0..e91ee25eb 100644 --- a/packages/ssr/Cargo.toml +++ b/packages/ssr/Cargo.toml @@ -15,7 +15,7 @@ askama_escape = "0.10.3" [dev-dependencies] dioxus = { workspace = true } -thiserror = "1.0.23" +thiserror = { workspace = true } log = { workspace = true } fern = { version = "0.6.0", features = ["colored"] } anyhow = "1.0" From 43e3304eaf4c3bc640e30cde0ab0091595925963 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 01:19:20 +0200 Subject: [PATCH 03/11] track caller for easier debugging --- packages/hooks/src/use_shared_state.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index a5762523e..753f194fa 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -2,6 +2,7 @@ use dioxus_core::{ScopeId, ScopeState}; use std::{ cell::{Ref, RefCell, RefMut}, collections::HashSet, + panic::Location, rc::Rc, sync::Arc, }; @@ -123,15 +124,17 @@ pub struct UseSharedState { #[derive(thiserror::Error, Debug)] pub enum UseSharedStateError { - #[error("{type_name} is already borrowed, so it cannot be borrowed mutably.")] + #[error("[{caller}] {type_name} is already borrowed, so it cannot be borrowed mutably.")] AlreadyBorrowed { source: core::cell::BorrowMutError, type_name: &'static str, + caller: &'static Location<'static>, }, - #[error("{type_name} is already borrowed mutably, so it cannot be borrowed anymore.")] + #[error("[caller] {type_name} is already borrowed mutably, so it cannot be borrowed anymore.")] AlreadyBorrowedMutably { source: core::cell::BorrowError, type_name: &'static str, + caller: &'static Location<'static>, }, } @@ -144,17 +147,20 @@ impl UseSharedState { } /// Try reading the shared state + #[track_caller] pub fn try_read(&self) -> UseSharedStateResult> { self.inner .try_borrow() .map_err(|source| UseSharedStateError::AlreadyBorrowedMutably { source, type_name: std::any::type_name::(), + caller: Location::caller(), }) .map(|value| Ref::map(value, |inner| &inner.value)) } /// Read the shared value + #[track_caller] pub fn read(&self) -> Ref<'_, T> { match self.try_read() { Ok(value) => value, @@ -166,12 +172,14 @@ impl UseSharedState { } /// Try writing the shared state + #[track_caller] pub fn try_write(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() .map_err(|source| UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), + caller: Location::caller(), }) .map(|mut value| { value.notify_consumers(); @@ -183,6 +191,7 @@ impl UseSharedState { /// /// // TODO: We prevent unncessary notifications only in the hook, but we should figure out some more global lock + #[track_caller] pub fn write(&self) -> RefMut<'_, T> { match self.try_write() { Ok(value) => value, @@ -194,17 +203,20 @@ impl UseSharedState { } /// Tries writing the value without forcing a re-render + #[track_caller] pub fn try_write_silent(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() .map_err(|source| UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), + caller: Location::caller(), }) .map(|value| RefMut::map(value, |inner| &mut inner.value)) } /// Writes the value without forcing a re-render + #[track_caller] pub fn write_silent(&self) -> RefMut<'_, T> { match self.try_write_silent() { Ok(value) => value, From b8c14ce35c1d7a7acab3a9f7e7936a3498c093ea Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 01:38:50 +0200 Subject: [PATCH 04/11] make location diagnostics optional (only available in debug mode) --- packages/hooks/src/use_shared_state.rs | 75 ++++++++++++++++++-------- 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index 753f194fa..311f9a87f 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -1,12 +1,61 @@ +use self::error::{UseSharedStateError, UseSharedStateResult}; use dioxus_core::{ScopeId, ScopeState}; use std::{ cell::{Ref, RefCell, RefMut}, collections::HashSet, - panic::Location, rc::Rc, sync::Arc, }; +pub mod error { + use std::panic::Location; + + #[macro_export] + macro_rules! debug_location { + () => {{ + #[cfg(debug_assertions)] + { + Some(std::panic::Location::caller()) + } + #[cfg(not(debug_assertions))] + { + None + } + }}; + } + fn display_location(location: Option<&&Location<'_>>) -> String { + location + .map(|location| format!("[{location}] ")) + .unwrap_or_else(String::new) + } + + #[derive(thiserror::Error, Debug)] + pub enum UseSharedStateError { + #[error( + "{}{type_name} is already borrowed, so it cannot be borrowed mutably.", + display_location(.location.as_ref()) + )] + AlreadyBorrowed { + source: core::cell::BorrowMutError, + type_name: &'static str, + /// Only available in debug mode + location: Option<&'static Location<'static>>, + }, + #[error( + "{}{type_name} is already borrowed mutably, so it cannot be borrowed anymore.", + display_location(.location.as_ref()) + )] + AlreadyBorrowedMutably { + source: core::cell::BorrowError, + type_name: &'static str, + /// Only available in debug mode + location: Option<&'static Location<'static>>, + }, + } + + pub type UseSharedStateResult = Result; +} + type ProvidedState = Rc>>; // Tracks all the subscribers to a shared State @@ -122,24 +171,6 @@ pub struct UseSharedState { pub(crate) inner: Rc>>, } -#[derive(thiserror::Error, Debug)] -pub enum UseSharedStateError { - #[error("[{caller}] {type_name} is already borrowed, so it cannot be borrowed mutably.")] - AlreadyBorrowed { - source: core::cell::BorrowMutError, - type_name: &'static str, - caller: &'static Location<'static>, - }, - #[error("[caller] {type_name} is already borrowed mutably, so it cannot be borrowed anymore.")] - AlreadyBorrowedMutably { - source: core::cell::BorrowError, - type_name: &'static str, - caller: &'static Location<'static>, - }, -} - -pub type UseSharedStateResult = Result; - impl UseSharedState { /// Notify all consumers of the state that it has changed. (This is called automatically when you call "write") pub fn notify_consumers(&self) { @@ -154,7 +185,7 @@ impl UseSharedState { .map_err(|source| UseSharedStateError::AlreadyBorrowedMutably { source, type_name: std::any::type_name::(), - caller: Location::caller(), + location: crate::debug_location!(), }) .map(|value| Ref::map(value, |inner| &inner.value)) } @@ -179,7 +210,7 @@ impl UseSharedState { .map_err(|source| UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), - caller: Location::caller(), + location: crate::debug_location!(), }) .map(|mut value| { value.notify_consumers(); @@ -210,7 +241,7 @@ impl UseSharedState { .map_err(|source| UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), - caller: Location::caller(), + location: crate::debug_location!(), }) .map(|value| RefMut::map(value, |inner| &mut inner.value)) } From 9da32d54651c6e958a1bc283f8e8378a59dbaee2 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 01:53:00 +0200 Subject: [PATCH 05/11] oh yeah, tracking itself should also be debug-only --- packages/hooks/src/use_shared_state.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index 311f9a87f..fbf16f790 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -178,7 +178,7 @@ impl UseSharedState { } /// Try reading the shared state - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn try_read(&self) -> UseSharedStateResult> { self.inner .try_borrow() @@ -191,7 +191,7 @@ impl UseSharedState { } /// Read the shared value - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn read(&self) -> Ref<'_, T> { match self.try_read() { Ok(value) => value, @@ -203,7 +203,7 @@ impl UseSharedState { } /// Try writing the shared state - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn try_write(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() @@ -222,7 +222,7 @@ impl UseSharedState { /// /// // TODO: We prevent unncessary notifications only in the hook, but we should figure out some more global lock - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn write(&self) -> RefMut<'_, T> { match self.try_write() { Ok(value) => value, @@ -234,7 +234,7 @@ impl UseSharedState { } /// Tries writing the value without forcing a re-render - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn try_write_silent(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() @@ -247,7 +247,7 @@ impl UseSharedState { } /// Writes the value without forcing a re-render - #[track_caller] + #[cfg_attr(debug_assertions, track_caller)] pub fn write_silent(&self) -> RefMut<'_, T> { match self.try_write_silent() { Ok(value) => value, From 760d93716faad8240c92f1cf5333746b9fa9703c Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 02:53:48 +0200 Subject: [PATCH 06/11] also track previous caller in the in debug mode to show the conflicting borrow --- packages/hooks/Cargo.toml | 1 + packages/hooks/src/use_shared_state.rs | 157 ++++++++++++++++++++----- 2 files changed, 126 insertions(+), 32 deletions(-) diff --git a/packages/hooks/Cargo.toml b/packages/hooks/Cargo.toml index 198cdb74e..b2dd4898f 100644 --- a/packages/hooks/Cargo.toml +++ b/packages/hooks/Cargo.toml @@ -21,3 +21,4 @@ thiserror = { workspace = true } futures-util = { workspace = true, default-features = false } dioxus-core = { workspace = true } dioxus = { workspace = true } +debug-cell = "0.1.1" diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index fbf16f790..8e9a05c29 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -7,49 +7,117 @@ use std::{ sync::Arc, }; -pub mod error { +#[cfg(not(debug_assertions))] +type Location = (); + +#[cfg(debug_assertions)] +type Location = &'static std::panic::Location<'static>; + +#[macro_export] +macro_rules! debug_location { + () => {{ + #[cfg(debug_assertions)] + { + std::panic::Location::caller() + } + #[cfg(not(debug_assertions))] + { + () + } + }}; +} + +pub mod diagnostics { use std::panic::Location; - #[macro_export] - macro_rules! debug_location { - () => {{ - #[cfg(debug_assertions)] - { - Some(std::panic::Location::caller()) - } - #[cfg(not(debug_assertions))] - { - None - } - }}; - } - fn display_location(location: Option<&&Location<'_>>) -> String { - location - .map(|location| format!("[{location}] ")) - .unwrap_or_else(String::new) + #[derive(Debug, Clone, Copy)] + pub enum BorrowKind { + Mutable, + Immutable, } + #[derive(Debug, Clone, Copy)] + pub struct PreviousBorrow { + pub location: Location<'static>, + pub kind: BorrowKind, + } + + impl super::UseSharedState { + #[cfg_attr(debug_assertions, track_caller)] + #[allow(unused_must_use)] + pub(super) fn debug_track_borrow(&self) { + #[cfg(debug_assertions)] + self.previous_borrow + .borrow_mut() + .insert(PreviousBorrow::borrowed()); + } + + #[cfg_attr(debug_assertions, track_caller)] + #[allow(unused_must_use)] + pub(super) fn debug_track_borrow_mut(&self) { + #[cfg(debug_assertions)] + self.previous_borrow + .borrow_mut() + .insert(PreviousBorrow::borrowed_mut()); + } + } + impl PreviousBorrow { + #[track_caller] + pub fn borrowed() -> Self { + Self { + location: *Location::caller(), + kind: BorrowKind::Immutable, + } + } + + #[track_caller] + pub fn borrowed_mut() -> Self { + Self { + location: *Location::caller(), + kind: BorrowKind::Mutable, + } + } + } +} + +pub mod error { #[derive(thiserror::Error, Debug)] pub enum UseSharedStateError { - #[error( - "{}{type_name} is already borrowed, so it cannot be borrowed mutably.", - display_location(.location.as_ref()) - )] + #[cfg_attr( + debug_assertions, + error( + "[{location}] {type_name} is already borrowed at [{previous_borrow:?}], so it cannot be borrowed mutably." + ) + )] + #[cfg_attr( + not(debug_assertions), + error("{type_name} is already borrowed, so it cannot be borrowed mutably.") + )] AlreadyBorrowed { source: core::cell::BorrowMutError, type_name: &'static str, /// Only available in debug mode - location: Option<&'static Location<'static>>, + location: super::Location, + #[cfg(debug_assertions)] + previous_borrow: Option, }, - #[error( - "{}{type_name} is already borrowed mutably, so it cannot be borrowed anymore.", - display_location(.location.as_ref()) - )] + #[cfg_attr( + debug_assertions, + error( + "[{location}] {type_name} is already borrowed mutably at [{previous_borrow:?}], so it cannot be borrowed anymore." + ) + )] + #[cfg_attr( + not(debug_assertions), + error("{type_name} is already borrowed mutably, so it cannot be borrowed anymore.") + )] AlreadyBorrowedMutably { source: core::cell::BorrowError, type_name: &'static str, /// Only available in debug mode - location: Option<&'static Location<'static>>, + location: super::Location, + #[cfg(debug_assertions)] + previous_borrow: Option, }, } @@ -145,7 +213,7 @@ pub fn use_shared_state(cx: &ScopeState) -> Option<&UseSharedState Drop for UseSharedStateOwner { /// State that is shared between components through the context system pub struct UseSharedState { pub(crate) inner: Rc>>, + #[cfg(debug_assertions)] + previous_borrow: Rc>>, } impl UseSharedState { + fn new(inner: Rc>>) -> Self { + Self { + inner, + #[cfg(debug_assertions)] + previous_borrow: Default::default(), + } + } + /// Notify all consumers of the state that it has changed. (This is called automatically when you call "write") pub fn notify_consumers(&self) { self.inner.borrow_mut().notify_consumers(); @@ -185,9 +263,14 @@ impl UseSharedState { .map_err(|source| UseSharedStateError::AlreadyBorrowedMutably { source, type_name: std::any::type_name::(), - location: crate::debug_location!(), + location: debug_location!(), + #[cfg(debug_assertions)] + previous_borrow: *self.previous_borrow.borrow(), + }) + .map(|value| { + self.debug_track_borrow(); + Ref::map(value, |inner| &inner.value) }) - .map(|value| Ref::map(value, |inner| &inner.value)) } /// Read the shared value @@ -211,9 +294,12 @@ impl UseSharedState { source, type_name: std::any::type_name::(), location: crate::debug_location!(), + #[cfg(debug_assertions)] + previous_borrow: *self.previous_borrow.borrow(), }) .map(|mut value| { value.notify_consumers(); + self.debug_track_borrow_mut(); RefMut::map(value, |inner| &mut inner.value) }) } @@ -242,8 +328,13 @@ impl UseSharedState { source, type_name: std::any::type_name::(), location: crate::debug_location!(), + #[cfg(debug_assertions)] + previous_borrow: *self.previous_borrow.borrow(), + }) + .map(|value| { + self.debug_track_borrow_mut(); + RefMut::map(value, |inner| &mut inner.value) }) - .map(|value| RefMut::map(value, |inner| &mut inner.value)) } /// Writes the value without forcing a re-render @@ -263,6 +354,8 @@ impl Clone for UseSharedState { fn clone(&self) -> Self { Self { inner: self.inner.clone(), + #[cfg(debug_assertions)] + previous_borrow: Default::default(), } } } From 3fa519563e08f9fcd09af8304d59874d717932f3 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 12:27:17 +0200 Subject: [PATCH 07/11] prevent inlining --- packages/hooks/src/use_shared_state.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index 8e9a05c29..cc1679b09 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -44,6 +44,7 @@ pub mod diagnostics { impl super::UseSharedState { #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] #[allow(unused_must_use)] pub(super) fn debug_track_borrow(&self) { #[cfg(debug_assertions)] @@ -53,6 +54,7 @@ pub mod diagnostics { } #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] #[allow(unused_must_use)] pub(super) fn debug_track_borrow_mut(&self) { #[cfg(debug_assertions)] @@ -63,6 +65,7 @@ pub mod diagnostics { } impl PreviousBorrow { #[track_caller] + #[inline(never)] pub fn borrowed() -> Self { Self { location: *Location::caller(), @@ -71,6 +74,7 @@ pub mod diagnostics { } #[track_caller] + #[inline(never)] pub fn borrowed_mut() -> Self { Self { location: *Location::caller(), @@ -257,6 +261,7 @@ impl UseSharedState { /// Try reading the shared state #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn try_read(&self) -> UseSharedStateResult> { self.inner .try_borrow() @@ -275,6 +280,7 @@ impl UseSharedState { /// Read the shared value #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn read(&self) -> Ref<'_, T> { match self.try_read() { Ok(value) => value, @@ -287,6 +293,7 @@ impl UseSharedState { /// Try writing the shared state #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn try_write(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() @@ -309,6 +316,7 @@ impl UseSharedState { /// // TODO: We prevent unncessary notifications only in the hook, but we should figure out some more global lock #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn write(&self) -> RefMut<'_, T> { match self.try_write() { Ok(value) => value, @@ -321,6 +329,7 @@ impl UseSharedState { /// Tries writing the value without forcing a re-render #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn try_write_silent(&self) -> UseSharedStateResult> { self.inner .try_borrow_mut() @@ -339,6 +348,7 @@ impl UseSharedState { /// Writes the value without forcing a re-render #[cfg_attr(debug_assertions, track_caller)] + #[cfg_attr(debug_assertions, inline(never))] pub fn write_silent(&self) -> RefMut<'_, T> { match self.try_write_silent() { Ok(value) => value, From af091c5bf82b7edce0d1ad846011ffa5dea63189 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 16:20:19 +0200 Subject: [PATCH 08/11] turns out Result combinators are not marked with #[trace_caller], so it must be written without function nesting in order to work --- packages/hooks/src/use_shared_state.rs | 50 +++++++++++++------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index cc1679b09..5ab88b7bd 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -263,19 +263,19 @@ impl UseSharedState { #[cfg_attr(debug_assertions, track_caller)] #[cfg_attr(debug_assertions, inline(never))] pub fn try_read(&self) -> UseSharedStateResult> { - self.inner - .try_borrow() - .map_err(|source| UseSharedStateError::AlreadyBorrowedMutably { + match self.inner.try_borrow() { + Ok(value) => { + self.debug_track_borrow(); + Ok(Ref::map(value, |inner| &inner.value)) + } + Err(source) => Err(UseSharedStateError::AlreadyBorrowedMutably { source, type_name: std::any::type_name::(), location: debug_location!(), #[cfg(debug_assertions)] previous_borrow: *self.previous_borrow.borrow(), - }) - .map(|value| { - self.debug_track_borrow(); - Ref::map(value, |inner| &inner.value) - }) + }), + } } /// Read the shared value @@ -295,20 +295,20 @@ impl UseSharedState { #[cfg_attr(debug_assertions, track_caller)] #[cfg_attr(debug_assertions, inline(never))] pub fn try_write(&self) -> UseSharedStateResult> { - self.inner - .try_borrow_mut() - .map_err(|source| UseSharedStateError::AlreadyBorrowed { + match self.inner.try_borrow_mut() { + Ok(mut value) => { + self.debug_track_borrow_mut(); + value.notify_consumers(); + Ok(RefMut::map(value, |inner| &mut inner.value)) + } + Err(source) => Err(UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), location: crate::debug_location!(), #[cfg(debug_assertions)] previous_borrow: *self.previous_borrow.borrow(), - }) - .map(|mut value| { - value.notify_consumers(); - self.debug_track_borrow_mut(); - RefMut::map(value, |inner| &mut inner.value) - }) + }), + } } /// Calling "write" will force the component to re-render @@ -331,19 +331,19 @@ impl UseSharedState { #[cfg_attr(debug_assertions, track_caller)] #[cfg_attr(debug_assertions, inline(never))] pub fn try_write_silent(&self) -> UseSharedStateResult> { - self.inner - .try_borrow_mut() - .map_err(|source| UseSharedStateError::AlreadyBorrowed { + match self.inner.try_borrow_mut() { + Ok(value) => { + self.debug_track_borrow_mut(); + Ok(RefMut::map(value, |inner| &mut inner.value)) + } + Err(source) => Err(UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), location: crate::debug_location!(), #[cfg(debug_assertions)] previous_borrow: *self.previous_borrow.borrow(), - }) - .map(|value| { - self.debug_track_borrow_mut(); - RefMut::map(value, |inner| &mut inner.value) - }) + }), + } } /// Writes the value without forcing a re-render From 9154b0f8ea57b2b46a167c6c508fbea11e0ae8f8 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 16:24:39 +0200 Subject: [PATCH 09/11] pass the information on previous caller on clone --- packages/hooks/src/use_shared_state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index 5ab88b7bd..c1791afed 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -365,7 +365,7 @@ impl Clone for UseSharedState { Self { inner: self.inner.clone(), #[cfg(debug_assertions)] - previous_borrow: Default::default(), + previous_borrow: self.previous_borrow.clone(), } } } From 60d6137f3901fab12999f4345ea99356a8be1ef6 Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Wed, 12 Jul 2023 16:36:17 +0200 Subject: [PATCH 10/11] better display for locations in panic messages --- packages/hooks/src/use_shared_state.rs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index c1791afed..cd5909d57 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -42,6 +42,22 @@ pub mod diagnostics { pub kind: BorrowKind, } + impl PreviousBorrow { + pub fn display_opt(value: &Option) -> String { + value + .as_ref() + .map(|value| value.to_string()) + .unwrap_or_default() + } + } + + impl std::fmt::Display for PreviousBorrow { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let Self { location, kind } = self; + write!(f, "{location} ({kind:?})") + } + } + impl super::UseSharedState { #[cfg_attr(debug_assertions, track_caller)] #[cfg_attr(debug_assertions, inline(never))] @@ -90,7 +106,8 @@ pub mod error { #[cfg_attr( debug_assertions, error( - "[{location}] {type_name} is already borrowed at [{previous_borrow:?}], so it cannot be borrowed mutably." + "[{location}] {type_name} is already borrowed at [{previous_borrow}], so it cannot be borrowed mutably.", + previous_borrow = super::diagnostics::PreviousBorrow::display_opt(.previous_borrow) ) )] #[cfg_attr( @@ -108,7 +125,8 @@ pub mod error { #[cfg_attr( debug_assertions, error( - "[{location}] {type_name} is already borrowed mutably at [{previous_borrow:?}], so it cannot be borrowed anymore." + "[{location}] {type_name} is already borrowed mutably at [{previous_borrow}], so it cannot be borrowed anymore.", + previous_borrow = super::diagnostics::PreviousBorrow::display_opt(.previous_borrow) ) )] #[cfg_attr( From 337697a8866c524df503b099ed060fb808615cdd Mon Sep 17 00:00:00 2001 From: niedzwiedzw Date: Thu, 13 Jul 2023 20:12:28 +0200 Subject: [PATCH 11/11] test with forked version of debug-cell --- packages/hooks/Cargo.toml | 6 +- packages/hooks/src/lib.rs | 2 + packages/hooks/src/use_shared_state.rs | 154 +++++-------------------- 3 files changed, 35 insertions(+), 127 deletions(-) diff --git a/packages/hooks/Cargo.toml b/packages/hooks/Cargo.toml index b2dd4898f..7bb40c5f3 100644 --- a/packages/hooks/Cargo.toml +++ b/packages/hooks/Cargo.toml @@ -9,16 +9,18 @@ repository = "https://github.com/DioxusLabs/dioxus/" homepage = "https://dioxuslabs.com" keywords = ["dom", "ui", "gui", "react"] +[features] +default = [] +nightly-features = [] [dependencies] dioxus-core = { workspace = true } futures-channel = { workspace = true } log = { workspace = true } thiserror = { workspace = true } - +debug-cell = { git = "https://github.com/Niedzwiedzw/debug-cell", rev = "3352a1f8aff19f56f5e3b2018200a3338fd43d2e" } # waiting for the merge / official DioxusLabs fork [dev-dependencies] futures-util = { workspace = true, default-features = false } dioxus-core = { workspace = true } dioxus = { workspace = true } -debug-cell = "0.1.1" diff --git a/packages/hooks/src/lib.rs b/packages/hooks/src/lib.rs index 21795a9d6..9589c5f72 100644 --- a/packages/hooks/src/lib.rs +++ b/packages/hooks/src/lib.rs @@ -1,3 +1,5 @@ +#![cfg_attr(feature = "nightly-features", feature(debug_refcell))] + #[macro_export] /// A helper macro for using hooks and properties in async environements. /// diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index cd5909d57..bde1fabb8 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -1,17 +1,15 @@ use self::error::{UseSharedStateError, UseSharedStateResult}; use dioxus_core::{ScopeId, ScopeState}; -use std::{ - cell::{Ref, RefCell, RefMut}, - collections::HashSet, - rc::Rc, - sync::Arc, +use std::{collections::HashSet, rc::Rc, sync::Arc}; + +#[cfg(debug_assertions)] +pub use debug_cell::{ + error::{BorrowError, BorrowMutError}, + Ref, RefCell, RefMut, }; #[cfg(not(debug_assertions))] -type Location = (); - -#[cfg(debug_assertions)] -type Location = &'static std::panic::Location<'static>; +pub use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut}; #[macro_export] macro_rules! debug_location { @@ -27,119 +25,49 @@ macro_rules! debug_location { }}; } -pub mod diagnostics { - use std::panic::Location; - - #[derive(Debug, Clone, Copy)] - pub enum BorrowKind { - Mutable, - Immutable, - } - - #[derive(Debug, Clone, Copy)] - pub struct PreviousBorrow { - pub location: Location<'static>, - pub kind: BorrowKind, - } - - impl PreviousBorrow { - pub fn display_opt(value: &Option) -> String { - value - .as_ref() - .map(|value| value.to_string()) - .unwrap_or_default() - } - } - - impl std::fmt::Display for PreviousBorrow { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let Self { location, kind } = self; - write!(f, "{location} ({kind:?})") - } - } - - impl super::UseSharedState { - #[cfg_attr(debug_assertions, track_caller)] - #[cfg_attr(debug_assertions, inline(never))] - #[allow(unused_must_use)] - pub(super) fn debug_track_borrow(&self) { - #[cfg(debug_assertions)] - self.previous_borrow - .borrow_mut() - .insert(PreviousBorrow::borrowed()); - } - - #[cfg_attr(debug_assertions, track_caller)] - #[cfg_attr(debug_assertions, inline(never))] - #[allow(unused_must_use)] - pub(super) fn debug_track_borrow_mut(&self) { - #[cfg(debug_assertions)] - self.previous_borrow - .borrow_mut() - .insert(PreviousBorrow::borrowed_mut()); - } - } - impl PreviousBorrow { - #[track_caller] - #[inline(never)] - pub fn borrowed() -> Self { - Self { - location: *Location::caller(), - kind: BorrowKind::Immutable, - } - } - - #[track_caller] - #[inline(never)] - pub fn borrowed_mut() -> Self { - Self { - location: *Location::caller(), - kind: BorrowKind::Mutable, - } - } - } -} - pub mod error { + fn locations_display(locations: &[&'static std::panic::Location<'static>]) -> String { + locations + .iter() + .map(|location| format!(" - {location}")) + .collect::>() + .join("\n") + } #[derive(thiserror::Error, Debug)] pub enum UseSharedStateError { #[cfg_attr( debug_assertions, error( - "[{location}] {type_name} is already borrowed at [{previous_borrow}], so it cannot be borrowed mutably.", - previous_borrow = super::diagnostics::PreviousBorrow::display_opt(.previous_borrow) + "[{0}] {1} is already borrowed at, so it cannot be borrowed mutably. Previous borrows:\n[{2}]\n\n", + .source.attempted_at, + .type_name, + locations_display(&.source.already_borrowed_at) ) )] #[cfg_attr( not(debug_assertions), - error("{type_name} is already borrowed, so it cannot be borrowed mutably.") + error("{type_name} is already borrowed, so it cannot be borrowed mutably. (More detail available in debug mode)") )] AlreadyBorrowed { - source: core::cell::BorrowMutError, + source: super::BorrowMutError, type_name: &'static str, - /// Only available in debug mode - location: super::Location, - #[cfg(debug_assertions)] - previous_borrow: Option, }, #[cfg_attr( debug_assertions, error( - "[{location}] {type_name} is already borrowed mutably at [{previous_borrow}], so it cannot be borrowed anymore.", - previous_borrow = super::diagnostics::PreviousBorrow::display_opt(.previous_borrow) + "[{0}] {1} is already borrowed mutably at [{2}], so it cannot be borrowed anymore.", + .source.attempted_at, + .type_name, + locations_display(&.source.already_borrowed_at) ) )] #[cfg_attr( not(debug_assertions), - error("{type_name} is already borrowed mutably, so it cannot be borrowed anymore.") + error("{type_name} is already borrowed mutably, so it cannot be borrowed anymore. (More detail available in debug mode)") )] AlreadyBorrowedMutably { - source: core::cell::BorrowError, + source: super::BorrowError, type_name: &'static str, - /// Only available in debug mode - location: super::Location, - #[cfg(debug_assertions)] - previous_borrow: Option, }, } @@ -259,17 +187,11 @@ impl Drop for UseSharedStateOwner { /// State that is shared between components through the context system pub struct UseSharedState { pub(crate) inner: Rc>>, - #[cfg(debug_assertions)] - previous_borrow: Rc>>, } impl UseSharedState { fn new(inner: Rc>>) -> Self { - Self { - inner, - #[cfg(debug_assertions)] - previous_borrow: Default::default(), - } + Self { inner } } /// Notify all consumers of the state that it has changed. (This is called automatically when you call "write") @@ -282,16 +204,10 @@ impl UseSharedState { #[cfg_attr(debug_assertions, inline(never))] pub fn try_read(&self) -> UseSharedStateResult> { match self.inner.try_borrow() { - Ok(value) => { - self.debug_track_borrow(); - Ok(Ref::map(value, |inner| &inner.value)) - } + Ok(value) => Ok(Ref::map(value, |inner| &inner.value)), Err(source) => Err(UseSharedStateError::AlreadyBorrowedMutably { source, type_name: std::any::type_name::(), - location: debug_location!(), - #[cfg(debug_assertions)] - previous_borrow: *self.previous_borrow.borrow(), }), } } @@ -315,16 +231,12 @@ impl UseSharedState { pub fn try_write(&self) -> UseSharedStateResult> { match self.inner.try_borrow_mut() { Ok(mut value) => { - self.debug_track_borrow_mut(); value.notify_consumers(); Ok(RefMut::map(value, |inner| &mut inner.value)) } Err(source) => Err(UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), - location: crate::debug_location!(), - #[cfg(debug_assertions)] - previous_borrow: *self.previous_borrow.borrow(), }), } } @@ -350,16 +262,10 @@ impl UseSharedState { #[cfg_attr(debug_assertions, inline(never))] pub fn try_write_silent(&self) -> UseSharedStateResult> { match self.inner.try_borrow_mut() { - Ok(value) => { - self.debug_track_borrow_mut(); - Ok(RefMut::map(value, |inner| &mut inner.value)) - } + Ok(value) => Ok(RefMut::map(value, |inner| &mut inner.value)), Err(source) => Err(UseSharedStateError::AlreadyBorrowed { source, type_name: std::any::type_name::(), - location: crate::debug_location!(), - #[cfg(debug_assertions)] - previous_borrow: *self.previous_borrow.borrow(), }), } } @@ -382,8 +288,6 @@ impl Clone for UseSharedState { fn clone(&self) -> Self { Self { inner: self.inner.clone(), - #[cfg(debug_assertions)] - previous_borrow: self.previous_borrow.clone(), } } }