Improve signia error handling (#2835)

This PR revamps how errors in signia are handled.

This was brought about by a situation that @MitjaBezensek encountered
where he added a reactor to a shape util class. During fuzz tests, that
reactor was being executed at times when the Editor was not in a usable
state (we had a minor hole in our sync rebase logic that allowed this,
fixed elsewhere) and the reactor was throwing errors because it
dereferenced a parent signal that relied on the page state
(getShapesInCurrentPage or whatever) when there were no page records in
the store.

The strange part was that even if we wrapped the body of the reactor
function in a try/catch, ignoring the error, we'd still see the error
bubble up somehow.

That was because the error was being thrown in a Computed derive
function, and those are evaluated independently (i.e. outside of the
reactor function) by signia as it traverses the dependency graph from
leaves to roots in the `haveParentsChanged()` internal function.

So the immediate fix was to make it so that `haveParentsChanged` ignores
errors somehow.

But the better fix involved completely revamping how signia handles
errors, and they work very much like how signia handles values now. i.e.

- signia still assumes that deriver functions are pure, and that if a
deriver function throws once it will throw again unless its parent
signals change value, so **it caches thrown errors for computed values**
and throws them again if .get() is called again before the parents
change
- it clears the history buffer if an error is thrown
- it does not allow errors to bubble during dirty checking i.e. inside
`haveParentsChanged` or while calculating diffs.

### Change Type

- [x] `patch` — Bug fix
- [ ] `minor` — New feature
- [ ] `major` — Breaking change
- [ ] `dependencies` — Changes to package dependencies[^1]
- [ ] `documentation` — Changes to the documentation only[^2]
- [ ] `tests` — Changes to any test code only[^2]
- [ ] `internal` — Any other changes that don't affect the published
package[^2]
- [ ] I don't know

[^1]: publishes a `patch` release, for devDependencies use `internal`
[^2]: will not publish a new version

### Test Plan

1. Add a step-by-step description of how to test your PR here.
2.

- [x] Unit Tests
- [ ] End to end tests

### Release Notes

- Add a brief release note for your PR here.

packages/state/api-report.md

@@ -95,7 +95,7 @@ export type RESET_VALUE = typeof RESET_VALUE;
 
 // @public
 export interface Signal<Value, Diff = unknown> {
-    __unsafe__getWithoutCapture(): Value;
+    __unsafe__getWithoutCapture(ignoreErrors?: boolean): Value;
     // @internal (undocumented)
     children: ArraySet<Child>;
     get(): Value;

packages/state/api/api.json

@@ -1910,7 +1910,15 @@
               "excerptTokens": [
                 {
                   "kind": "Content",
-                  "text": "__unsafe__getWithoutCapture(): "
+                  "text": "__unsafe__getWithoutCapture(ignoreErrors?: "
+                },
+                {
+                  "kind": "Content",
+                  "text": "boolean"
+                },
+                {
+                  "kind": "Content",
+                  "text": "): "
                 },
                 {
                   "kind": "Content",
@@ -1923,12 +1931,21 @@
               ],
               "isOptional": false,
               "returnTypeTokenRange": {
-                "startIndex": 1,
-                "endIndex": 2
+                "startIndex": 3,
+                "endIndex": 4
               },
               "releaseTag": "Public",
               "overloadIndex": 1,
-              "parameters": [],
+              "parameters": [
+                {
+                  "parameterName": "ignoreErrors",
+                  "parameterTypeTokenRange": {
+                    "startIndex": 1,
+                    "endIndex": 2
+                  },
+                  "isOptional": true
+                }
+              ],
               "name": "__unsafe__getWithoutCapture"
             },
             {

packages/state/src/lib/core/Atom.ts

@@ -95,7 +95,7 @@ class __Atom__<Value, Diff = unknown> implements Atom<Value, Diff> {
 
 	historyBuffer?: HistoryBuffer<Diff>
 
-	__unsafe__getWithoutCapture(): Value {
+	__unsafe__getWithoutCapture(_ignoreErrors?: boolean): Value {
 		return this.current
 	}
 

packages/state/src/lib/core/Computed.ts

@@ -155,6 +155,8 @@ class __UNSAFE__Computed<Value, Diff = unknown> implements Computed<Value, Diff>
 
 	// The last-computed value of this signal.
 	private state: Value = UNINITIALIZED as unknown as Value
+	// If the signal throws an error we stash it so we can rethrow it on the next get()
+	private error: null | { thrownValue: any } = null
 
 	private computeDiff?: ComputeDiff<Value, Diff>
 
@@ -181,20 +183,29 @@ class __UNSAFE__Computed<Value, Diff = unknown> implements Computed<Value, Diff>
 		this.isEqual = options?.isEqual ?? equals
 	}
 
-	__unsafe__getWithoutCapture(): Value {
+	__unsafe__getWithoutCapture(ignoreErrors?: boolean): Value {
 		const isNew = this.lastChangedEpoch === GLOBAL_START_EPOCH
 
 		if (!isNew && (this.lastCheckedEpoch === getGlobalEpoch() || !haveParentsChanged(this))) {
 			this.lastCheckedEpoch = getGlobalEpoch()
-			return this.state
+			if (this.error) {
+				if (!ignoreErrors) {
+					throw this.error.thrownValue
+				} else {
+					return this.state // will be UNINITIALIZED
+				}
+			} else {
+				return this.state
+			}
 		}
 
 		try {
 			startCapturingParents(this)
 			const result = this.derive(this.state, this.lastCheckedEpoch)
 			const newState = result instanceof WithDiff ? result.value : result
-			if (this.state === UNINITIALIZED || !this.isEqual(newState, this.state)) {
-				if (this.historyBuffer && !isNew) {
+			const isUninitialized = this.state === UNINITIALIZED
+			if (isUninitialized || !this.isEqual(newState, this.state)) {
+				if (this.historyBuffer && !isUninitialized) {
 					const diff = result instanceof WithDiff ? result.diff : undefined
 					this.historyBuffer.pushEntry(
 						this.lastChangedEpoch,
@@ -207,8 +218,24 @@ class __UNSAFE__Computed<Value, Diff = unknown> implements Computed<Value, Diff>
 				this.lastChangedEpoch = getGlobalEpoch()
 				this.state = newState
 			}
+			this.error = null
 			this.lastCheckedEpoch = getGlobalEpoch()
 
+			return this.state
+		} catch (e) {
+			// if a derived value throws an error, we reset the state to UNINITIALIZED
+			if (this.state !== UNINITIALIZED) {
+				this.state = UNINITIALIZED as unknown as Value
+				this.lastChangedEpoch = getGlobalEpoch()
+			}
+			this.lastCheckedEpoch = getGlobalEpoch()
+			// we also clear the history buffer if an error was thrown
+			if (this.historyBuffer) {
+				this.historyBuffer.clear()
+			}
+			this.error = { thrownValue: e }
+			// we don't wish to propagate errors when derefed via haveParentsChanged()
+			if (!ignoreErrors) throw e
 			return this.state
 		} finally {
 			stopCapturingParents()
@@ -216,15 +243,19 @@ class __UNSAFE__Computed<Value, Diff = unknown> implements Computed<Value, Diff>
 	}
 
 	get(): Value {
-		const value = this.__unsafe__getWithoutCapture()
-		maybeCaptureParent(this)
-		return value
+		try {
+			return this.__unsafe__getWithoutCapture()
+		} finally {
+			// if the deriver throws an error we still need to capture
+			maybeCaptureParent(this)
+		}
 	}
 
 	getDiffSince(epoch: number): RESET_VALUE | Diff[] {
-		// need to call .get() to ensure both that this derivation is up to date
-		// and that tracking happens correctly
-		this.get()
+		// we can ignore any errors thrown during derive
+		this.__unsafe__getWithoutCapture(true)
+		// and we still need to capture this signal as a parent
+		maybeCaptureParent(this)
 
 		if (epoch >= this.lastChangedEpoch) {
 			return EMPTY_ARRAY

packages/state/src/lib/core/__tests__/errors.test.ts

@@ -0,0 +1,165 @@
+import { atom } from '../Atom'
+import { computed } from '../Computed'
+import { EffectScheduler, react } from '../EffectScheduler'
+import { haveParentsChanged } from '../helpers'
+import { getGlobalEpoch, transact } from '../transactions'
+import { RESET_VALUE } from '../types'
+
+describe('reactors that error', () => {
+	it('will not roll back the atom value', () => {
+		const a = atom('', 1)
+		react('', () => {
+			if (a.get() === 2) throw new Error('test')
+		})
+		expect(() => a.set(2)).toThrowErrorMatchingInlineSnapshot(`"test"`)
+		expect(a.get()).toBe(2)
+	})
+	it('will not roll back the changes in a transaction', () => {
+		const a = atom('', 1)
+		const b = atom('', 2)
+
+		react('', () => {
+			if (a.get() + b.get() === 4) throw new Error('test')
+		})
+
+		expect(() =>
+			transact(() => {
+				a.set(3)
+				b.set(1)
+			})
+		).toThrowErrorMatchingInlineSnapshot(`"test"`)
+
+		expect(a.get()).toBe(3)
+		expect(b.get()).toBe(1)
+	})
+})
+
+describe('derivations that error', () => {
+	it('will cache thrown values', () => {
+		let numComputations = 0
+		const a = atom('', 1)
+		const b = computed('', () => {
+			numComputations++
+			if (a.get() === 2) throw new Error('test')
+			return a.get()
+		})
+
+		expect(b.get()).toBe(1)
+		expect(numComputations).toBe(1)
+
+		a.set(2)
+
+		expect(() => b.get()).toThrowErrorMatchingInlineSnapshot(`"test"`)
+		expect(numComputations).toBe(2)
+		expect(() => b.get()).toThrowErrorMatchingInlineSnapshot(`"test"`)
+		expect(numComputations).toBe(2)
+		expect(() => b.get()).toThrowErrorMatchingInlineSnapshot(`"test"`)
+		expect(numComputations).toBe(2)
+
+		a.set(3)
+
+		expect(b.get()).toBe(3)
+		expect(b.get()).toBe(3)
+	})
+
+	it('will not trigger effects if they continue to error', () => {
+		const a = atom('', 1)
+		let numComputations = 0
+		const b = computed('', () => {
+			numComputations++
+			if (a.get() % 2 === 0) throw new Error('test')
+			return a.get()
+		})
+
+		let numReactions = 0
+		react('', () => {
+			try {
+				b.get()
+			} catch (e) {
+				// ignore
+			}
+			numReactions++
+		})
+		expect(numReactions).toBe(1)
+		expect(numComputations).toBe(1)
+
+		a.set(2)
+
+		expect(numReactions).toBe(2)
+		expect(numComputations).toBe(2)
+
+		a.set(4)
+
+		expect(numComputations).toBe(3)
+		expect(numReactions).toBe(2)
+
+		a.set(3)
+
+		expect(numComputations).toBe(4)
+		expect(numReactions).toBe(3)
+	})
+
+	it('clears the history buffer when an error is thrown', () => {
+		const a = atom('', 1)
+		const b = computed(
+			'',
+			() => {
+				if (a.get() === 5) throw new Error('test')
+				return a.get()
+			},
+			{
+				historyLength: 10,
+				computeDiff: (a, b) => {
+					return b - a
+				},
+			}
+		)
+
+		expect(b.get()).toBe(1)
+		const startEpoch = getGlobalEpoch()
+
+		a.set(2)
+		expect(b.get()).toBe(2)
+		expect(b.getDiffSince(startEpoch)).toEqual([1])
+
+		a.set(4)
+		expect(b.get()).toBe(4)
+		expect(b.getDiffSince(startEpoch)).toEqual([1, 2])
+
+		a.set(5)
+
+		expect(() => b.get()).toThrowErrorMatchingInlineSnapshot(`"test"`)
+		expect(b.getDiffSince(startEpoch)).toEqual(RESET_VALUE)
+		const errorEpoch = getGlobalEpoch()
+
+		a.set(6)
+		expect(b.get()).toBe(6)
+		expect(b.getDiffSince(errorEpoch)).toEqual(RESET_VALUE)
+		expect(b.getDiffSince(errorEpoch + 1)).toEqual([])
+
+		a.set(7)
+		expect(b.get()).toBe(7)
+		expect(b.getDiffSince(errorEpoch)).toEqual(RESET_VALUE)
+		expect(b.getDiffSince(errorEpoch + 1)).toEqual([1])
+	})
+})
+
+test('haveParentsChanged will not throw if one of the parents is throwing', () => {
+	const a = atom('', 1)
+	const scheduler = new EffectScheduler('', () => {
+		a.get()
+		throw new Error('test')
+	})
+	expect(() => {
+		scheduler.attach()
+		scheduler.execute()
+	}).toThrowErrorMatchingInlineSnapshot(`"test"`)
+
+	expect(haveParentsChanged(scheduler)).toBe(false)
+
+	expect(() => a.set(2)).toThrowErrorMatchingInlineSnapshot(`"test"`)
+
+	// haveParentsChanged should still be false because it already
+	// executed the effect and it errored
+	expect(haveParentsChanged(scheduler)).toBe(false)
+})

packages/state/src/lib/core/capture.ts

@@ -164,7 +164,7 @@ export function whyAmIRunning() {
 				'\t',
 				(changedParent as any).name,
 				'changed =>',
-				changedParent.__unsafe__getWithoutCapture()
+				changedParent.__unsafe__getWithoutCapture(true)
 			)
 		}
 	}

packages/state/src/lib/core/helpers.ts

@@ -19,7 +19,7 @@ function isChild(x: any): x is Child {
 export function haveParentsChanged(child: Child) {
 	for (let i = 0, n = child.parents.length; i < n; i++) {
 		// Get the parent's value without capturing it.
-		child.parents[i].__unsafe__getWithoutCapture()
+		child.parents[i].__unsafe__getWithoutCapture(true)
 
 		// If the parent's epoch does not match the child's view of the parent's epoch, then the parent has changed.
 		if (child.parents[i].lastChangedEpoch !== child.parentEpochs[i]) {

packages/state/src/lib/core/types.ts

@@ -43,7 +43,7 @@ export interface Signal<Value, Diff = unknown> {
 	 * Returns the current value of the signal without capturing it as a dependency.
 	 * Use this if you need to retrieve the signal's value in a hot loop where the performance overhead of dependency tracking is too high.
 	 */
-	__unsafe__getWithoutCapture(): Value
+	__unsafe__getWithoutCapture(ignoreErrors?: boolean): Value
 	/** @internal */
 	children: ArraySet<Child>
 }