Unify the warning system (#2649)

* unify the warning system

* fix VirtualDom::new warning with a component

* move warnings to dioxuslabs

* also allow writes in the component body when converting from T -> ReadOnlySignal<T>

* fix clippy from merge conflict

---------

Co-authored-by: Jonathan Kelley <jkelleyrtp@gmail.com>

Cargo.lock

@@ -2526,6 +2526,7 @@ dependencies = [
  "tracing",
  "tracing-fluent-assertions",
  "tracing-subscriber",
+ "warnings",
  "web-sys",
 ]
 
@@ -2711,6 +2712,7 @@ dependencies = [
  "slab",
  "tokio",
  "tracing",
+ "warnings",
  "web-sys",
 ]
 
@@ -2970,6 +2972,7 @@ dependencies = [
  "tokio",
  "tracing",
  "tracing-subscriber",
+ "warnings",
 ]
 
 [[package]]
@@ -10363,6 +10366,25 @@ dependencies = [
  "try-lock",
 ]
 
+[[package]]
+name = "warnings"
+version = "0.1.0"
+source = "git+https://github.com/DioxusLabs/warnings#9889b96cccb6ac91a8af924cfee51a8895146d08"
+dependencies = [
+ "pin-project",
+ "warnings-macro",
+]
+
+[[package]]
+name = "warnings-macro"
+version = "0.1.0"
+source = "git+https://github.com/DioxusLabs/warnings#9889b96cccb6ac91a8af924cfee51a8895146d08"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.71",
+]
+
 [[package]]
 name = "wasi"
 version = "0.9.0+wasi-snapshot-preview1"

packages/core/Cargo.toml

@@ -24,6 +24,7 @@ serde = { version = "1", features = ["derive"], optional = true }
 generational-box = { workspace = true }
 rustversion = "1.0.17"
 const_format = { workspace = true }
+warnings = { git = "https://github.com/DioxusLabs/warnings" }
 
 [dev-dependencies]
 tokio = { workspace = true, features = ["full"] }

packages/core/src/properties.rs

@@ -117,34 +117,36 @@ where
     P::builder()
 }
 
+/// A warning that will trigger if a component is called as a function
+#[warnings::warning]
+pub(crate) fn component_called_as_function<C: ComponentFunction<P, M>, P, M>(_: C) {
+    // We trim WithOwner from the end of the type name for component with a builder that include a special owner which may not match the function name directly
+    let mut type_name = std::any::type_name::<C>();
+    if let Some((_, after_colons)) = type_name.rsplit_once("::") {
+        type_name = after_colons;
+    }
+    let component_name = Runtime::with(|rt| {
+        current_scope_id()
+            .ok()
+            .and_then(|id| rt.get_state(id).map(|scope| scope.name))
+    })
+    .ok()
+    .flatten();
+
+    // If we are in a component, and the type name is the same as the active component name, then we can just return
+    if component_name == Some(type_name) {
+        return;
+    }
+
+    // Otherwise the component was called like a function, so we should log an error
+    tracing::error!("It looks like you called the component {type_name} like a function instead of a component. Components should be called with braces like `{type_name} {{ prop: value }}` instead of as a function");
+}
+
 /// Make sure that this component is currently running as a component, not a function call
 #[doc(hidden)]
-#[allow(unused)]
+#[allow(clippy::no_effect)]
 pub fn verify_component_called_as_component<C: ComponentFunction<P, M>, P, M>(component: C) {
-    #[cfg(debug_assertions)]
-    {
-        // We trim WithOwner from the end of the type name for component with a builder that include a special owner which may not match the function name directly
-        let mut type_name = std::any::type_name::<C>();
-        if let Some((_, after_colons)) = type_name.rsplit_once("::") {
-            type_name = after_colons;
-        }
-        let component_name = Runtime::with(|rt| {
-            current_scope_id()
-                .ok()
-                .and_then(|id| rt.get_state(id))
-                .map(|scope| scope.name)
-        })
-        .ok()
-        .flatten();
-
-        // If we are in a component, and the type name is the same as the active component name, then we can just return
-        if component_name == Some(type_name) {
-            return;
-        }
-
-        // Otherwise the component was called like a function, so we should log an error
-        tracing::error!("It looks like you called the component {type_name} like a function instead of a component. Components should be called with braces like `{type_name} {{ prop: value }}` instead of as a function");
-    }
+    component_called_as_function(component);
 }
 
 /// Any component that implements the `ComponentFn` trait can be used as a component.

packages/core/src/virtual_dom.rs

@@ -254,8 +254,15 @@ impl VirtualDom {
     /// ```
     ///
     /// Note: the VirtualDom is not progressed, you must either "run_with_deadline" or use "rebuild" to progress it.
-    pub fn new<F: Fn() -> Element + Clone + 'static>(app: F) -> Self {
-        Self::new_with_props(app, ())
+    pub fn new(app: fn() -> Element) -> Self {
+        Self::new_with_props(
+            move || {
+                use warnings::Warning;
+                // The root props don't come from a vcomponent so we need to manually rerun them sometimes
+                crate::properties::component_called_as_function::allow(app)
+            },
+            (),
+        )
     }
 
     /// Create a new VirtualDom with the given properties for the root component.
@@ -313,7 +320,7 @@ impl VirtualDom {
     }
 
     /// Create a new virtualdom and build it immediately
-    pub fn prebuilt<F: Fn() -> Element + Clone + 'static>(app: F) -> Self {
+    pub fn prebuilt(app: fn() -> Element) -> Self {
         let mut dom = Self::new(app);
         dom.rebuild_in_place();
         dom

packages/hooks/Cargo.toml

@@ -22,6 +22,7 @@ slab = { workspace = true }
 futures-util = { workspace = true}
 generational-box.workspace = true
 rustversion = "1.0.17"
+warnings = { git = "https://github.com/DioxusLabs/warnings" }
 
 [dev-dependencies]
 futures-util = { workspace = true, default-features = false }

packages/hooks/src/use_coroutine.rs

@@ -1,3 +1,4 @@
+use ::warnings::Warning;
 use dioxus_core::prelude::{consume_context, provide_context, spawn, use_hook};
 use dioxus_core::Task;
 use dioxus_signals::*;
@@ -84,13 +85,17 @@ where
 
     // We do this here so we can capture data with FnOnce
     // this might not be the best API
-    if *coroutine.needs_regen.peek() {
-        let (tx, rx) = futures_channel::mpsc::unbounded();
-        let task = spawn(init(rx));
-        coroutine.tx.set(Some(tx));
-        coroutine.task.set(Some(task));
-        coroutine.needs_regen.set(false);
-    }
+    dioxus_signals::warnings::signal_read_and_write_in_reactive_scope::allow(|| {
+        dioxus_signals::warnings::signal_write_in_component_body::allow(|| {
+            if *coroutine.needs_regen.peek() {
+                let (tx, rx) = futures_channel::mpsc::unbounded();
+                let task = spawn(init(rx));
+                coroutine.tx.set(Some(tx));
+                coroutine.task.set(Some(task));
+                coroutine.needs_regen.set(false);
+            }
+        })
+    });
 
     coroutine
 }

packages/hooks/src/use_reactive.rs

@@ -110,7 +110,14 @@ pub fn use_reactive<O, D: Dependency>(
         non_reactive_data.out()
     });
     if !first_run && non_reactive_data.changed(&*last_state.peek()) {
-        last_state.set(non_reactive_data.out());
+        use warnings::Warning;
+        // In use_reactive we do read and write to a signal during rendering to bridge the reactive and non-reactive worlds.
+        // We ignore
+        dioxus_signals::warnings::signal_read_and_write_in_reactive_scope::allow(|| {
+            dioxus_signals::warnings::signal_write_in_component_body::allow(|| {
+                last_state.set(non_reactive_data.out())
+            })
+        });
     }
     move || closure(last_state())
 }

packages/signals/Cargo.toml

@@ -22,6 +22,7 @@ once_cell = "1.18.0"
 rustc-hash = { workspace = true }
 futures-channel = { workspace = true }
 futures-util = { workspace = true }
+warnings = { git = "https://github.com/DioxusLabs/warnings" }
 
 [dev-dependencies]
 dioxus = { workspace = true }

packages/signals/src/read_only_signal.rs

@@ -46,7 +46,13 @@ impl<T: 'static, S: Storage<SignalData<T>>> ReadOnlySignal<T, S> {
     /// This should only be used by the `rsx!` macro.
     pub fn __set(&mut self, value: T) {
         use crate::write::Writable;
-        self.inner.set(value);
+        use warnings::Warning;
+        // This is only called when converting T -> ReadOnlySignal<T> which will not cause loops
+        crate::warnings::signal_write_in_component_body::allow(|| {
+            crate::warnings::signal_read_and_write_in_reactive_scope::allow(|| {
+                self.inner.set(value);
+            });
+        });
     }
 
     #[doc(hidden)]

packages/signals/src/signal.rs

@@ -603,39 +603,56 @@ struct SignalSubscriberDrop<T: 'static, S: Storage<SignalData<T>>> {
     origin: &'static std::panic::Location<'static>,
 }
 
-impl<T: 'static, S: Storage<SignalData<T>>> Drop for SignalSubscriberDrop<T, S> {
-    fn drop(&mut self) {
-        #[cfg(debug_assertions)]
-        {
-            tracing::trace!(
-                "Write on signal at {} finished, updating subscribers",
-                self.origin
-            );
-
-            // 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.",
-                    self.origin
-                );
-            }
+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, this will probably cause an infinite loop.
-            if let Some(reactive_context) = ReactiveContext::current() {
-                if let Ok(inner) = self.signal.inner.try_read() {
-                    if let Ok(subscribers) = inner.subscribers.lock() {
-                        for subscriber in subscribers.iter() {
-                            if reactive_context == *subscriber {
-                                let origin = self.origin;
-                                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}",
-                                );
-                            }
+    /// 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) {
+        tracing::trace!(
+            "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);
         self.signal.update_subscribers();
     }
 }