Merge branch 'main' into fix-delayed-destroy-error

docs/api/reactivity-utility.md

@@ -131,6 +131,8 @@ Creates a new context.
 
     Calling `context()` returns a new context function.
     Call this function with no arguments to get the context value.
+    If no context is set, the default value is returned, or an error is thrown
+    if no default was given.
     Call this function with a value and a function to create a new context with
     the given value.
 

src/context.luau

@@ -40,7 +40,7 @@ local function context<T>(...: T): Context<T>
                 return (if value ~= nil_symbol then value else nil) :: T
             end
 
-            if has_default ~= nil then
+            if has_default then
                 return default_value
             else
                 error("attempt to get context when no context is set and no default context is set", 0)

src/graph.luau

@@ -98,15 +98,20 @@ local function push_cleanup<T>(node: Node<T>, cleanup: () -> ())
     end
 end
 
-local function flush_cleanups<T>(node: Node<T>)
+-- runs every cleanup even if one errors, returns the first error
+local function flush_cleanups<T>(node: Node<T>): string?
+    local first_err: string?
+
     if node.cleanups then
         for _, fn in node.cleanups do
             local ok, err: string? = xpcall(fn, debug.traceback)
-            if not ok then error(`cleanup error: {err}`, 0) end
+            if not ok and not first_err then first_err = `cleanup error: {err}` end
         end
 
         table.clear(node.cleanups)
     end
+
+    return first_err
 end
 
 local function find_and_swap_pop<T>(t: { T }, v: T)
@@ -125,12 +130,15 @@ local function unparent<T>(node: Node<T>)
     end
 end
 
-local function destroy<T>(node: Node<T>)
+local destroy_owned: <T>(node: Node<T>) -> string?
+
+-- always fully destroys the node, returns the first cleanup error
+local function destroy_node<T>(node: Node<T>): string?
     if flags.strict and table.find(scopes, node) then
         error("attempt to destroy an active scope", 0)
     end
 
-    flush_cleanups(node)
+    local err = flush_cleanups(node)
     unparent(node)
     
     if node.owner then
@@ -138,70 +146,85 @@ local function destroy<T>(node: Node<T>)
         node.owner = false
     end
 
-    if node.owned then
+    local owned_err = destroy_owned(node)
-        local owned = node.owned
+    return err or owned_err
-        while owned[1] do destroy(owned[1]) end
-    end
 end
 
-local function destroy_owned<T>(node: Node<T>)
+function destroy_owned<T>(node: Node<T>): string?
+    local err: string?
+
     if node.owned then
         local owned = node.owned
-        while owned[1] do destroy(owned[1]) end
+        while owned[1] do
+            local owned_err = destroy_node(owned[1])
+            err = err or owned_err
+        end
     end
+
+    return err
+end
+
+local function destroy<T>(node: Node<T>)
+    local err = destroy_node(node)
+    if err then error(err, 0) end
 end
 
 local update_queue = { n = 0 } :: { n: number, [number]: Node<any> }
 
-local function evaluate_node<T>(node: Node<T>)
+-- returns whether the cached value changed, and an error instead of throwing
+local function try_evaluate_node<T>(node: Node<T>): (boolean, string?)
     if flags.strict then
         if table.find(scopes, node) then
-            error("a scope, that should rerun due to the update of a source, is already active", 0)
+            return false, "a scope, that should rerun due to the update of a source, is already active"
         end
 
         local initial_value = node.cache
+        local err: string?
 
         for i = 1, 2 do
             local cur_value = node.cache
 
-            flush_cleanups(node)
+            local cleanup_err = flush_cleanups(node)
-            destroy_owned(node)
+            local owned_err = destroy_owned(node)
+            err = err or cleanup_err or owned_err
     
             push_scope(node)
             local ok, new_value = ycall(node.effect :: (T) -> T, cur_value)
             pop_scope()
             
             if not ok then
-                table.clear(update_queue)
+                return false, `effect error stacktrace\n{new_value :: string}`
-                update_queue.n = 0
-                error(`effect error stacktrace\n{new_value :: string}`, 0)
             end
 
             node.cache = new_value :: T
         end
 
-        return initial_value ~= node.cache
+        return initial_value ~= node.cache, err
     else
         local cur_value = node.cache
 
-        flush_cleanups(node)
+        local cleanup_err = flush_cleanups(node)
-        destroy_owned(node)
+        local owned_err = destroy_owned(node)
 
         push_scope(node)
         local ok, new_value = xpcall(node.effect :: (T) -> T, effect_traceback, node.cache)
         pop_scope()
 
         if not ok then
-            table.clear(update_queue)
+            return false, new_value
-            update_queue.n = 0
-            error(new_value, 0)
         end
     
         node.cache = new_value
-        return cur_value ~= new_value
+        return cur_value ~= new_value, cleanup_err or owned_err
     end
 end
 
+local function evaluate_node<T>(node: Node<T>): boolean
+    local changed, err = try_evaluate_node(node)
+    if err then error(err, 0) end
+    return changed
+end
+
 local function queue_children_for_update<T>(node: SourceNode<T>)
     local i = update_queue.n
     while node[1] do
@@ -216,21 +239,30 @@ local function get_update_queue_length()
     return update_queue.n
 end
 
+-- queued nodes are already unparented, so every one must be evaluated even if
+-- another errors; the first error is thrown after the queue is flushed
 local function flush_update_queue(from: number)
+    local first_err: string?
+
     local i = from + 1
     while i <= update_queue.n do
         local node = update_queue[i]
         --assert(node.effect)
 
-        if node.owner and evaluate_node(node) then
+        -- check if node is still owned in case destroyed after queued
-            queue_children_for_update(node)
+        if node.owner then
+            local changed, err = try_evaluate_node(node)
+            if changed then queue_children_for_update(node) end
+            first_err = first_err or err
         end
 
-        update_queue[i] = false :: any
+        update_queue[i] = false :: any -- false instead of nil to avoid sparse
         i += 1
     end
     
     update_queue.n = from
+
+    if first_err then error(first_err, 0) end
 end
 
 local function update_descendants<T>(root: SourceNode<T>)
@@ -239,21 +271,7 @@ local function update_descendants<T>(root: SourceNode<T>)
 
     if flags.batch then return end
 
-    local i = n0 + 1
+    flush_update_queue(n0)
-    while i <= update_queue.n do
-        local node = update_queue[i]
-        --assert(node.effect)
-
-        -- check if node is still owned in case destroyed after queued
-        if node.owner and evaluate_node(node) then
-            queue_children_for_update(node)
-        end
-
-        update_queue[i] = false :: any -- false instead of nil to avoid sparse
-        i += 1
-    end
-
-    update_queue.n = n0
 end
 
 local function push_scope_as_child_of<T>(node: SourceNode<T>)

src/implicit_effect.luau

@@ -62,7 +62,7 @@ local function update_children_effect(p: {
 
             push_cleanup(assert(get_scope()), function()
                 for child in node.cache.cur_children_set do
-                    child.Parent = nil
+                    if child.Parent == p.instance then child.Parent = nil end
                 end
             end)
         end
@@ -71,7 +71,7 @@ local function update_children_effect(p: {
     process_child(new_children)
 
     for child in cur_children_set do
-        child.Parent = nil -- unparent all children that weren't in the new children set
+        if child.Parent == p.instance then child.Parent = nil end -- unparent all children that weren't in the new children set
     end
 
     table.clear(cur_children_set) -- clear cache, preserve capacity
@@ -116,7 +116,7 @@ return {
 
         push_cleanup(assert_stable_scope(), function()
             for child in node.cache.cur_children_set do
-                child.Parent = nil
+                if child.Parent == instance then child.Parent = nil end
             end
         end)
 

test/tests.luau

@@ -633,6 +633,25 @@ TEST("effect()", wrap_root(function()
 
         CHECK(count == 2)
     end
+
+    do CASE "error does not stop other effects"
+        local src = source(0)
+
+        local count = 0
+        effect(function()
+            if src() == 1 then error("effect error") end
+        end)
+        effect(function()
+            src()
+            count += 1
+        end)
+
+        CHECK(count == 1)
+        CHECK(not pcall(src, 1))
+        CHECK(count == 2)
+        src(2)
+        CHECK(count == 3)
+    end
 end))
 
 TEST("cleanup()", wrap_root(function()
@@ -709,6 +728,46 @@ TEST("cleanup()", wrap_root(function()
         CHECK(ran.destroy)
         CHECK(ran.Destroy)
     end
+
+    do CASE "error does not stop destruction"
+        local src = source(0)
+
+        local cleaned = false
+        local count = 0
+
+        local destroy = root(function()
+            cleanup(function() error("cleanup error") end)
+            cleanup(function() cleaned = true end)
+            effect(function()
+                src()
+                count += 1
+            end)
+        end)
+
+        CHECK(not pcall(destroy))
+        CHECK(cleaned)
+        src(1)
+        CHECK(count == 1)
+    end
+
+    do CASE "error on rerun does not stop updates"
+        local src = source(0)
+
+        local count = 0
+        effect(function()
+            src()
+            cleanup(function() if src() == 1 then error("cleanup error") end end)
+        end)
+        effect(function()
+            src()
+            count += 1
+        end)
+
+        CHECK(not pcall(src, 1))
+        CHECK(count == 2)
+        src(2)
+        CHECK(count == 3)
+    end
 end))
 
 TEST("create()", wrap_root(function()
@@ -1050,6 +1109,33 @@ TEST("create()", wrap_root(function()
         CHECK(parent:FindFirstChild "c")
     end
 
+    do CASE "children moved to another parent"
+        local other = create "Frame" {}
+
+        local child = create "Frame" {}
+        local children_a = source { child }
+        local children_b = source {}
+        create "Frame" { children_a }
+        local parent_b = create "Frame" { children_b }
+
+        children_b { child }
+        children_a {}
+        CHECK(child.Parent == parent_b)
+
+        local a = create "Frame" {}
+        local b = create "Frame" {}
+        local destroy = root(function()
+            create "Frame" { function() return a end }
+            create "Frame" { function() return { function() return b end } end }
+        end)
+
+        a.Parent = other
+        b.Parent = other
+        destroy()
+        CHECK(a.Parent == other)
+        CHECK(b.Parent == other)
+    end
+
     do CASE "garbage collection test"
         local wref
 
@@ -2822,6 +2908,16 @@ TEST("context()", function()
             end)
         end)
     end
+
+    do CASE "get context without default"
+        local ctx = context()
+
+        CHECK(not pcall(ctx))
+
+        root(function()
+            CHECK(not pcall(ctx))
+        end)
+    end
 end)
 
 TEST("nested effects cases", function()
@@ -3165,6 +3261,21 @@ TEST("strict", wrap_root(function()
         CHECK(count == 4)
     end
 
+    do CASE "effect updating its own source"
+        local src = source(0)
+        local count = 0
+
+        effect(function()
+            count += 1
+            if count > 100 then return end
+            local v = src()
+            if v > 0 then src(v + 1) end
+        end)
+
+        CHECK(not pcall(src, 1))
+        CHECK(count < 100)
+    end
+
     do CASE "destruction of active scope"
         local src = source(false)
         local count = 0