From eab89077f6267ec6435c881a75c57d023d85f5e9 Mon Sep 17 00:00:00 2001 From: littensy <56808540+littensy@users.noreply.github.com> Date: Sat, 11 Apr 2026 13:06:53 -0700 Subject: [PATCH] refactor: improve error traceback --- packages/charm/src/init.luau | 89 ++++++---- packages/charm/test/effect.test.luau | 11 -- packages/charm/test/effectScope.test.luau | 9 - packages/charm/test/errors.test.luau | 200 ++++++++++++++++++++++ packages/charm/test/init.luau | 4 + packages/charm/test/observe.test.luau | 28 +++ 6 files changed, 285 insertions(+), 56 deletions(-) create mode 100644 packages/charm/test/errors.test.luau diff --git a/packages/charm/src/init.luau b/packages/charm/src/init.luau index 9a2ae5b..ec4570c 100644 --- a/packages/charm/src/init.luau +++ b/packages/charm/src/init.luau @@ -92,32 +92,43 @@ local flags = { trackInnerEffects = true, } -local traceback = debug.traceback :: (string?) -> string - --[[ Wraps a function to prevent yielding when in strict mode. If the function - attempts to yield, an error is thrown. + was created by this module, it will be excluded to omit unnecessary Charm + internals from stack traces. - @param callback The function to wrap. + @param callback The user-created function to wrap. @return A wrapped function that cannot yield in strict mode. ]] -local function noYield(callback: (Args...) -> Results...): (Args...) -> Results... +local function wrapUserSpace(callback: (Args...) -> Results...): (Args...) -> Results... if not flags.strict then return callback end - local iterator = function(...): { any } - return { callback(...) } + -- If the callback is already wrapped, avoid double-wrapping to simplify + -- stack traces + if debug.info(1, "s") == debug.info(callback, "s") then + return callback + end + + local handler = function(thread: thread, ok: boolean, ...: Results...): Results... + if not ok then + local err = (...) + if type(err) == "string" then + error(debug.traceback(thread, err), 0) + else + error(tostring(err), 0) + end + elseif coroutine.status(thread) ~= "dead" then + error(debug.traceback(thread, "Attempted to yield in an effect or scope"), 0) + else + return ... + end end return function(...: Args...): Results... - if coroutine.isyieldable() then - for results in iterator, ... do - return unpack(results) - end - else - return callback(...) - end + local thread = coroutine.create(callback) + return handler(thread, coroutine.resume(thread, ...)) end end @@ -187,7 +198,7 @@ local function runCleanups(cleanups: { Cleanup }) cleanups[index] = nil local prevSub = setActiveSub(nil) - local success, err = xpcall(cleanup :: () -> nil, traceback) + local success, err = pcall(cleanup) activeSub = prevSub if not success then @@ -225,7 +236,7 @@ local function run(effect: EffectNode) effect.flags = WATCHING local prevSub = setActiveSub(effect) - local success, result = xpcall(effect.fn, traceback) + local success, result = pcall(effect.fn) activeSub = prevSub purgeDeps(effect) @@ -233,7 +244,7 @@ local function run(effect: EffectNode) if not success then error(result, 0) elseif result then - table.insert(effect.cleanups, noYield(result)) + table.insert(effect.cleanups, wrapUserSpace(result)) end if effect.flags == NONE then @@ -311,7 +322,7 @@ local function updateComputed(computed: ComputedNode): boolean local prevSub = setActiveSub(computed) local oldValue = computed.value - local success, newValue = xpcall(computed.getter, traceback, oldValue) + local success, newValue = pcall(computed.getter, oldValue) activeSub = prevSub computed.flags = bit32.band(computed.flags, bit32.bnot(RECURSED_CHECK)) @@ -466,14 +477,14 @@ local function signal(initialValue: T, equals: Equals?): (Getter, Sette deepFreeze(initialValue) end - local function writeSignal(newValue: Update): T + local function signalSetter(update: Update): T local value: T -- Determine the new value based on whether the update is a function or -- a direct value - if type(newValue) == "function" then + if type(update) == "function" then local prevSub = setActiveSub(nil) - local success, result = xpcall(noYield(newValue), traceback, node.pendingValue) + local success, result = pcall(wrapUserSpace(update), node.pendingValue) activeSub = prevSub if success then @@ -482,7 +493,7 @@ local function signal(initialValue: T, equals: Equals?): (Getter, Sette error(result, 2) end else - value = newValue + value = update end -- Check if the new value is different from the pending value @@ -506,7 +517,7 @@ local function signal(initialValue: T, equals: Equals?): (Getter, Sette return node.currentValue end - local function readSignal(): T + local function signalGetter(): T -- Check if the signal has a pending update that needs to be applied if bit32.btest(node.flags, DIRTY) then if updateSignal(node) then @@ -532,7 +543,7 @@ local function signal(initialValue: T, equals: Equals?): (Getter, Sette return node.currentValue end - return readSignal, writeSignal + return signalGetter, signalSetter end --[[ @@ -564,7 +575,7 @@ end local function computed(getter: (previousValue: T?) -> T): Getter local node: ComputedNode = { flags = NONE, - getter = noYield(getter), + getter = wrapUserSpace(getter), } local function computedOper(): T @@ -589,7 +600,7 @@ local function computed(getter: (previousValue: T?) -> T): Getter node.flags = bit32.bor(MUTABLE, RECURSED_CHECK) local prevSub = setActiveSub(node) - local success, result = xpcall(node.getter, traceback) + local success, result = pcall(node.getter) activeSub = prevSub node.flags = bit32.band(node.flags, bit32.bnot(RECURSED_CHECK)) @@ -628,7 +639,7 @@ end ]] local function effect(fn: () -> ...Cleanup?): Cleanup local node: EffectNode = { - fn = noYield(fn), + fn = wrapUserSpace(fn), flags = WATCHING, cleanups = {}, } @@ -642,14 +653,14 @@ local function effect(fn: () -> ...Cleanup?): Cleanup startBatch() local prevSub = setActiveSub(node) - local success, result = xpcall(node.fn, traceback) + local success, result = pcall(node.fn) activeSub = prevSub if not success then endBatch() error(result, 2) elseif result then - table.insert(node.cleanups, noYield(result)) + table.insert(node.cleanups, wrapUserSpace(result)) end endBatch() @@ -679,13 +690,13 @@ local function effectScope(fn: () -> ...Cleanup?, detached: boolean?): Cleanup end local prevSub = setActiveSub(node) - local success, result = xpcall(noYield(fn), traceback) + local success, result = pcall(wrapUserSpace(fn)) activeSub = prevSub if not success then error(result, 2) elseif result then - table.insert(node.cleanups, noYield(result)) + table.insert(node.cleanups, wrapUserSpace(result)) end return function() @@ -706,7 +717,7 @@ local function trigger(fn: () -> ...any) } local prevSub = setActiveSub(sub) - local success, err = xpcall(noYield(fn), traceback) + local success, err = pcall(wrapUserSpace(fn)) activeSub = prevSub if not success then @@ -739,7 +750,7 @@ end ]] local function onCleanup(fn: Cleanup, failSilently: boolean?) if activeSub and (activeSub :: EffectNode).cleanups then - table.insert((activeSub :: EffectNode).cleanups, noYield(fn)) + table.insert((activeSub :: EffectNode).cleanups, wrapUserSpace(fn)) elseif not failSilently then warn(debug.traceback("onCleanup() can only be called inside an effect or a scope.", 2)) end @@ -761,7 +772,7 @@ end local function untracked(fn: (Args...) -> Results..., ...: Args...): Results... if activeSub then local prevSub = setActiveSub(nil) - local results: { any } = { xpcall(noYield(fn), traceback, ...) } + local results: { any } = { pcall(wrapUserSpace(fn), ...) } activeSub = prevSub if not results[1] then @@ -785,7 +796,7 @@ end ]] local function batch(fn: (Args...) -> Results..., ...: Args...): Results... startBatch() - local results: { any } = { xpcall(noYield(fn), traceback, ...) } + local results: { any } = { pcall(wrapUserSpace(fn), ...) } endBatch() if not results[1] then @@ -805,7 +816,7 @@ end ]] local function isSignal(fn: Getter): boolean local name = debug.info(fn, "n") - return name == "readSignal" or name == "computedOper" or name == "atomOper" + return name == "signalGetter" or name == "computedOper" or name == "atomOper" end --[[ @@ -819,6 +830,7 @@ end @see https://github.com/littensy/charm?tab=readme-ov-file#listengetter-callback ]] local function listen(getter: Getter, callback: (state: T, lastState: T?) -> ()): Cleanup + local callback = wrapUserSpace(callback) local tracked: Getter = if isSignal(getter) then getter else computed(getter :: (T?) -> T) local current: T @@ -840,6 +852,7 @@ end @see https://github.com/littensy/charm?tab=readme-ov-file#subscribegetter-callback ]] local function subscribe(getter: Getter, callback: (state: T, lastState: T) -> ()): Cleanup + local callback = wrapUserSpace(callback) local tracked: Getter = if isSignal(getter) then getter else computed(getter :: (T?) -> T) local current: T local firstRun = true @@ -870,6 +883,8 @@ end @see https://github.com/littensy/charm?tab=readme-ov-file#observegetter-callback ]] local function observe(getter: Getter<{ [K]: V }>, callback: (item: V, key: K) -> ...Cleanup?): Cleanup + local getter = wrapUserSpace(getter) + local callback = wrapUserSpace(callback) local scopes: { [K]: Cleanup } = {} local stopObserving = false @@ -928,6 +943,8 @@ end @see https://github.com/littensy/charm?tab=readme-ov-file#mappedgetter-transform ]] local function mapped(getter: Getter<{ [KI]: VI }>, transform: (VI, KI) -> (VO, KO)): Getter<{ [KO]: VO }> + local getter = wrapUserSpace(getter) + local transform = wrapUserSpace(transform) local nextInput: { [KI]: VI } = {} local keyMap: { [KI]: KO } = {} diff --git a/packages/charm/test/effect.test.luau b/packages/charm/test/effect.test.luau index def4c68..d697513 100644 --- a/packages/charm/test/effect.test.luau +++ b/packages/charm/test/effect.test.luau @@ -408,17 +408,6 @@ test("should run cleanups outside any evaluation context", function() assert(calls == 0, "should not run after b update") end) -test.skip("infinite recursion should eventually throw", function() - local counter, setCounter = signal(0) - - local success = pcall(function() - effect(function() - setCounter(counter() + 1) - end) - end) - assert(not success, "should have thrown") -end) - test("should not retrigger the effect if the cleanup modifies one of the dependencies", function() local a, setA = signal(0) local calls = {} diff --git a/packages/charm/test/effectScope.test.luau b/packages/charm/test/effectScope.test.luau index c1c06c2..964b7d5 100644 --- a/packages/charm/test/effectScope.test.luau +++ b/packages/charm/test/effectScope.test.luau @@ -92,13 +92,4 @@ test("should dispose inner effects if created in an effect", function() assert(triggers == 2, "ran after dispose") end) -test("should disallow yielding", function() - local success = pcall(function() - effectScope(function() - coroutine.yield() - end) - end) - assert(not success, "should error on yield") -end) - return {} diff --git a/packages/charm/test/errors.test.luau b/packages/charm/test/errors.test.luau new file mode 100644 index 0000000..151a0d5 --- /dev/null +++ b/packages/charm/test/errors.test.luau @@ -0,0 +1,200 @@ +--!optimize 1 + +local Charm = require("../src") +local test = require("../../../tests/test") + +test("should disallow yielding", function() + local success = pcall(function() + Charm.effectScope(function() + coroutine.yield() + end) + end) + assert(not success, "should error on yield") +end) + +test.describe("traceback", function() + local DEFAULT_ERROR = "Test error" + + local function fail(message: string?) + error(message or DEFAULT_ERROR, 0) + end + + local function indirectError(message: string?) + fail(message) + end + + local function assertStackTrace(err: unknown, expectation: string?) + expectation = expectation or DEFAULT_ERROR + assert(type(err) == "string", "Expected error to be a string") + assert(string.find(err, expectation), `Expected error message to be "{expectation}", got: {err}`) + assert(string.find(err, "fail"), `Expected stack trace to contain fail, got: {err}`) + assert(string.find(err, "indirectError"), `Expected stack trace to contain indirectError, got: {err}`) + end + + test("signal update", function() + local _, err = pcall(function() + local _, setSrc = Charm.signal(0) + setSrc(function() + indirectError() + return 1 + end) + end) + assertStackTrace(err) + end) + + test("atom update", function() + local _, err = pcall(function() + local src = Charm.atom(0) + src(function() + indirectError() + return 1 + end) + end) + assertStackTrace(err) + end) + + test("computed", function() + local _, err = pcall(function() + local src = Charm.computed(function() + indirectError() + end) + src() + end) + assertStackTrace(err) + end) + + test("effect init", function() + local _, err = pcall(function() + Charm.effect(function() + indirectError() + end) + end) + assertStackTrace(err) + end) + + test("effect run", function() + local _, err = pcall(function() + local src, setSrc = Charm.signal(0) + Charm.effect(function() + if src() == 1 then + indirectError() + end + end) + setSrc(1) + end) + assertStackTrace(err) + end) + + test("effect cleanups", function() + local _, err = pcall(function() + local dispose = Charm.effect(function() + Charm.onCleanup(function() + indirectError("onCleanup") + end) + return function() + indirectError("returned cleanup") + end + end) + dispose() + end) + assertStackTrace(err, "onCleanup") + assertStackTrace(err, "returned cleanup") + end) + + test("trigger", function() + local _, err = pcall(function() + Charm.trigger(function() + indirectError() + end) + end) + assertStackTrace(err) + end) + + test("untracked", function() + local _, err = pcall(function() + Charm.effectScope(function() + Charm.untracked(function() + indirectError() + end) + end) + end) + assertStackTrace(err) + end) + + test("batch", function() + local _, err = pcall(function() + Charm.batch(function() + indirectError() + end) + end) + assertStackTrace(err) + end) + + test("subscribe getter", function() + local _, err = pcall(function() + Charm.subscribe(function() + indirectError() + end, function() end) + end) + assertStackTrace(err) + end) + + test("subscribe callback", function() + local _, err = pcall(function() + local src, setSrc = Charm.signal(0) + Charm.subscribe(src, function() + indirectError() + end) + setSrc(1) + end) + assertStackTrace(err) + end) + + test("listen getter", function() + local _, err = pcall(function() + Charm.listen(function() + indirectError() + end, function() end) + end) + assertStackTrace(err) + end) + + test("listen callback", function() + local _, err = pcall(function() + local src, setSrc = Charm.signal(0) + Charm.listen(src, function() + indirectError() + end) + setSrc(1) + end) + assertStackTrace(err) + end) + + test("mapped getter", function() + local _, err = pcall(function() + local src = Charm.mapped(function() + indirectError() + return { 1 } + end, function() + return 1 + end) + src() + end) + assertStackTrace(err) + end) + + test("mapped transform", function() + local _, err = pcall(function() + local src = Charm.mapped(function() + return { 1 } + end, function() + indirectError() + return 1 + end) + src() + end) + assertStackTrace(err) + end) +end) + +return {} diff --git a/packages/charm/test/init.luau b/packages/charm/test/init.luau index e13fab7..52da435 100644 --- a/packages/charm/test/init.luau +++ b/packages/charm/test/init.luau @@ -12,6 +12,10 @@ test.describe("effectScope", function() require("@self/effectScope.test") end) +test.describe("error", function() + require("@self/errors.test") +end) + test.describe("gc", function() require("@self/gc.test") end) diff --git a/packages/charm/test/observe.test.luau b/packages/charm/test/observe.test.luau index 223b1b0..cd01b83 100644 --- a/packages/charm/test/observe.test.luau +++ b/packages/charm/test/observe.test.luau @@ -167,4 +167,32 @@ test("should allow disposal during removal", function() assert(table.concat(edits) == "+1+2+3+4-1-2-3-4", `bad edits: {table.concat(edits)}`) end) +test("should not hang on error after recursive update", function() + local items, setItems = signal({}) + + observe(items, function() + setItems({ {} }) + error("test error") + end) + + local success, err = pcall(function() + setItems({ {} }) + end) + assert(not success, "should error") + assert(string.find(tostring(err), "test error"), `bad error: {err}`) +end) + +test("should not hang on error after recursive init", function() + local items, setItems = signal({ {} }) + + local success, err = pcall(function() + observe(items, function() + setItems({ {} }) + error("test error") + end) + end) + assert(not success, "should error") + assert(string.find(tostring(err), "test error"), `bad error: {err}`) +end) + return {}