From 65fe3fcf47547cbfe4960d276b59601e1f57245e Mon Sep 17 00:00:00 2001
From: Aaron Smith <83140718+centau@users.noreply.github.com>
Date: Thu, 16 Nov 2023 18:01:45 +0000
Subject: [PATCH] Fix some graph edge cases

---
 CHANGELOG.md    | 14 ++++++++-
 src/graph.luau  | 25 ++++++---------
 test/tests.luau | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 558b398..5ea3ffa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,7 +8,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
 
--
+### Added
+
+- Batched updates with `batch()`.
+
+### Changed
+
+- Improved graph updating algorithm.
+- Graph nodes no longer destroy children; only owned.
+
+### Fixed
+
+- Graph edge case where a destroyed node can be readded if it was queued for
+  evaluation before being destroyed.
 
 --------------------------------------------------------------------------------
 
diff --git a/src/graph.luau b/src/graph.luau
index e32f3d0..9288a82 100644
--- a/src/graph.luau
+++ b/src/graph.luau
@@ -99,27 +99,16 @@ end
 
 local function find_and_swap_pop<T>(t: { T }, v: T)
     local idx = table.find(t, v) :: number
-    --assert(idx, "value not found")
     local n = #t
     t[idx] = t[n]
     t[n] = nil
 end
 
-local function remove_child<T>(parent: StartNode<T>, child: Node<T>)
-    find_and_swap_pop(parent, child)
-end
-
-local function disown<T>(node: Node<T>)
-    if node.owner then
-        find_and_swap_pop(node.owner.owned :: { Node<T> }, node)
-    end
-end
-
 local function unparent<T>(node: Node<T>)
     local parents = node.parents
 
     for i, parent in next, parents do
-        remove_child(parent, node)
+        find_and_swap_pop(parent, node)
         parents[i] = nil
     end
 end
@@ -127,13 +116,16 @@ end
 local function destroy<T>(node: Node<T>)
     run_cleanups(node)
     unparent(node)
-    disown(node)
+    
+    if node.owner then
+        find_and_swap_pop(node.owner.owned :: { Node<T> }, node)
+        node.owner = false
+    end
 
     if node.owned then
         local owned = node.owned
         while owned[1] do destroy(owned[1]) end
     end
-    while node[1] do destroy(node[1]) end
 end
 
 local function destroy_owned<T>(node: Node<T>)
@@ -202,7 +194,7 @@ local function flush_update_queue()
         local node = update_queue[i]
         --assert(node.effect)
 
-        if evaluate_node(node) then
+        if node.owner and evaluate_node(node) then
             queue_children(node)
         end
 
@@ -224,7 +216,8 @@ local function update<T>(root: StartNode<T>)
         local node = update_queue[i]
         --assert(node.effect)
 
-        if evaluate_node(node) then
+        -- check if node is still owned in case destroyed after queued
+        if node.owner and evaluate_node(node) then
             queue_children(node)
         end
 
diff --git a/test/tests.luau b/test/tests.luau
index 16fa50d..9cd6afe 100644
--- a/test/tests.luau
+++ b/test/tests.luau
@@ -1923,6 +1923,7 @@ TEST("graph edge cases", wrap_root(function()
     local source = vide.source
     local derive = vide.derive
     local effect = vide.effect
+    local root = vide.root
 
     do CASE "diamond A,B,C,D"
         --[[
@@ -2006,6 +2007,89 @@ TEST("graph edge cases", wrap_root(function()
         CHECK(b() == 2)
         CHECK(count == 2)
     end
+
+    do CASE "do not destroy children"
+        local parent = source(0)   
+
+        local
+            destroy,
+            parent_to_destroy,
+            update_parent_to_destroy
+        = root(function(destroy)
+            local src = source(0)
+            return
+                destroy,
+                derive(function() return src() end),
+                src
+        end)
+
+        local count = 0
+
+        effect(function()
+            count += 1
+            parent()
+            parent_to_destroy()
+        end)
+
+        parent(parent() + 1)
+        CHECK(count == 2)
+        update_parent_to_destroy(1)
+        CHECK(count == 3)
+
+        destroy()
+        
+        update_parent_to_destroy(2)
+        CHECK(count == 3)
+        
+        parent(parent() + 1)
+        CHECK(count == 4)
+    end
+
+    do CASE "double destroy"
+        -- issue:
+        -- parent evaluates
+        -- child A queued
+        -- child B queued
+        -- child A destroys child B
+        -- child B reevaluates due to already being queued
+        -- parent destroys, destroys child B - uh oh
+
+        local
+            destroy_parent,
+            parent,
+            update_parent
+        = root(function(destroy)
+            local src = source(0)
+            return
+                destroy,
+                derive(function() return src() end),
+                src
+        end)
+
+        local destroy_child, _child_B = function() end, nil
+
+        local count_A = 0
+    
+        -- child_A
+        effect(function()
+            count_A += 1
+            parent()
+            destroy_child()
+        end)
+
+        local count_B = 0
+        destroy_child, _child_B = root(function(destroy)
+            return
+                destroy,
+                derive(function() count_B += 1; return parent() end)
+        end)
+
+        update_parent(parent() + 1)
+        CHECK(count_A == 2)
+        CHECK(count_B == 1) -- child B should not run again
+        destroy_parent() -- should not error
+        CHECK(true)
+    end
 end))
 
 TEST("strict", wrap_root(function()