From b24666fc57b32187b83210a128ac9af9b58ee1c8 Mon Sep 17 00:00:00 2001 From: Kaan <81116098+kaan650@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:31:04 +0300 Subject: [PATCH 1/4] Keep the graph consistent when effects or cleanups error (#100) * Keep the graph consistent when effects or cleanups error * Fix effect updating its own source looping in strict mode --- src/graph.luau | 102 ++++++++++++++++++++++++++++-------------------- test/tests.luau | 74 +++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 42 deletions(-) diff --git a/src/graph.luau b/src/graph.luau index b85b66e..be76dc1 100644 --- a/src/graph.luau +++ b/src/graph.luau @@ -98,15 +98,20 @@ local function push_cleanup(node: Node, cleanup: () -> ()) end end -local function flush_cleanups(node: Node) +-- runs every cleanup even if one errors, returns the first error +local function flush_cleanups(node: Node): 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 }, v: T) @@ -125,12 +130,15 @@ local function unparent(node: Node) end end -local function destroy(node: Node) +local destroy_owned: (node: Node) -> string? + +-- always fully destroys the node, returns the first cleanup error +local function destroy_node(node: Node): 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(node: Node) node.owner = false end - if node.owned then - local owned = node.owned - while owned[1] do destroy(owned[1]) end - end + local owned_err = destroy_owned(node) + return err or owned_err end -local function destroy_owned(node: Node) +function destroy_owned(node: Node): 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(node: Node) + local err = destroy_node(node) + if err then error(err, 0) end end local update_queue = { n = 0 } :: { n: number, [number]: Node } -local function evaluate_node(node: Node) +-- returns whether the cached value changed, and an error instead of throwing +local function try_evaluate_node(node: Node): (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) - destroy_owned(node) + local cleanup_err = flush_cleanups(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) - update_queue.n = 0 - error(`effect error stacktrace\n{new_value :: string}`, 0) + return false, `effect error stacktrace\n{new_value :: string}` 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) - destroy_owned(node) + local cleanup_err = flush_cleanups(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) - update_queue.n = 0 - error(new_value, 0) + return false, new_value 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(node: Node): boolean + local changed, err = try_evaluate_node(node) + if err then error(err, 0) end + return changed +end + local function queue_children_for_update(node: SourceNode) 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 - queue_children_for_update(node) + -- check if node is still owned in case destroyed after queued + 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(root: SourceNode) @@ -239,21 +271,7 @@ local function update_descendants(root: SourceNode) if flags.batch then return end - local i = n0 + 1 - 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 + flush_update_queue(n0) end local function push_scope_as_child_of(node: SourceNode) diff --git a/test/tests.luau b/test/tests.luau index cd9cd1d..dd07517 100644 --- a/test/tests.luau +++ b/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() @@ -3112,6 +3171,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 From c2bd2c2d4c6f02b92e5084971452514c5452da38 Mon Sep 17 00:00:00 2001 From: Kaan <81116098+kaan650@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:33:40 +0300 Subject: [PATCH 2/4] Error when getting an unset context without a default (#101) Co-authored-by: centau <83140718+centau@users.noreply.github.com> --- docs/api/reactivity-utility.md | 2 ++ src/context.luau | 2 +- test/tests.luau | 10 ++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/api/reactivity-utility.md b/docs/api/reactivity-utility.md index b8bff4d..c93b786 100644 --- a/docs/api/reactivity-utility.md +++ b/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. diff --git a/src/context.luau b/src/context.luau index 2ce3eff..ca9f8fb 100644 --- a/src/context.luau +++ b/src/context.luau @@ -40,7 +40,7 @@ local function context(...: T): Context 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) diff --git a/test/tests.luau b/test/tests.luau index dd07517..9aa2da1 100644 --- a/test/tests.luau +++ b/test/tests.luau @@ -2828,6 +2828,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() From a75e3ee8430d208d5f77fe993f530706b3922d0e Mon Sep 17 00:00:00 2001 From: Kaan <81116098+kaan650@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:38:05 +0300 Subject: [PATCH 3/4] Fix children being unparented after moving to another parent (#102) Co-authored-by: centau <83140718+centau@users.noreply.github.com> --- src/implicit_effect.luau | 6 +++--- test/tests.luau | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/implicit_effect.luau b/src/implicit_effect.luau index 6fed9bc..402abb7 100644 --- a/src/implicit_effect.luau +++ b/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) diff --git a/test/tests.luau b/test/tests.luau index 9aa2da1..c634d8a 100644 --- a/test/tests.luau +++ b/test/tests.luau @@ -1109,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 From 673105c51a9041f3861e551cfbea39de11ccc861 Mon Sep 17 00:00:00 2001 From: Kaan <81116098+kaan650@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:56:56 +0300 Subject: [PATCH 4/4] Fix destroyed components staying in the output when a delayed cleanup errors (#103) Co-authored-by: centau <83140718+centau@users.noreply.github.com> --- src/indexes.luau | 3 ++- src/switch.luau | 3 ++- src/values.luau | 3 ++- test/tests.luau | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/indexes.luau b/src/indexes.luau index fe8416e..1189973 100644 --- a/src/indexes.luau +++ b/src/indexes.luau @@ -101,9 +101,10 @@ local function indexes( scope.value = nil -- set to nil for the `scope.value ~= v` check if scope.timeout == nil then scope.timeout = timeout(scope.delay, function() -- todo: possible redundant updates - scope.destroy() + local ok, err = pcall(scope.destroy) scopes[i] = nil update_output() + if not ok then error(err, 0) end end) end end diff --git a/src/switch.luau b/src/switch.luau index 0e74932..bb4a1f1 100644 --- a/src/switch.luau +++ b/src/switch.luau @@ -50,9 +50,10 @@ local function switch_map( else if scope.timeout == nil then scope.timeout = timeout(scope.delay, function() - scope.destroy() + local ok, err = pcall(scope.destroy) scopes[k] = nil update_output() + if not ok then error(err, 0) end end) end end diff --git a/src/values.luau b/src/values.luau index 33bc5e4..b59fd0e 100644 --- a/src/values.luau +++ b/src/values.luau @@ -111,9 +111,10 @@ local function values( scope.index = nil -- set to nil for the `scope.index ~= i` check if scope.timeout == nil then scope.timeout = timeout(scope.delay, function() -- todo: possible redundant updates - scope.destroy() + local ok, err = pcall(scope.destroy) scopes[v] = nil update_output() + if not ok then error(err, 0) end end) end end diff --git a/test/tests.luau b/test/tests.luau index c634d8a..8cda87b 100644 --- a/test/tests.luau +++ b/test/tests.luau @@ -1625,6 +1625,25 @@ TEST("switch()", wrap_root(function() do CASE "delay" -- probably unneeded because show() uses switch() internally end + + do CASE "delay with cleanup error" + local input = source(1) + local errored = false + + local output = switch(input) { + [1] = function() + cleanup(function() + if not errored then errored = true; error("cleanup error") end + end) + return 1, 1 + end, + [2] = function() return 2 end + } + + input(2) + CHECK(not pcall(step, 1 + 0.01)) + CHECK(output() == 2) + end end)) TEST("indexes()", wrap_root(function() @@ -1895,6 +1914,23 @@ TEST("indexes()", wrap_root(function() CHECK(cleaned_counts[2] == 1) end end + + do CASE "delay with cleanup error" + local input = source { 1 } + local errored = false + + local output = indexes(input, function() + cleanup(function() + if not errored then errored = true; error("cleanup error") end + end) + return {}, 1 + end) + + input {} + CHECK(#output() == 1) + CHECK(not pcall(step, 1 + 0.01)) + CHECK(#output() == 0) + end end)) TEST("values()", wrap_root(function() @@ -2110,6 +2146,23 @@ TEST("values()", wrap_root(function() --CHECK(count == 3) CHECK(count == 5) end + + do CASE "delay with cleanup error" + local input = source { 1 } + local errored = false + + local output = values(input, function() + cleanup(function() + if not errored then errored = true; error("cleanup error") end + end) + return {}, 1 + end) + + input {} + CHECK(#output() == 1) + CHECK(not pcall(step, 1 + 0.01)) + CHECK(#output() == 0) + end end)) TEST("spring()", wrap_root(function()