Merge branch 'main' into fix-action-priority

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.
 

docs/api/strict-mode.md

@@ -16,11 +16,10 @@ Currently, strict mode will:
 
 1. Run reactive scopes twice when a source updates.
 2. Throw an error if yields occur where they are not allowed.
-3. Checks for `indexes()` and `values()` outputting primitive values.
+3. Checks for `values()` input having duplicate values.
-4. Checks for `values()` input having duplicate values.
+4. Checks for duplicate nested properties at same depth.
-5. Checks for duplicate nested properties at same depth.
+5. Checks for destruction of an active scope.
-6. Checks for destruction of an active scope.
+6. Better error reporting and stack traces.
-7. Better error reporting and stack traces.
 
 By rerunning reactive scopes twice each time they update, it helps ensure that
 computations are pure, and that any cleanup is done correctly.

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

@@ -21,10 +21,11 @@ export type Node<T> =  {
 
 local scopes = { n = 0 } :: { [number]: Node<any>, n: number } -- scopes stack
 
-local function efn(err: string)
+local function efn(err: unknown)
-    local trace = debug.traceback(err, 2)
+    local message = tostring(err)
+    local trace = debug.traceback(message, 2)
 
-    if string.find(err, "^effect error stacktrace") then -- if effect error is nested
+    if string.find(message, "^effect error stacktrace") then -- if effect error is nested
         trace = string.gsub("    " .. trace, "\n", function() -- indent entire error
             return "\n    "
         end)
@@ -98,15 +99,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 +131,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 +147,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 +240,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 +272,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)
 

src/indexes.luau

@@ -101,9 +101,10 @@ local function indexes<K, V, Obj>(
                     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

src/switch.luau

@@ -50,9 +50,10 @@ local function switch_map<K, Obj>(
             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

src/values.luau

@@ -111,9 +111,10 @@ local function values<K, V, Obj>(
                     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

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
 
@@ -1539,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()
@@ -1809,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()
@@ -2024,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()
@@ -2783,6 +2922,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()
@@ -3061,6 +3210,15 @@ TEST("strict", wrap_root(function()
         CHECK(not ok)
     end
 
+    do CASE "error with a non-string value"
+        local ok, err = pcall(effect, function()
+            error({})
+        end)
+
+        CHECK(not ok)
+        CHECK(string.find(err, "table: "))
+    end
+
     do CASE "duplicate properties"
         local ok = pcall(function()
             create "TextLabel" {
@@ -3126,6 +3284,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