Add a warning when a copy value is used in a higher scope than it was created in (#2771)

packages/core/src/scope_context.rs

@@ -511,6 +511,18 @@ impl ScopeId {
             .flatten()
     }
 
+    /// Check if the current scope is a descendant of the given scope
+    pub fn is_descendant_of(self, other: ScopeId) -> bool {
+        let mut current = self;
+        while let Some(parent) = current.parent_scope() {
+            if parent == other {
+                return true;
+            }
+            current = parent;
+        }
+        false
+    }
+
     /// Mark the current scope as dirty, causing it to re-render
     pub fn needs_update(self) {
         Runtime::with_scope(self, |cx| cx.needs_update()).unwrap();

packages/generational-box/src/lib.rs

@@ -115,6 +115,18 @@ impl<T, S: Storage<T>> GenerationalBox<T, S> {
     {
         self.raw.take()
     }
+
+    /// Try to get the location the generational box was created at. In release mode this will always return None.
+    pub fn created_at(&self) -> Option<&'static std::panic::Location<'static>> {
+        #[cfg(debug_assertions)]
+        {
+            Some(self.raw.location.created_at)
+        }
+        #[cfg(not(debug_assertions))]
+        {
+            None
+        }
+    }
 }
 
 impl<T, S> Copy for GenerationalBox<T, S> {}

packages/signals/docs/hoist/error.rs

@@ -0,0 +1,31 @@
+#[component]
+fn Counters() -> Element {
+    let counts = use_signal(Vec::new);
+    let mut children = use_signal(|| 0);
+
+    rsx! {
+        button { onclick: move |_| children += 1, "Add child" }
+        button { onclick: move |_| children -= 1, "Remove child" }
+        // A signal from a child is read or written to in a parent scope
+        "{counts:?}"
+        for _ in 0..children() {
+            Counter {
+                counts
+            }
+        }
+    }
+}
+
+#[component]
+fn Counter(mut counts: Signal<Vec<Signal<i32>>>) -> Element {
+    let mut signal_owned_by_child = use_signal(|| 0);
+    // Moving the signal up to the parent may cause issues if you read the signal after the child scope is dropped
+    use_hook(|| counts.push(signal_owned_by_child));
+
+    rsx! {
+        button {
+            onclick: move |_| signal_owned_by_child += 1,
+            "{signal_owned_by_child}"
+        }
+    }
+}

packages/signals/docs/hoist/fixed_list.rs

@@ -0,0 +1,32 @@
+#[component]
+fn Counters() -> Element {
+    let mut counts = use_signal(Vec::new);
+
+    rsx! {
+        button { onclick: move |_| counts.write().push(0), "Add child" }
+        button {
+            onclick: move |_| {
+                counts.write().pop();
+            },
+            "Remove child"
+        }
+        "{counts:?}"
+        // Instead of passing up a signal, we can just write to the signal that lives in the parent
+        for index in 0..counts.len() {
+            Counter {
+                index,
+                counts
+            }
+        }
+    }
+}
+
+#[component]
+fn Counter(index: usize, mut counts: Signal<Vec<i32>>) -> Element {
+    rsx! {
+        button {
+            onclick: move |_| counts.write()[index] += 1,
+            "{counts.read()[index]}"
+        }
+    }
+}

packages/signals/src/copy_value.rs

@@ -1,3 +1,6 @@
+#![allow(clippy::unnecessary_operation)]
+#![allow(clippy::no_effect)]
+
 use generational_box::UnsyncStorage;
 use generational_box::{BorrowResult, GenerationalBoxId};
 use std::ops::Deref;
@@ -18,7 +21,7 @@ use crate::{default_impl, write_impls};
 /// It is internally backed by [`generational_box::GenerationalBox`].
 pub struct CopyValue<T: 'static, S: Storage<T> = UnsyncStorage> {
     pub(crate) value: GenerationalBox<T, S>,
-    origin_scope: ScopeId,
+    pub(crate) origin_scope: ScopeId,
 }
 
 #[cfg(feature = "serialize")]
@@ -65,12 +68,7 @@ impl<T: 'static, S: Storage<T>> CopyValue<T, S> {
     /// Once the component this value is created in is dropped, the value will be dropped.
     #[track_caller]
     pub fn new_maybe_sync(value: T) -> Self {
-        let owner = current_owner();
-
-        Self {
-            value: owner.insert(value),
-            origin_scope: current_scope_id().expect("in a virtual dom"),
-        }
+        Self::new_with_caller(value, std::panic::Location::caller())
     }
 
     pub(crate) fn new_with_caller(
@@ -88,10 +86,20 @@ impl<T: 'static, S: Storage<T>> CopyValue<T, S> {
     /// Create a new CopyValue. The value will be stored in the given scope. When the specified scope is dropped, the value will be dropped.
     #[track_caller]
     pub fn new_maybe_sync_in_scope(value: T, scope: ScopeId) -> Self {
+        Self::new_maybe_sync_in_scope_with_caller(value, scope, std::panic::Location::caller())
+    }
+
+    /// Create a new CopyValue with a custom caller. The value will be stored in the given scope. When the specified scope is dropped, the value will be dropped.
+    #[track_caller]
+    pub fn new_maybe_sync_in_scope_with_caller(
+        value: T,
+        scope: ScopeId,
+        caller: &'static std::panic::Location<'static>,
+    ) -> Self {
         let owner = scope.owner();
 
         Self {
-            value: owner.insert(value),
+            value: owner.insert_with_caller(value, caller),
             origin_scope: scope,
         }
     }
@@ -125,11 +133,13 @@ impl<T: 'static, S: Storage<T>> Readable for CopyValue<T, S> {
     fn try_read_unchecked(
         &self,
     ) -> Result<ReadableRef<'static, Self>, generational_box::BorrowError> {
+        crate::warnings::copy_value_hoisted(self, std::panic::Location::caller());
         self.value.try_read()
     }
 
     #[track_caller]
     fn try_peek_unchecked(&self) -> BorrowResult<ReadableRef<'static, Self>> {
+        crate::warnings::copy_value_hoisted(self, std::panic::Location::caller());
         self.value.try_read()
     }
 }
@@ -161,11 +171,13 @@ impl<T: 'static, S: Storage<T>> Writable for CopyValue<T, S> {
     fn try_write_unchecked(
         &self,
     ) -> Result<WritableRef<'static, Self>, generational_box::BorrowMutError> {
+        crate::warnings::copy_value_hoisted(self, std::panic::Location::caller());
         self.value.try_write()
     }
 
     #[track_caller]
     fn set(&mut self, value: T) {
+        crate::warnings::copy_value_hoisted(self, std::panic::Location::caller());
         self.value.set(value);
     }
 }

packages/signals/src/global/signal.rs

@@ -13,6 +13,7 @@ use crate::Signal;
 pub struct GlobalSignal<T> {
     initializer: fn() -> T,
     key: GlobalKey<'static>,
+    created_at: &'static std::panic::Location<'static>,
 }
 
 impl<T: 'static> GlobalSignal<T> {
@@ -23,6 +24,7 @@ impl<T: 'static> GlobalSignal<T> {
         GlobalSignal {
             initializer,
             key: GlobalKey::new(key),
+            created_at: key,
         }
     }
 
@@ -34,10 +36,12 @@ impl<T: 'static> GlobalSignal<T> {
     /// Create this global signal with a specific key.
     /// This is useful for ensuring that the signal is unique across the application and accessible from
     /// outside the application too.
+    #[track_caller]
     pub const fn with_key(initializer: fn() -> T, key: &'static str) -> GlobalSignal<T> {
         GlobalSignal {
             initializer,
             key: GlobalKey::new_from_str(key),
+            created_at: std::panic::Location::caller(),
         }
     }
 
@@ -56,7 +60,11 @@ impl<T: 'static> GlobalSignal<T> {
                 // Constructors are always run in the root scope
                 // The signal also exists in the root scope
                 let value = ScopeId::ROOT.in_runtime(self.initializer);
-                let signal = Signal::new_in_scope(value, ScopeId::ROOT);
+                let signal = Signal::new_maybe_sync_in_scope_with_caller(
+                    value,
+                    ScopeId::ROOT,
+                    self.created_at,
+                );
 
                 let entry = context.signal.borrow_mut().insert(key, Box::new(signal));
                 debug_assert!(entry.is_none(), "Global signal already exists");

packages/signals/src/lib.rs

@@ -39,3 +39,5 @@ pub use write::*;
 
 mod props;
 pub use props::*;
+
+pub mod warnings;

packages/signals/src/signal.rs

@@ -183,13 +183,24 @@ impl<T: 'static, S: Storage<SignalData<T>>> Signal<T, S> {
     #[track_caller]
     #[tracing::instrument(skip(value))]
     pub fn new_maybe_sync_in_scope(value: T, owner: ScopeId) -> Self {
+        Self::new_maybe_sync_in_scope_with_caller(value, owner, std::panic::Location::caller())
+    }
+
+    /// Create a new signal with a custom owner scope and a custom caller. The signal will be dropped when the owner scope is dropped instead of the current scope.
+    #[tracing::instrument(skip(value))]
+    pub fn new_maybe_sync_in_scope_with_caller(
+        value: T,
+        owner: ScopeId,
+        caller: &'static std::panic::Location<'static>,
+    ) -> Self {
         Self {
-            inner: CopyValue::<SignalData<T>, S>::new_maybe_sync_in_scope(
+            inner: CopyValue::<SignalData<T>, S>::new_maybe_sync_in_scope_with_caller(
                 SignalData {
                     subscribers: Default::default(),
                     value,
                 },
                 owner,
+                caller,
             ),
         }
     }
@@ -534,115 +545,12 @@ impl<T: ?Sized, S: AnyStorage> DerefMut for Write<'_, T, S> {
     }
 }
 
-#[allow(unused)]
-const SIGNAL_READ_WRITE_SAME_SCOPE_HELP: &str = r#"This issue is caused by reading and writing to the same signal in a reactive scope. Components, effects, memos, and resources each have their own a reactive scopes. Reactive scopes rerun when any signal you read inside of them are changed. If you read and write to the same signal in the same scope, the write will cause the scope to rerun and trigger the write again. This can cause an infinite loop.
-
-You can fix the issue by either:
-1) Splitting up your state and Writing, reading to different signals:
-
-For example, you could change this broken code:
-
-#[derive(Clone, Copy)]
-struct Counts {
-    count1: i32,
-    count2: i32,
-}
-
-fn app() -> Element {
-    let mut counts = use_signal(|| Counts { count1: 0, count2: 0 });
-
-    use_effect(move || {
-        // This effect both reads and writes to counts
-        counts.write().count1 = counts().count2;
-    })
-}
-
-Into this working code:
-
-fn app() -> Element {
-    let mut count1 = use_signal(|| 0);
-    let mut count2 = use_signal(|| 0);
-
-    use_effect(move || {
-        count1.write(count2());
-    });
-}
-2) Reading and Writing to the same signal in different scopes:
-
-For example, you could change this broken code:
-
-fn app() -> Element {
-    let mut count = use_signal(|| 0);
-
-    use_effect(move || {
-        // This effect both reads and writes to count
-        println!("{}", count());
-        count.write(count());
-    });
-}
-
-
-To this working code:
-
-fn app() -> Element {
-    let mut count = use_signal(|| 0);
-
-    use_effect(move || {
-        count.write(count());
-    });
-    use_effect(move || {
-        println!("{}", count());
-    });
-}
-"#;
-
 struct SignalSubscriberDrop<T: 'static, S: Storage<SignalData<T>>> {
     signal: Signal<T, S>,
     #[cfg(debug_assertions)]
     origin: &'static std::panic::Location<'static>,
 }
 
-pub mod warnings {
-    //! Warnings that can be triggered by suspicious usage of signals
-
-    use super::*;
-    use ::warnings::warning;
-
-    /// Check if the write happened during a render. If it did, warn the user that this is generally a bad practice.
-    #[warning]
-    pub fn signal_write_in_component_body(origin: &'static std::panic::Location<'static>) {
-        // Check if the write happened during a render. If it did, we should warn the user that this is generally a bad practice.
-        if dioxus_core::vdom_is_rendering() {
-            tracing::warn!(
-            "Write on signal at {} happened while a component was running. Writing to signals during a render can cause infinite rerenders when you read the same signal in the component. Consider writing to the signal in an effect, future, or event handler if possible.",
-            origin
-        );
-        }
-    }
-
-    /// Check if the write happened during a scope that the signal is also subscribed to. If it did, trigger a warning because it will likely cause an infinite loop.
-    #[warning]
-    pub fn signal_read_and_write_in_reactive_scope<T: 'static, S: Storage<SignalData<T>>>(
-        origin: &'static std::panic::Location<'static>,
-        signal: Signal<T, S>,
-    ) {
-        // Check if the write happened during a scope that the signal is also subscribed to. If it did, this will probably cause an infinite loop.
-        if let Some(reactive_context) = ReactiveContext::current() {
-            if let Ok(inner) = signal.inner.try_read() {
-                if let Ok(subscribers) = inner.subscribers.lock() {
-                    for subscriber in subscribers.iter() {
-                        if reactive_context == *subscriber {
-                            tracing::warn!(
-                            "Write on signal at {origin} finished in {reactive_context} which is also subscribed to the signal. This will likely cause an infinite loop. When the write finishes, {reactive_context} will rerun which may cause the write to be rerun again.\nHINT:\n{SIGNAL_READ_WRITE_SAME_SCOPE_HELP}",
-                        );
-                        }
-                    }
-                }
-            }
-        }
-    }
-}
-
 #[allow(clippy::no_effect)]
 impl<T: 'static, S: Storage<SignalData<T>>> Drop for SignalSubscriberDrop<T, S> {
     fn drop(&mut self) {
@@ -652,8 +560,11 @@ impl<T: 'static, S: Storage<SignalData<T>>> Drop for SignalSubscriberDrop<T, S>
                 "Write on signal at {} finished, updating subscribers",
                 self.origin
             );
-            warnings::signal_write_in_component_body(self.origin);
-            warnings::signal_read_and_write_in_reactive_scope::<T, S>(self.origin, self.signal);
+            crate::warnings::signal_write_in_component_body(self.origin);
+            crate::warnings::signal_read_and_write_in_reactive_scope::<T, S>(
+                self.origin,
+                self.signal,
+            );
         }
         self.signal.update_subscribers();
     }

packages/signals/src/warnings.rs

@@ -0,0 +1,154 @@
+//! Warnings that can be triggered by suspicious usage of signals
+
+use warnings::warning;
+
+/// A warning that is triggered when a copy value is used in a higher scope that it is owned by
+#[warning]
+pub fn copy_value_hoisted<T: 'static, S: generational_box::Storage<T>>(
+    value: &crate::CopyValue<T, S>,
+    caller: &'static std::panic::Location<'static>,
+) {
+    let origin_scope = value.origin_scope;
+    if let Ok(current_scope) = dioxus_core::prelude::current_scope_id() {
+        // If the current scope is a descendant of the origin scope or is the same scope, we don't need to warn
+        if origin_scope == current_scope || current_scope.is_descendant_of(origin_scope) {
+            return;
+        }
+        let create_location = value.value.created_at().unwrap();
+        let broken_example = include_str!("../docs/hoist/error.rs");
+        let fixed_example = include_str!("../docs/hoist/fixed_list.rs");
+
+        // Otherwise, warn that the value is being used in a higher scope and may be dropped
+        tracing::warn!(
+            r#"A Copy Value created in {origin_scope:?} (at {create_location}). It will be dropped when that scope is dropped, but it was used in {current_scope:?} (at {caller}) which is not a descendant of the owning scope.
+This may cause reads or writes to fail because the value is dropped while it still held.
+
+Help:
+Copy values (like CopyValue, Signal, Memo, and Resource) are owned by the scope they are created in. If you use the value in a scope that may be dropped after the origin scope,
+it is very easy to use the value after it has been dropped. To fix this, you can move the value to the parent of all of the scopes that it is used in.
+
+Broken example ❌:
+```rust
+{broken_example}
+```
+
+Fixed example ✅:
+```rust
+{fixed_example}
+```"#
+        );
+    }
+}
+
+// Include the examples from the warning to make sure they compile
+#[test]
+#[allow(unused)]
+fn hoist() {
+    mod broken {
+        use dioxus::prelude::*;
+        include!("../docs/hoist/error.rs");
+    }
+    mod fixed {
+        use dioxus::prelude::*;
+        include!("../docs/hoist/fixed_list.rs");
+    }
+}
+
+/// Check if the write happened during a render. If it did, warn the user that this is generally a bad practice.
+#[warning]
+pub fn signal_write_in_component_body(origin: &'static std::panic::Location<'static>) {
+    // Check if the write happened during a render. If it did, we should warn the user that this is generally a bad practice.
+    if dioxus_core::vdom_is_rendering() {
+        tracing::warn!(
+            "Write on signal at {} happened while a component was running. Writing to signals during a render can cause infinite rerenders when you read the same signal in the component. Consider writing to the signal in an effect, future, or event handler if possible.",
+            origin
+        );
+    }
+}
+
+/// Check if the write happened during a scope that the signal is also subscribed to. If it did, trigger a warning because it will likely cause an infinite loop.
+#[warning]
+pub fn signal_read_and_write_in_reactive_scope<
+    T: 'static,
+    S: generational_box::Storage<crate::SignalData<T>>,
+>(
+    origin: &'static std::panic::Location<'static>,
+    signal: crate::Signal<T, S>,
+) {
+    // Check if the write happened during a scope that the signal is also subscribed to. If it did, this will probably cause an infinite loop.
+    if let Some(reactive_context) = dioxus_core::prelude::ReactiveContext::current() {
+        if let Ok(inner) = crate::Readable::try_read(&signal.inner) {
+            if let Ok(subscribers) = inner.subscribers.lock() {
+                for subscriber in subscribers.iter() {
+                    if reactive_context == *subscriber {
+                        tracing::warn!(
+                            "Write on signal at {origin} finished in {reactive_context} which is also subscribed to the signal. This will likely cause an infinite loop. When the write finishes, {reactive_context} will rerun which may cause the write to be rerun again.\nHINT:\n{SIGNAL_READ_WRITE_SAME_SCOPE_HELP}",
+                        );
+                    }
+                }
+            }
+        }
+    }
+}
+
+#[allow(unused)]
+const SIGNAL_READ_WRITE_SAME_SCOPE_HELP: &str = r#"This issue is caused by reading and writing to the same signal in a reactive scope. Components, effects, memos, and resources each have their own a reactive scopes. Reactive scopes rerun when any signal you read inside of them are changed. If you read and write to the same signal in the same scope, the write will cause the scope to rerun and trigger the write again. This can cause an infinite loop.
+
+You can fix the issue by either:
+1) Splitting up your state and Writing, reading to different signals:
+
+For example, you could change this broken code:
+
+#[derive(Clone, Copy)]
+struct Counts {
+    count1: i32,
+    count2: i32,
+}
+
+fn app() -> Element {
+    let mut counts = use_signal(|| Counts { count1: 0, count2: 0 });
+
+    use_effect(move || {
+        // This effect both reads and writes to counts
+        counts.write().count1 = counts().count2;
+    })
+}
+
+Into this working code:
+
+fn app() -> Element {
+    let mut count1 = use_signal(|| 0);
+    let mut count2 = use_signal(|| 0);
+
+    use_effect(move || {
+        count1.write(count2());
+    });
+}
+2) Reading and Writing to the same signal in different scopes:
+
+For example, you could change this broken code:
+
+fn app() -> Element {
+    let mut count = use_signal(|| 0);
+
+    use_effect(move || {
+        // This effect both reads and writes to count
+        println!("{}", count());
+        count.write(count());
+    });
+}
+
+
+To this working code:
+
+fn app() -> Element {
+    let mut count = use_signal(|| 0);
+
+    use_effect(move || {
+        count.write(count());
+    });
+    use_effect(move || {
+        println!("{}", count());
+    });
+}
+"#;