Fix early drop of eventhandlers (#2126)

* fix early drop of eventhandlers

* add a test for stale props that are memorized in place

* fix clippy

packages/check/src/check.rs

@@ -37,6 +37,7 @@ pub fn check_file(path: PathBuf, file_content: &str) -> IssueReport {
     )
 }
 
+#[allow(unused)]
 #[derive(Debug, Clone)]
 enum Node {
     If(IfInfo),

packages/core-macro/src/props/mod.rs

@@ -648,38 +648,45 @@ mod struct_info {
                 )*
             };
 
+            // If there are signals, we automatically try to memoize the signals
             if !signal_fields.is_empty() {
                 Ok(quote! {
-                    // First check if the fields are equal
+                    // First check if the fields are equal. This will compare the signal fields by pointer
                     let exactly_equal = self == new;
                     if exactly_equal {
+                        // If they are return early, they can be memoized without any changes
                         return true;
                     }
 
-                    // If they are not, move over the signal fields and check if they are equal
+                    // If they are not, move the signal fields into self and check if they are equal now that the signal fields are equal
                     #(
                         let mut #signal_fields = self.#signal_fields;
                         self.#signal_fields = new.#signal_fields;
                     )*
 
-                    // Then check if the fields are equal again
+                    // Then check if the fields are equal now that we know the signal fields are equal
+                    // NOTE: we don't compare other fields individually because we want to let users opt-out of memoization for certain fields by implementing PartialEq themselves
                     let non_signal_fields_equal = self == new;
 
-                    // If they are not equal, we still need to rerun the component
+                    // If they are not equal, we need to move over all the fields to self
                     if !non_signal_fields_equal {
-                        return false;
+                        *self = new.clone();
                     }
-
+                    // Move any signal and event fields into their old container.
+                    // We update signals and event handlers in place so that they are always up to date even if they were moved into a future in a previous render
                     #move_signal_fields
                     #move_event_handlers
 
-                    true
+                    non_signal_fields_equal
                 })
             } else {
                 Ok(quote! {
                     let equal = self == new;
-                    if equal {
+                    // Move any signal and event fields into their old container.
-                        #move_event_handlers
+                    #move_event_handlers
+                    // If they are not equal, we need to move over all the fields to self
+                    if !equal {
+                        *self = new.clone();
                     }
                     equal
                 })

packages/core-macro/tests/values_memoize_in_place.rs

@@ -0,0 +1,75 @@
+use dioxus::prelude::*;
+
+thread_local! {
+    static DROP_COUNT: std::cell::RefCell<usize> = const { std::cell::RefCell::new(0) };
+}
+
+#[test]
+fn values_memoize_in_place() {
+    let mut dom = VirtualDom::new(app);
+
+    dom.rebuild_in_place();
+    dom.mark_dirty(ScopeId::ROOT);
+    for _ in 0..20 {
+        dom.render_immediate(&mut dioxus_core::NoOpMutations);
+    }
+    dom.render_immediate(&mut dioxus_core::NoOpMutations);
+    // As we rerun the app, the drop count should be 15 one for each render of the app component
+    let drop_count = DROP_COUNT.with(|c| *c.borrow());
+    assert_eq!(drop_count, 15);
+}
+
+struct CountsDrop;
+
+impl Drop for CountsDrop {
+    fn drop(&mut self) {
+        DROP_COUNT.with(|c| *c.borrow_mut() += 1);
+    }
+}
+
+fn app() -> Element {
+    let mut count = use_signal(|| 0);
+    let x = CountsDrop;
+
+    if generation() < 15 {
+        count += 1;
+    }
+
+    rsx! {
+        TakesEventHandler {
+            click: move |num| {
+                // Force the closure to own the drop counter
+                let _ = &x;
+                println!("num is {num}");
+            },
+            children: count() / 2
+        }
+        TakesSignal { sig: count(), children: count() / 2 }
+    }
+}
+
+#[component]
+fn TakesEventHandler(click: EventHandler<usize>, children: usize) -> Element {
+    let first_render_click = use_hook(move || click);
+    if generation() > 0 {
+        // Make sure the event handler is memoized in place and never gets dropped
+        first_render_click(children);
+    }
+
+    rsx! {
+        button { "{children}" }
+    }
+}
+
+#[component]
+fn TakesSignal(sig: ReadOnlySignal<usize>, children: usize) -> Element {
+    let first_render_sig = use_hook(move || sig);
+    if generation() > 0 {
+        // Make sure the signal is memoized in place and never gets dropped
+        println!("{first_render_sig}");
+    }
+
+    rsx! {
+        button { "{children}" }
+    }
+}

packages/core/src/any_props.rs

@@ -11,7 +11,7 @@ pub(crate) type BoxedAnyProps = Box<dyn AnyProps>;
 pub(crate) trait AnyProps: 'static {
     /// Render the component with the internal props.
     fn render(&self) -> RenderReturn;
-    /// Check if the props are the same as the type erased props of another component.
+    /// Make the old props equal to the new type erased props. Return if the props were equal and should be memoized.
     fn memoize(&mut self, other: &dyn Any) -> bool;
     /// Get the props as a type erased `dyn Any`.
     fn props(&self) -> &dyn Any;

packages/core/src/diff/component.rs

@@ -83,9 +83,6 @@ impl VNode {
             return;
         }
 
-        // First, move over the props from the old to the new, dropping old props in the process
-        dom.scopes[scope_id.0].props = new.props.duplicate();
-
         // Now run the component and diff it
         let new = dom.run_scope(scope_id);
         dom.diff_scope(to, scope_id, new);

packages/core/src/properties.rs

@@ -31,7 +31,7 @@ pub trait Properties: Clone + Sized + 'static {
     /// Create a builder for this component.
     fn builder() -> Self::Builder;
 
-    /// Compare two props to see if they are memoizable.
+    /// Make the old props equal to the new props. Return if the props were equal and should be memoized.
     fn memoize(&mut self, other: &Self) -> bool;
 
     /// Create a component from the props.

packages/hooks/tests/memo.rs

@@ -5,7 +5,7 @@ async fn memo_updates() {
     use dioxus::prelude::*;
 
     thread_local! {
-        static VEC_SIGNAL: RefCell<Option<Signal<Vec<usize>, SyncStorage>>> = RefCell::new(None);
+        static VEC_SIGNAL: RefCell<Option<Signal<Vec<usize>, SyncStorage>>> = const { RefCell::new(None) };
     }
 
     fn app() -> Element {