Fix memo dirty flag after unrelated writes (#2992)

* Fix memo dirty flag after unrelated writes

* improve regression test

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <matt@hunzinger.me>

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <matt@hunzinger.me>

* Update packages/signals/tests/memo.rs

Co-authored-by: Matt Hunzinger <matt@hunzinger.me>

---------

Co-authored-by: Matt Hunzinger <matt@hunzinger.me>

packages/hooks/src/use_memo.rs

@@ -1,6 +1,6 @@
 use crate::use_callback;
 use dioxus_core::prelude::*;
-use dioxus_signals::{Memo, Signal};
+use dioxus_signals::Memo;
 
 #[doc = include_str!("../docs/derived_state.md")]
 #[doc = include_str!("../docs/rules_of_hooks.md")]
@@ -8,6 +8,7 @@ use dioxus_signals::{Memo, Signal};
 #[track_caller]
 pub fn use_memo<R: PartialEq>(mut f: impl FnMut() -> R + 'static) -> Memo<R> {
     let callback = use_callback(move |_| f());
+    let caller = std::panic::Location::caller();
     #[allow(clippy::redundant_closure)]
-    use_hook(|| Signal::memo(move || callback(())))
+    use_hook(|| Memo::new_with_location(move || callback(()), caller))
 }

packages/signals/src/memo.rs

@@ -139,10 +139,11 @@ impl<T: 'static> Memo<T> {
             drop(peak);
             let mut copy = self.inner;
             copy.set(new_value);
-            update_write
-                .dirty
-                .store(false, std::sync::atomic::Ordering::Relaxed);
         }
+        // Always mark the memo as no longer dirty even if the value didn't change
+        update_write
+            .dirty
+            .store(false, std::sync::atomic::Ordering::Relaxed);
     }
 
     /// Get the scope that the signal was created in.

packages/signals/src/signal.rs

@@ -557,6 +557,7 @@ struct SignalSubscriberDrop<T: 'static, S: Storage<SignalData<T>>> {
 #[allow(clippy::no_effect)]
 impl<T: 'static, S: Storage<SignalData<T>>> Drop for SignalSubscriberDrop<T, S> {
     fn drop(&mut self) {
+        println!("dropping signal subscriber");
         #[cfg(debug_assertions)]
         {
             tracing::trace!(

packages/signals/tests/memo.rs

@@ -1,148 +1,178 @@
-// TODO: fix #1935
+#![allow(unused, non_upper_case_globals, non_snake_case)]
+use dioxus::html::p;
+use dioxus::prelude::*;
+use dioxus_core::ElementId;
+use dioxus_core::NoOpMutations;
+use dioxus_signals::*;
+use std::cell::RefCell;
+use std::collections::HashMap;
+use std::rc::Rc;
+use std::sync::atomic::{AtomicBool, Ordering};
 
-// #![allow(unused, non_upper_case_globals, non_snake_case)]
+#[test]
-// use dioxus_core::NoOpMutations;
+fn memos_rerun() {
-// use std::collections::HashMap;
+    let _ = simple_logger::SimpleLogger::new().init();
-// use std::rc::Rc;
 
-// use dioxus::html::p;
+    #[derive(Default)]
-// use dioxus::prelude::*;
+    struct RunCounter {
-// use dioxus_core::ElementId;
+        component: usize,
-// use dioxus_signals::*;
+        effect: usize,
-// use std::cell::RefCell;
+    }
 
-// #[test]
+    let counter = Rc::new(RefCell::new(RunCounter::default()));
-// fn memos_rerun() {
+    let mut dom = VirtualDom::new_with_props(
-//     let _ = simple_logger::SimpleLogger::new().init();
+        |counter: Rc<RefCell<RunCounter>>| {
+            counter.borrow_mut().component += 1;
 
-//     #[derive(Default)]
+            let mut signal = use_signal(|| 0);
-//     struct RunCounter {
+            let memo = use_memo({
-//         component: usize,
+                to_owned![counter];
-//         effect: usize,
+                move || {
-//     }
+                    counter.borrow_mut().effect += 1;
+                    println!("Signal: {:?}", signal);
+                    signal()
+                }
+            });
+            assert_eq!(memo(), 0);
+            signal += 1;
+            assert_eq!(memo(), 1);
 
-//     let counter = Rc::new(RefCell::new(RunCounter::default()));
+            rsx! {
-//     let mut dom = VirtualDom::new_with_props(
+                div {}
-//         |counter: Rc<RefCell<RunCounter>>| {
+            }
-//             counter.borrow_mut().component += 1;
+        },
+        counter.clone(),
+    );
 
-//             let mut signal = use_signal(|| 0);
+    dom.rebuild_in_place();
-//             let memo = use_memo({
-//                 to_owned![counter];
-//                 move || {
-//                     counter.borrow_mut().effect += 1;
-//                     println!("Signal: {:?}", signal);
-//                     signal()
-//                 }
-//             });
-//             assert_eq!(memo(), 0);
-//             signal += 1;
-//             assert_eq!(memo(), 1);
 
-//             rsx! {
+    let current_counter = counter.borrow();
-//                 div {}
+    assert_eq!(current_counter.component, 1);
-//             }
+    assert_eq!(current_counter.effect, 2);
-//         },
+}
-//         counter.clone(),
-//     );
 
-//     dom.rebuild_in_place();
+#[test]
+fn memos_prevents_component_rerun() {
+    let _ = simple_logger::SimpleLogger::new().init();
 
-//     let current_counter = counter.borrow();
+    #[derive(Default)]
-//     assert_eq!(current_counter.component, 1);
+    struct RunCounter {
-//     assert_eq!(current_counter.effect, 2);
+        component: usize,
-// }
+        memo: usize,
+    }
 
-// #[test]
+    let counter = Rc::new(RefCell::new(RunCounter::default()));
-// fn memos_prevents_component_rerun() {
+    let mut dom = VirtualDom::new_with_props(
-//     let _ = simple_logger::SimpleLogger::new().init();
+        |props: Rc<RefCell<RunCounter>>| {
+            let mut signal = use_signal(|| 0);
 
-//     #[derive(Default)]
+            if generation() == 1 {
-//     struct RunCounter {
+                *signal.write() = 0;
-//         component: usize,
+            }
-//         memo: usize,
+            if generation() == 2 {
-//     }
+                println!("Writing to signal");
+                *signal.write() = 1;
+            }
 
-//     let counter = Rc::new(RefCell::new(RunCounter::default()));
+            rsx! {
-//     let mut dom = VirtualDom::new_with_props(
+                Child {
-//         |props: Rc<RefCell<RunCounter>>| {
+                    signal: signal,
-//             let mut signal = use_signal(|| 0);
+                    counter: props.clone(),
+                }
+            }
+        },
+        counter.clone(),
+    );
 
-//             if generation() == 1 {
+    #[derive(Default, Props, Clone)]
-//                 *signal.write() = 0;
+    struct ChildProps {
-//             }
+        signal: Signal<usize>,
-//             if generation() == 2 {
+        counter: Rc<RefCell<RunCounter>>,
-//                 println!("Writing to signal");
+    }
-//                 *signal.write() = 1;
-//             }
 
-//             rsx! {
+    impl PartialEq for ChildProps {
-//                 Child {
+        fn eq(&self, other: &Self) -> bool {
-//                     signal: signal,
+            self.signal == other.signal
-//                     counter: props.clone(),
+        }
-//                 }
+    }
-//             }
-//         },
-//         counter.clone(),
-//     );
 
-//     #[derive(Default, Props, Clone)]
+    fn Child(props: ChildProps) -> Element {
-//     struct ChildProps {
+        let counter = &props.counter;
-//         signal: Signal<usize>,
+        let signal = props.signal;
-//         counter: Rc<RefCell<RunCounter>>,
+        counter.borrow_mut().component += 1;
-//     }
 
-//     impl PartialEq for ChildProps {
+        let memo = use_memo({
-//         fn eq(&self, other: &Self) -> bool {
+            to_owned![counter];
-//             self.signal == other.signal
+            move || {
-//         }
+                counter.borrow_mut().memo += 1;
-//     }
+                println!("Signal: {:?}", signal);
+                signal()
+            }
+        });
+        match generation() {
+            0 => {
+                assert_eq!(memo(), 0);
+            }
+            1 => {
+                assert_eq!(memo(), 1);
+            }
+            _ => panic!("Unexpected generation"),
+        }
 
-//     fn Child(props: ChildProps) -> Element {
+        rsx! {
-//         let counter = &props.counter;
+            div {}
-//         let signal = props.signal;
+        }
-//         counter.borrow_mut().component += 1;
+    }
 
-//         let memo = use_memo({
+    dom.rebuild_in_place();
-//             to_owned![counter];
+    dom.mark_dirty(ScopeId::APP);
-//             move || {
+    dom.render_immediate(&mut NoOpMutations);
-//                 counter.borrow_mut().memo += 1;
-//                 println!("Signal: {:?}", signal);
-//                 signal()
-//             }
-//         });
-//         match generation() {
-//             0 => {
-//                 assert_eq!(memo(), 0);
-//             }
-//             1 => {
-//                 assert_eq!(memo(), 1);
-//             }
-//             _ => panic!("Unexpected generation"),
-//         }
 
-//         rsx! {
+    {
-//             div {}
+        let current_counter = counter.borrow();
-//         }
+        assert_eq!(current_counter.component, 1);
-//     }
+        assert_eq!(current_counter.memo, 2);
+    }
 
-//     dom.rebuild_in_place();
+    dom.mark_dirty(ScopeId::APP);
-//     dom.mark_dirty(ScopeId::ROOT);
+    dom.render_immediate(&mut NoOpMutations);
-//     dom.render_immediate(&mut NoOpMutations);
+    dom.render_immediate(&mut NoOpMutations);
 
-//     {
+    {
-//         let current_counter = counter.borrow();
+        let current_counter = counter.borrow();
-//         assert_eq!(current_counter.component, 1);
+        assert_eq!(current_counter.component, 2);
-//         assert_eq!(current_counter.memo, 2);
+        assert_eq!(current_counter.memo, 3);
-//     }
+    }
+}
 
-//     dom.mark_dirty(ScopeId::ROOT);
+// Regression test for https://github.com/DioxusLabs/dioxus/issues/2990
-//     dom.render_immediate(&mut NoOpMutations);
+#[test]
-//     dom.render_immediate(&mut NoOpMutations);
+fn memos_sync_rerun_after_unrelated_write() {
+    static PASSED: AtomicBool = AtomicBool::new(false);
+    let mut dom = VirtualDom::new(|| {
+        let mut signal = use_signal(|| 0);
+        let memo = use_memo(move || dbg!(signal() < 2));
+        if generation() == 0 {
+            assert!(memo());
+            signal += 1;
+        } else {
+            // It should be fine to hold the write and read the memo at the same time
+            let mut write = signal.write();
+            println!("Memo: {:?}", memo());
+            assert!(memo());
+            *write = 2;
+            drop(write);
+            assert!(!memo());
+            PASSED.store(true, std::sync::atomic::Ordering::SeqCst);
+        }
 
-//     {
+        rsx! {
-//         let current_counter = counter.borrow();
+            div {}
-//         assert_eq!(current_counter.component, 2);
+        }
-//         assert_eq!(current_counter.memo, 3);
+    });
-//     }
+
-// }
+    dom.rebuild_in_place();
+    dom.mark_dirty(ScopeId::APP);
+    dom.render_immediate(&mut NoOpMutations);
+    assert!(PASSED.load(Ordering::SeqCst));
+}