fix: let onchange callbacks write state, contain their errors, bound link growth

packages/svelte/src/internal/client/proxy.js

@@ -6,8 +6,7 @@ import {
 	update_version,
 	active_reaction,
 	set_update_version,
-	set_active_reaction,
+	set_active_reaction
-	untrack
 } from './runtime.js';
 import { destroy_effect, eager_effect } from './reactivity/effects.js';
 import {
@@ -21,10 +20,15 @@ import {
 	state as source,
 	set,
 	increment,
-	flush_eager_effects,
+	set_eager_effects_deferred,
-	set_eager_effects_deferred
+	unset_eager_effects_deferred
 } from './reactivity/sources.js';
-import { COMPONENT_SYMBOL, PROXY_META_SYMBOL, PROXY_PATH_SYMBOL, STATE_SYMBOL } from '#client/constants';
+import {
+	COMPONENT_SYMBOL,
+	PROXY_META_SYMBOL,
+	PROXY_PATH_SYMBOL,
+	STATE_SYMBOL
+} from '#client/constants';
 import { UNINITIALIZED } from '../../constants.js';
 import * as e from './errors.js';
 import { tag } from './dev/tracing.js';
@@ -39,7 +43,7 @@ const regex_is_valid_identifier = /^[a-zA-Z_$][a-zA-Z_$0-9]*$/;
  *   self: any;
  *   sources: Map<any, Source<any>>;
  *   links: Array<{ pm: ProxyMeta, k: any }>;
- *   fires: Array<{ cb: () => void, fire: () => void, e: Effect }>;
+ *   fires: Array<{ cb: () => void, n: Source<number>, e: Effect }>;
  *   observed: boolean;
  * }} ProxyMeta
  */
@@ -50,7 +54,7 @@ const regex_is_valid_identifier = /^[a-zA-Z_$][a-zA-Z_$0-9]*$/;
  * fork gating and deferral behaviour. The original callback is kept alongside so it
  * can be detached again by identity
  * @param {() => void} onchange
- * @returns {{ cb: () => void, fire: () => void, e: Effect }}
+ * @returns {{ cb: () => void, n: Source<number>, e: Effect }}
  */
 function create_fire(onchange) {
 	var notifier = source(0);
@@ -69,14 +73,40 @@ function create_fire(onchange) {
 		if (running) return;
 		running = true;
 
+		// the callback is user code, not part of the effect: it may write state
+		// (which `set` forbids inside eager effects) and must not track its reads
+		var previous_reaction = active_reaction;
+		set_active_reaction(null);
+
 		try {
-			untrack(onchange);
+			onchange();
 		} finally {
+			set_active_reaction(previous_reaction);
 			running = false;
 		}
 	});
 
-	return { cb: onchange, fire: () => increment(notifier), e: effect };
+	return { cb: onchange, n: notifier, e: effect };
+}
+
+/**
+ * Whether `link` still describes where `meta.self` sits in its parent: the parent's
+ * source for that key holds this proxy, or a user wrapper that forwards to it
+ * @param {{ pm: ProxyMeta, k: any }} link
+ * @param {ProxyMeta} meta
+ */
+function is_live(link, meta) {
+	var s = link.pm.sources.get(link.k);
+	if (s === undefined) return false;
+
+	var v = s.v;
+	return (
+		v === meta.self ||
+		(v !== null &&
+			typeof v === 'object' &&
+			STATE_SYMBOL in v &&
+			/** @type {any} */ (v)[PROXY_META_SYMBOL] === meta)
+	);
 }
 
 /**
@@ -94,10 +124,23 @@ function link_child(child, parent_meta, key) {
 
 	var links = meta.links;
 
-	for (var i = 0; i < links.length; i += 1) {
+	var linked = false;
-		if (links[i].pm === parent_meta && links[i].k === key) return;
+
+	// links to this parent under other keys survive only while those slots still hold
+	// the child, so index churn (`unshift`, `sort`) cannot grow the list
+	for (var i = links.length - 1; i >= 0; i -= 1) {
+		var link = links[i];
+		if (link.pm !== parent_meta) continue;
+
+		if (link.k === key) {
+			linked = true;
+		} else if (!is_live(link, meta)) {
+			links.splice(i, 1);
+		}
 	}
 
+	if (linked) return;
+
 	links.push({ pm: parent_meta, k: key });
 	observe(meta);
 }
@@ -119,54 +162,50 @@ function observe(meta) {
 }
 
 /**
- * Walks rootward from `meta`, verifying each link against the parent's backing source
+ * Walks rootward from `meta`, verifying each link against the parent's backing source.
- * (`parent.sources.get(key).v === child`). Dead links are pruned in place; live chains
+ * Dead links are pruned in place; live chains contribute their root notifiers to `fires`
- * contribute their root callbacks to `fires`
  * @param {ProxyMeta} meta
  * @param {Set<ProxyMeta>} visited
- * @param {Set<() => void>} fires
+ * @param {Set<Source<number>>} fires
  */
 function collect_roots(meta, visited, fires) {
 	if (visited.has(meta)) return;
 	visited.add(meta);
 
 	for (var i = 0; i < meta.fires.length; i += 1) {
-		fires.add(meta.fires[i].fire);
+		fires.add(meta.fires[i].n);
 	}
 
 	var links = meta.links;
 
 	for (var j = links.length - 1; j >= 0; j -= 1) {
-		var link = links[j];
+		if (is_live(links[j], meta)) {
-		var s = link.pm.sources.get(link.k);
+			collect_roots(links[j].pm, visited, fires);
-
-		if (
-			s !== undefined &&
-			(s.v === meta.self ||
-				// the slot may hold a user wrapper around this proxy, which forwards the meta lookup
-				(s.v !== null &&
-					typeof s.v === 'object' &&
-					STATE_SYMBOL in s.v &&
-					/** @type {any} */ (s.v)[PROXY_META_SYMBOL] === meta))
-		) {
-			collect_roots(link.pm, visited, fires);
 		} else {
 			links.splice(j, 1);
 		}
 	}
-
-	if (links.length === 0 && meta.fires.length === 0) {
-		meta.observed = false;
-	}
 }
 
 /** @param {ProxyMeta} meta */
 function notify_onchange(meta) {
-	/** @type {Set<() => void>} */
+	/** @type {Set<ProxyMeta>} */
+	var visited = new Set();
+	/** @type {Set<Source<number>>} */
 	var fires = new Set();
-	collect_roots(meta, new Set(), fires);
+	collect_roots(meta, visited, fires);
+
+	if (fires.size === 0) {
+		// nothing reachable fires any more (roots detached, or every route pruned), so the
+		// whole visited region leaves the observed state instead of walking on every write
+		for (var m of visited) {
+			m.links.length = 0;
+			m.observed = false;
+		}
+		return;
+	}
 
-	for (var fire of fires) fire();
+	for (var n of fires) increment(n);
 }
 
 /**
@@ -196,22 +235,34 @@ export function remove_onchange(value, onchange) {
 	}
 }
 
+/** @type {WeakMap<Function, Function>} */
+var batched_methods = new WeakMap();
+
 /**
- * Wraps an array mutating method so onchange roots fire once per method call
+ * Wraps an array mutating method so eager effects run once per method call rather than
- * rather than once per internal `set` (e.g. `push` writes an element and `length`)
+ * once per internal `set` (e.g. `push` writes an element and `length`). Memoised per
+ * method, so `arr.push === arr.push` holds and reads allocate nothing
  * @param {Function} fn
  */
 function batch_eager_method(fn) {
-	return function (/** @type {any[]} */ ...args) {
+	var wrapped = batched_methods.get(fn);
-		set_eager_effects_deferred();
 
-		try {
+	if (wrapped === undefined) {
-			// @ts-ignore
+		wrapped = function (/** @type {any[]} */ ...args) {
-			return fn.apply(this, args);
+			set_eager_effects_deferred();
-		} finally {
+
-			flush_eager_effects();
+			try {
-		}
+				// @ts-ignore
-	};
+				return fn.apply(this, args);
+			} finally {
+				unset_eager_effects_deferred();
+			}
+		};
+
+		batched_methods.set(fn, wrapped);
+	}
+
+	return wrapped;
 }
 
 /**
@@ -325,6 +376,8 @@ export function proxy(value, onchange) {
 				e.state_descriptors_fixed();
 			}
 			var s = sources.get(prop);
+			var changed = s === undefined || s.v !== descriptor.value;
+
 			if (s === undefined) {
 				with_parent(() => {
 					var s = source(descriptor.value, stack);
@@ -338,6 +391,11 @@ export function proxy(value, onchange) {
 				set(s, descriptor.value, true);
 			}
 
+			if (changed && meta !== null && meta.observed) {
+				link_child(/** @type {Source<any>} */ (sources.get(prop)).v, meta, prop);
+				notify_onchange(meta);
+			}
+
 			return true;
 		},
 
@@ -544,24 +602,18 @@ export function proxy(value, onchange) {
 					set(s, np);
 					changed = !has || target[prop] !== value;
 
-					if (meta !== null && meta.observed) {
-						link_child(np, /** @type {ProxyMeta} */ (meta), prop);
-					}
-
 					sources.set(prop, s);
 				}
 			} else {
 				has = s.v !== UNINITIALIZED;
 
-				var p = with_parent(() => proxy(value));
+				np = with_parent(() => proxy(value));
+				changed = s.v !== np;
+				set(s, np);
+			}
 
-				if (meta !== null && meta.observed) {
+			if (meta !== null && meta.observed) {
-					if (s.v !== p) changed = true;
+				link_child(np, meta, prop);
-					set(s, p);
-					link_child(p, meta, prop);
-				} else {
-					set(s, p);
-				}
 			}
 
 			var descriptor = Reflect.getOwnPropertyDescriptor(target, prop);
@@ -689,16 +741,7 @@ function inspectable_array(array) {
 				return value;
 			}
 
-			/**
+			return batch_eager_method(value);
-			 * @this {any[]}
-			 * @param {any[]} args
-			 */
-			return function (...args) {
-				set_eager_effects_deferred();
-				var result = value.apply(this, args);
-				flush_eager_effects();
-				return result;
-			};
 		}
 	});
 }

packages/svelte/src/internal/client/reactivity/sources.js

@@ -59,10 +59,19 @@ export function set_eager_effects(v) {
 	eager_effects = v;
 }
 
-let eager_effects_deferred = false;
+// a depth, so a mutating array method called from inside another keeps the outer one deferred
+let eager_effects_deferred = 0;
 
 export function set_eager_effects_deferred() {
-	eager_effects_deferred = true;
+	eager_effects_deferred += 1;
+}
+
+export function unset_eager_effects_deferred() {
+	eager_effects_deferred -= 1;
+
+	if (eager_effects_deferred === 0 && eager_effects.size > 0) {
+		flush_eager_effects();
+	}
 }
 
 /**
@@ -259,7 +268,7 @@ export function internal_set(source, value, updated_during_traversal = null) {
 			}
 		}
 
-		if (!batch.is_fork && eager_effects.size > 0 && !eager_effects_deferred) {
+		if (!batch.is_fork && eager_effects.size > 0 && eager_effects_deferred === 0) {
 			flush_eager_effects();
 		}
 	}
@@ -268,32 +277,33 @@ export function internal_set(source, value, updated_during_traversal = null) {
 }
 
 export function flush_eager_effects() {
-	eager_effects_deferred = false;
+	try {
-
+		for (const effect of eager_effects) {
-	for (const effect of eager_effects) {
+			// Mark clean inspect-effects as maybe dirty and then check their dirtiness
-		// Mark clean inspect-effects as maybe dirty and then check their dirtiness
+			// instead of just updating the effects - this way we avoid overfiring.
-		// instead of just updating the effects - this way we avoid overfiring.
+			if ((effect.f & CLEAN) !== 0) {
-		if ((effect.f & CLEAN) !== 0) {
+				set_signal_status(effect, MAYBE_DIRTY);
-			set_signal_status(effect, MAYBE_DIRTY);
+			}
-		}
 
-		let dirty;
+			let dirty;
 
-		try {
+			try {
-			dirty = is_dirty(effect);
+				dirty = is_dirty(effect);
-		} catch {
+			} catch {
-			// Dirty-checking can evaluate derived dependencies and throw in cases where
+				// Dirty-checking can evaluate derived dependencies and throw in cases where
-			// parent effects are about to destroy this eager effect. Run the effect so
+				// parent effects are about to destroy this eager effect. Run the effect so
-			// its own error handling can deal with transient failures.
+				// its own error handling can deal with transient failures.
-			dirty = true;
+				dirty = true;
-		}
+			}
 
-		if (dirty) {
+			if (dirty) {
-			update_effect(effect);
+				update_effect(effect);
+			}
 		}
+	} finally {
+		// an effect that throws must not stay queued, or the next unrelated write would rethrow it
+		eager_effects.clear();
 	}
-
-	eager_effects.clear();
 }
 
 /**

packages/svelte/tests/signals/onchange.test.ts

@@ -2,6 +2,7 @@ import { assert, describe, it } from 'vitest';
 import { effect_root } from '../../src/internal/client/reactivity/effects';
 import { push, pop } from '../../src/internal/client/context';
 import { proxy, remove_onchange } from '../../src/internal/client/proxy';
+import { PROXY_META_SYMBOL } from '../../src/internal/client/constants';
 
 function run(fn: () => void) {
 	push({}, true);
@@ -295,4 +296,103 @@ describe('proxy onchange kernel', () => {
 			assert.equal(count, 1);
 		});
 	});
+
+	it('lets the callback write state', () => {
+		run(() => {
+			const other = proxy({ n: 0 } as any);
+			const state = proxy({ a: 1 } as any, () => other.n++);
+
+			state.a = 2;
+			assert.equal(other.n, 1);
+		});
+	});
+
+	it('does not let a throwing callback break later unrelated writes', () => {
+		run(() => {
+			const a = proxy({ v: 0 } as any, () => {
+				throw new Error('boom');
+			});
+			assert.throws(() => (a.v = 1), /boom/);
+
+			const b = proxy({ v: 0 } as any);
+			b.v = 1;
+			assert.equal(b.v, 1);
+		});
+	});
+
+	it('fires once for an array method whose callback mutates another observed array', () => {
+		run(() => {
+			let count = 0;
+			const other = proxy([1] as any[]);
+			const arr = proxy([3, 1, 2] as any[], () => count++);
+
+			arr.sort((x, y) => {
+				other.push(0);
+				return x - y;
+			});
+
+			assert.equal(count, 1);
+		});
+	});
+
+	it('keeps links bounded under index churn', () => {
+		run(() => {
+			const arr = proxy([{ id: 0 }, { id: 1 }, { id: 2 }] as any[], () => {});
+			const last = arr[2];
+
+			for (let i = 3; i < 200; i += 1) {
+				arr.unshift({ id: i });
+			}
+
+			// every element was read through the array's get trap many times under shifting keys
+			for (let i = 0; i < arr.length; i += 1) arr[i];
+
+			assert.equal(last[PROXY_META_SYMBOL].links.length, 1);
+		});
+	});
+
+	it('fires for defineProperty', () => {
+		run(() => {
+			let count = 0;
+			const state = proxy({ a: 1 } as any, () => count++);
+
+			Object.defineProperty(state, 'b', {
+				value: 2,
+				enumerable: true,
+				configurable: true,
+				writable: true
+			});
+			assert.equal(count, 1);
+			assert.equal(state.b, 2);
+		});
+	});
+
+	it('stops walking a subtree once no root can fire', () => {
+		run(() => {
+			const cb = () => {};
+			const state = proxy({ a: { b: 1 } } as any, cb);
+			const child = state.a;
+			assert.equal(child[PROXY_META_SYMBOL].observed, true);
+
+			remove_onchange(state, cb);
+			child.b = 2;
+
+			assert.equal(child[PROXY_META_SYMBOL].observed, false);
+			assert.equal(child[PROXY_META_SYMBOL].links.length, 0);
+		});
+	});
+
+	it('does not loop when the callback mutates its own tree', () => {
+		run(() => {
+			let count = 0;
+			const state = proxy({ a: 1, log: 0 } as any, () => {
+				count++;
+				state.log++;
+			});
+
+			state.a = 2;
+			assert.equal(count, 1);
+			assert.equal(state.log, 1);
+		});
+	});
 });