fix: memoize repeated state reads in guard expressions

.changeset/wicked-keys-search.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: memoize repeated state reads in guard expressions

packages/svelte/src/compiler/phases/3-transform/client/types.d.ts

@@ -37,6 +37,10 @@ export interface ClientTransformState extends TransformState {
 			update?: (node: UpdateExpression) => Expression;
 		}
 	>;
+
+	readonly guard_snapshots?: Map<string, { readonly id: Identifier }>;
+
+	readonly collect_guard_snapshots?: Map<string, { readonly id: Identifier }>;
 }
 
 export interface ComponentClientTransformState extends ClientTransformState {

packages/svelte/src/compiler/phases/3-transform/client/visitors/ConstTag.js

@@ -26,7 +26,14 @@ export function ConstTag(node, context) {
 
 		context.state.consts.push(b.const(declaration.id, expression));
 
-		context.state.transform[declaration.id.name] = { read: get_value };
+		const snapshot = context.state.guard_snapshots?.get(declaration.id.name);
+		context.state.transform[declaration.id.name] = snapshot
+			? {
+					read() {
+						return snapshot.id;
+					}
+				}
+			: { read: get_value };
 
 		// we need to eagerly evaluate the expression in order to hit any
 		// 'Cannot access x before initialization' errors
@@ -78,9 +85,16 @@ export function ConstTag(node, context) {
 		}
 
 		for (const node of identifiers) {
-			context.state.transform[node.name] = {
-				read: (node) => b.member(b.call('$.get', tmp), node)
-			};
+			const snapshot = context.state.guard_snapshots?.get(node.name);
+			context.state.transform[node.name] = snapshot
+				? {
+						read() {
+							return snapshot.id;
+						}
+					}
+				: {
+						read: (node) => b.member(b.call('$.get', tmp), node)
+					};
 		}
 	}
 }

packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js

@@ -12,23 +12,64 @@ export function IfBlock(node, context) {
 	context.state.template.push_comment();
 	const statements = [];
 
-	const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));
+	const { has_await } = node.metadata.expression;
+	const guard_snapshots = new Map();
+	const expression = build_expression(context, node.test, node.metadata.expression, {
+		...context.state,
+		collect_guard_snapshots: guard_snapshots
+	});
+	const test = has_await ? b.call('$.get', b.id('$$condition')) : expression;
+
+	let branch_state = context.state;
+
+	if (guard_snapshots.size > 0) {
+		const snapshots = new Map(context.state.guard_snapshots ?? undefined);
+		const transform = { ...context.state.transform };
+
+		for (const [name, snapshot] of guard_snapshots) {
+			snapshots.set(name, snapshot);
+
+			const base = transform[name] ?? context.state.transform[name];
+			transform[name] = {
+				...base,
+				read() {
+					return snapshot.id;
+				},
+				assign: base?.assign,
+				mutate: base?.mutate,
+				update: base?.update
+			};
+		}
+
+		branch_state = {
+			...context.state,
+			collect_guard_snapshots: undefined,
+			guard_snapshots: snapshots,
+			transform
+		};
+	}
+
+	const consequent = /** @type {BlockStatement} */ (
+		branch_state === context.state
+			? context.visit(node.consequent)
+			: context.visit(node.consequent, branch_state)
+	);
 	const consequent_id = b.id(context.state.scope.generate('consequent'));
 
 	statements.push(b.var(consequent_id, b.arrow([b.id('$$anchor')], consequent)));
 
 	let alternate_id;
 
 	if (node.alternate) {
-		const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
+		const alternate = /** @type {BlockStatement} */ (
+			branch_state === context.state
+				? context.visit(node.alternate)
+				: context.visit(node.alternate, branch_state)
+		);
 		alternate_id = b.id(context.state.scope.generate('alternate'));
 		statements.push(b.var(alternate_id, b.arrow([b.id('$$anchor')], alternate)));
 	}
 
-	const { has_await } = node.metadata.expression;
-	const expression = build_expression(context, node.test, node.metadata.expression);
-	const test = has_await ? b.call('$.get', b.id('$$condition')) : expression;
-
 	/** @type {Expression[]} */
 	const args = [
 		context.state.node,

packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js

@@ -1,5 +1,5 @@
 /** @import { AssignmentExpression, Expression, Identifier, MemberExpression, SequenceExpression, Literal, Super, UpdateExpression, ExpressionStatement } from 'estree' */
-/** @import { AST, ExpressionMetadata } from '#compiler' */
+/** @import { AST, Binding, ExpressionMetadata } from '#compiler' */
 /** @import { ComponentClientTransformState, ComponentContext, Context } from '../../types' */
 import { walk } from 'zimmerframe';
 import { object } from '../../../../../utils/ast.js';
@@ -10,6 +10,59 @@ import is_reference from 'is-reference';
 import { dev, is_ignored, locator, component_name } from '../../../../../state.js';
 import { build_getter } from '../../utils.js';
 
+/**
+ * @param {import('estree').Node | null | undefined} node
+ * @returns {node is import('estree').Function}
+ */
+function is_function(node) {
+	return Boolean(
+		node &&
+			(node.type === 'ArrowFunctionExpression' ||
+				node.type === 'FunctionExpression' ||
+				node.type === 'FunctionDeclaration')
+	);
+}
+
+/**
+ * Determines whether repeated reads of a binding within a single expression
+ * should be memoized to avoid inconsistent results.
+ * @param {Binding | null} binding
+ * @returns {binding is Binding}
+ */
+function should_memoize_binding(binding) {
+	if (binding === null) return false;
+
+	switch (binding.kind) {
+		case 'state':
+		case 'raw_state':
+		case 'derived':
+		case 'legacy_reactive':
+		case 'template':
+			return true;
+		default:
+			return false;
+	}
+}
+
+/**
+ * @param {Binding | null} binding
+ * @returns {boolean}
+ */
+function should_snapshot_guard(binding) {
+	if (binding === null) return false;
+
+	switch (binding.kind) {
+		case 'derived':
+		case 'legacy_reactive':
+		case 'template':
+			return true;
+		default:
+			return false;
+	}
+}
+/** @type {WeakMap<any, Map<string, { id: Identifier; getter: (node: Identifier) => Expression }>>} */
+const memoized_reads_by_scope = new WeakMap();
+
 /**
  * A utility for extracting complex expressions (such as call expressions)
  * from templates and replacing them with `$0`, `$1` etc
@@ -391,7 +444,135 @@ export function validate_mutation(node, context, expression) {
  * @param {ExpressionMetadata} metadata
  */
 export function build_expression(context, expression, metadata, state = context.state) {
-	const value = /** @type {Expression} */ (context.visit(expression, state));
+	/** @type {import('../../types.js').ComponentClientTransformState} */
+	let child_state = state;
+	/** @type {import('estree').Statement[]} */
+	const assignments = [];
+	/** @type {Set<string> | null} */
+	let memoized_ids = null;
+
+	if (state.analysis.runes && !metadata.has_await) {
+		const component_state = /** @type {ComponentClientTransformState} */ (context.state);
+		/** @type {Map<Binding, number>} */
+		const counts = new Map();
+
+		walk(expression, null, {
+			Identifier(node, { path }) {
+				const parent = /** @type {Expression} */ (path.at(-1));
+				if (!is_reference(node, parent)) return;
+
+				// avoid memoizing reads that occur within nested functions, since those
+				// must re-evaluate when the function executes later
+				if (path.some((ancestor, i) => i < path.length - 1 && is_function(ancestor))) {
+					return;
+				}
+
+				const binding = state.scope.get(node.name);
+				if (!should_memoize_binding(binding)) return;
+
+				counts.set(binding, (counts.get(binding) ?? 0) + 1);
+			}
+		});
+
+		memoized_ids = new Set();
+
+		const guard_snapshots = state.collect_guard_snapshots;
+
+		for (const [binding, count] of counts) {
+			const name = binding.node?.name;
+			if (!name) continue;
+
+			const original = state.transform[name];
+			if (!original?.read) continue;
+
+			const capture_for_guard = Boolean(guard_snapshots && should_snapshot_guard(binding));
+			if (count <= 1 && !capture_for_guard) continue;
+
+			let scope_records = memoized_reads_by_scope.get(binding.scope);
+			if (!scope_records) {
+				scope_records = new Map();
+				memoized_reads_by_scope.set(binding.scope, scope_records);
+			}
+
+			if (child_state === state) {
+				child_state = {
+					...state,
+					transform: { ...state.transform }
+				};
+			}
+
+			let record = scope_records.get(name);
+
+			if (!record) {
+				const memo_id = b.id(state.scope.generate(`${name}_value`));
+				record = {
+					id: memo_id,
+					getter: original.read
+				};
+				scope_records.set(name, record);
+				component_state.init.push(b.let(memo_id));
+			}
+			memoized_ids.add(record.id.name);
+
+			const previous = child_state.transform[name];
+			child_state.transform[name] = {
+				...previous,
+				read() {
+					return record.id;
+				},
+				assign: previous?.assign,
+				mutate: previous?.mutate,
+				update: previous?.update
+			};
+
+			assignments.push(b.stmt(b.assignment('=', record.id, record.getter(b.id(name)))));
+
+			if (guard_snapshots && capture_for_guard) {
+				guard_snapshots.set(name, { id: record.id });
+			}
+		}
+	}
+
+	let value = /** @type {Expression} */ (context.visit(expression, child_state));
+
+	const optional_sources = new Set();
+
+	if (memoized_ids !== null) {
+		for (const name of memoized_ids) optional_sources.add(name);
+	}
+
+	if (state.guard_snapshots) {
+		for (const snapshot of state.guard_snapshots.values()) {
+			optional_sources.add(snapshot.id.name);
+		}
+	}
+
+	if (optional_sources.size > 0) {
+		walk(value, null, {
+			MemberExpression(node) {
+				let root = node.object;
+				while (root && root.type === 'MemberExpression') {
+					root = root.object;
+				}
+
+				if (root?.type === 'Identifier' && optional_sources.has(root.name)) {
+					/** @type {import('estree').MemberExpression | null} */
+					let current = node;
+					while (current) {
+						current.optional = true;
+						const next = /** @type {import('estree').Expression | import('estree').Super} */ (
+							current.object
+						);
+						current = next.type === 'MemberExpression' ? next : null;
+					}
+				}
+			}
+		});
+	}
+
+	if (assignments.length > 0) {
+		value = b.call(b.arrow([], b.block([...assignments, b.return(value)])));
+	}
 
 	// Components not explicitly in legacy mode might be expected to be in runes mode (especially since we didn't
 	// adjust this behavior until recently, which broke people's existing components), so we also bail in this case.

packages/svelte/tests/runtime-runes/samples/guard-derived-await/_config.js

@@ -0,0 +1,17 @@
+import { test } from '../../test';
+import { flushSync, tick } from 'svelte';
+
+export default test({
+	mode: ['client'],
+	async test({ target }) {
+		const button = target.querySelector('button');
+
+		flushSync(() => button?.click());
+		await tick();
+
+		const text = target.textContent?.trim() ?? '';
+		if (!text.endsWith('trigger')) {
+			throw new Error(`unexpected text: ${text}`);
+		}
+	}
+});

packages/svelte/tests/runtime-runes/samples/guard-derived-await/main.svelte

@@ -0,0 +1,24 @@
+<script>
+	let value = $state({ nested: { depth: 2 } });
+	let trigger = $state(false);
+
+	$effect(() => {
+		if (trigger) value = null;
+	});
+
+	function load() {
+		return Promise.resolve(value);
+	}
+</script>
+
+{#await load() then result}
+	{#if result?.nested?.depth > 1}
+		ok
+	{:else}
+		low
+	{/if}
+{:catch}
+	error
+{/await}
+
+<button onclick={() => (trigger = true)}>trigger</button>

packages/svelte/tests/runtime-runes/samples/guard-derived-const/_config.js

@@ -0,0 +1,16 @@
+import { test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	mode: ['client'],
+	async test({ target }) {
+		const button = target.querySelector('button');
+
+		flushSync(() => button?.click());
+
+		const text = target.textContent?.trim() ?? '';
+		if (!text.endsWith('toggle')) {
+			throw new Error(`unexpected text: ${text}`);
+		}
+	}
+});