diff --git a/.changeset/wicked-keys-search.md b/.changeset/wicked-keys-search.md new file mode 100644 index 000000000000..e1eecbbe4015 --- /dev/null +++ b/.changeset/wicked-keys-search.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: memoize repeated state reads in guard expressions diff --git a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts index 932d35367162..b65d973597eb 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts +++ b/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; + + readonly collect_guard_snapshots?: Map; } export interface ComponentClientTransformState extends ClientTransformState { diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ConstTag.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/ConstTag.js index 81dd9e07edd5..712b46f572e8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ConstTag.js +++ b/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) + }; } } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js index 3dd36b180bee..53242b2665e7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js @@ -12,7 +12,48 @@ 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))); @@ -20,15 +61,15 @@ export function IfBlock(node, context) { 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, diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index ba140a153e91..dee457be0b09 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/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 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 | null} */ + let memoized_ids = null; + + if (state.analysis.runes && !metadata.has_await) { + const component_state = /** @type {ComponentClientTransformState} */ (context.state); + /** @type {Map} */ + 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. diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-await/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-await/_config.js new file mode 100644 index 000000000000..9a84a3a37770 --- /dev/null +++ b/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}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-await/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-await/main.svelte new file mode 100644 index 000000000000..78c4dad78c88 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-await/main.svelte @@ -0,0 +1,24 @@ + + +{#await load() then result} + {#if result?.nested?.depth > 1} + ok + {:else} + low + {/if} +{:catch} + error +{/await} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-const/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-const/_config.js new file mode 100644 index 000000000000..bf75e8938dd0 --- /dev/null +++ b/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}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-const/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-const/main.svelte new file mode 100644 index 000000000000..5633f07bd0c7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-const/main.svelte @@ -0,0 +1,23 @@ + + +{#if first?.nested?.value > 1} + big +{:else} + small +{/if} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/_config.js new file mode 100644 index 000000000000..82d22a474b54 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/_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('advance')) { + throw new Error(`unexpected text: ${text}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/main.svelte new file mode 100644 index 000000000000..38268e49038a --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-derived/main.svelte @@ -0,0 +1,24 @@ + + +{#if nested?.depth > 2} + deep +{:else} + shallow +{/if} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-each/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-each/_config.js new file mode 100644 index 000000000000..f63e547216ed --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-each/_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('clear')) { + throw new Error(`unexpected text: ${text}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-each/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-each/main.svelte new file mode 100644 index 000000000000..d86d99ff7815 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-each/main.svelte @@ -0,0 +1,21 @@ + + +{#each items as item (item.nested)} + {#if item?.nested?.ok} + ok + {:else} + fail + {/if} +{/each} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/_config.js new file mode 100644 index 000000000000..a2fc1009615e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/_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('switch')) { + throw new Error(`unexpected text: ${text}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/main.svelte new file mode 100644 index 000000000000..55131f13a399 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-elseif/main.svelte @@ -0,0 +1,25 @@ + + +{#if centerRow?.nested?.optional > 2} + A +{:else if centerRow?.nested?.optional > 1} + B +{:else} + C +{/if} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-key/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-key/_config.js new file mode 100644 index 000000000000..bf75e8938dd0 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-key/_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}`); + } + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-key/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-key/main.svelte new file mode 100644 index 000000000000..93cd89c56455 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-key/main.svelte @@ -0,0 +1,18 @@ + + +{#key item?.nested?.value} + {#if item?.nested?.value > 0} + positive + {:else} + zero + {/if} +{/key} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/_config.js new file mode 100644 index 000000000000..881c1545ee9f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/_config.js @@ -0,0 +1,13 @@ +import { test } from '../../test'; +import { flushSync } from 'svelte'; + +export default test({ + mode: ['client'], + async test({ target, assert }) { + const button = target.querySelector('button'); + + flushSync(() => button?.click()); + + assert.equal(target.textContent?.trim(), 'Trigger'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/main.svelte new file mode 100644 index 000000000000..c2955cb34d30 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-nested/main.svelte @@ -0,0 +1,26 @@ + + +{#each virtualItems as row (row.index)} + {@const centerRow = centerRows[row.index]} + {#if centerRow?.nested != undefined} + {#if centerRow?.nested?.optional != undefined && centerRow.nested.optional > 0} + op: {centerRow.nested.optional}
+ {:else} + req: {centerRow.nested.required}
+ {/if} + {/if} +{/each} + + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/_config.js new file mode 100644 index 000000000000..881c1545ee9f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/_config.js @@ -0,0 +1,13 @@ +import { test } from '../../test'; +import { flushSync } from 'svelte'; + +export default test({ + mode: ['client'], + async test({ target, assert }) { + const button = target.querySelector('button'); + + flushSync(() => button?.click()); + + assert.equal(target.textContent?.trim(), 'Trigger'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/main.svelte new file mode 100644 index 000000000000..34442120c290 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-derived-short-circuit/main.svelte @@ -0,0 +1,20 @@ + + +{#each virtualItems as row (row.index)} + {@const centerRow = centerRows[row.index]} + {#if centerRow != undefined && centerRow.depth != undefined && typeof centerRow === "object" && "depth" in centerRow && typeof centerRow.depth === "number"} + {#if centerRow.depth != undefined && centerRow.depth > 0} + Hello World
+ {/if} + {/if} +{/each} + + \ No newline at end of file