From 6b33dd2a1e8aa48dc88c9ce6e19c4a49a2eac51a Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Sat, 21 Mar 2026 13:29:39 +0100 Subject: [PATCH] fix: group sync statements (#17977) We were just putting each statement into its own promise. Besides this being bad for perf, it also introduces subtle timing issues - the execution order of the code could change in bad ways. Fixes #17940 --- .changeset/puny-masks-run.md | 5 + .../src/compiler/phases/2-analyze/index.js | 36 ++++- .../3-transform/shared/transform-async.js | 124 ++++++++++-------- .../svelte/src/compiler/phases/types.d.ts | 2 +- .../_expected/client/index.svelte.js | 16 ++- .../_expected/server/index.svelte.js | 16 ++- .../async-top-level-group-sync-run/_config.js | 3 + .../_expected/client/index.svelte.js | 26 ++++ .../_expected/server/index.svelte.js | 21 +++ .../index.svelte | 9 ++ 10 files changed, 184 insertions(+), 74 deletions(-) create mode 100644 .changeset/puny-masks-run.md create mode 100644 packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_config.js create mode 100644 packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/client/index.svelte.js create mode 100644 packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/server/index.svelte.js create mode 100644 packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte diff --git a/.changeset/puny-masks-run.md b/.changeset/puny-masks-run.md new file mode 100644 index 0000000000..162abe4ae7 --- /dev/null +++ b/.changeset/puny-masks-run.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: group sync statements diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index cadd159b3e..dec0081aa9 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -1074,6 +1074,9 @@ function calculate_blockers(instance, analysis) { let awaited = false; + /** @type {Array} */ + let sync_group = []; + // TODO this should probably be attached to the scope? const promises = b.id('$$promises'); @@ -1088,6 +1091,13 @@ function calculate_blockers(instance, analysis) { binding.blocker = blocker; } + function flush_sync_group() { + if (sync_group.length === 0) return; + + analysis.instance_body.async.push({ nodes: sync_group, has_await: false }); + sync_group = []; + } + /** * Analysis of blockers for functions is deferred until we know which statements are async/blockers * @type {Array} @@ -1149,6 +1159,9 @@ function calculate_blockers(instance, analysis) { trace_references(declarator, reads, writes, instance.scope); + // Needs to happen before blocker computation + if (has_await) flush_sync_group(); + const blocker = /** @type {NonNullable} */ ( b.member(promises, b.literal(analysis.instance_body.async.length), true) ); @@ -1161,11 +1174,12 @@ function calculate_blockers(instance, analysis) { push_declaration(id, blocker); } - // one declarator per declaration, makes things simpler - analysis.instance_body.async.push({ - node: declarator, - has_await - }); + if (has_await) { + // one declarator per declaration, makes things simpler + analysis.instance_body.async.push({ nodes: [declarator], has_await: true }); + } else { + sync_group.push(declarator); + } } } } else if (awaited) { @@ -1177,6 +1191,9 @@ function calculate_blockers(instance, analysis) { trace_references(node, reads, writes, instance.scope); + // Needs to happen before blocker computation + if (has_await) flush_sync_group(); + const blocker = /** @type {NonNullable} */ ( b.member(promises, b.literal(analysis.instance_body.async.length), true) ); @@ -1187,15 +1204,20 @@ function calculate_blockers(instance, analysis) { if (node.type === 'ClassDeclaration') { push_declaration(node.id, blocker); - analysis.instance_body.async.push({ node, has_await }); + } + + if (has_await) { + analysis.instance_body.async.push({ nodes: [node], has_await: true }); } else { - analysis.instance_body.async.push({ node, has_await }); + sync_group.push(node); } } else { analysis.instance_body.sync.push(node); } } + flush_sync_group(); + for (const fn of functions) { /** @type {Set} */ const reads_writes = new Set(); diff --git a/packages/svelte/src/compiler/phases/3-transform/shared/transform-async.js b/packages/svelte/src/compiler/phases/3-transform/shared/transform-async.js index 5c8f901d7c..ac90f1db4b 100644 --- a/packages/svelte/src/compiler/phases/3-transform/shared/transform-async.js +++ b/packages/svelte/src/compiler/phases/3-transform/shared/transform-async.js @@ -47,64 +47,24 @@ export function transform_body(instance_body, runner, transform) { // Thunks for the await expressions if (instance_body.async.length > 0) { - const thunks = instance_body.async.map((s) => { - if (s.node.type === 'VariableDeclarator') { - const visited = /** @type {ESTree.VariableDeclaration | ESTree.EmptyStatement} */ ( - transform(b.var(s.node.id, s.node.init)) - ); - - const statements = - visited.type === 'VariableDeclaration' - ? visited.declarations.map((node) => { - if ( - node.id.type === 'Identifier' && - (node.id.name.startsWith('$$d') || node.id.name.startsWith('$$array')) - ) { - // this is an intermediate declaration created in VariableDeclaration.js; - // subsequent statements depend on it - return b.var(node.id, node.init); - } - - return b.stmt(b.assignment('=', node.id, node.init ?? b.void0)); - }) - : []; - - if (statements.length === 1) { - const statement = /** @type {ESTree.ExpressionStatement} */ (statements[0]); - return b.thunk(statement.expression, s.has_await); - } - - return b.thunk(b.block(statements), s.has_await); - } + const thunks = instance_body.async.map((entry) => { + /** @type {ESTree.Statement[]} */ + const entry_statements = []; - if (s.node.type === 'ClassDeclaration') { - return b.thunk( - b.assignment( - '=', - s.node.id, - /** @type {ESTree.ClassExpression} */ ({ ...s.node, type: 'ClassExpression' }) - ), - s.has_await - ); + for (const node of entry.nodes) { + entry_statements.push(...transform_async_node(node, transform)); } - if (s.node.type === 'ExpressionStatement') { - // the expression may be a $inspect call, which will be transformed into an empty statement - const expression = /** @type {ESTree.Expression | ESTree.EmptyStatement} */ ( - transform(s.node.expression) - ); - - if (expression.type === 'EmptyStatement') { - // Keep indices stable for async sequencing while avoiding array holes in run([...]). - return b.thunk(b.void0, false); - } + if (entry_statements.length === 0) { + // Keep indices stable for async sequencing while avoiding array holes in run([...]). + return b.thunk(b.void0, false); + } - return expression.type === 'AwaitExpression' - ? b.thunk(expression, true) - : b.thunk(b.unary('void', expression), s.has_await); + if (entry_statements.length === 1 && entry_statements[0].type === 'ExpressionStatement') { + return b.thunk(entry_statements[0].expression, entry.has_await); } - return b.thunk(b.block([/** @type {ESTree.Statement} */ (transform(s.node))]), s.has_await); + return b.thunk(b.block(entry_statements), entry.has_await); }); // TODO get the `$$promises` ID from scope @@ -113,3 +73,63 @@ export function transform_body(instance_body, runner, transform) { return statements; } + +/** + * @param {ESTree.Statement | ESTree.VariableDeclarator} node + * @param {(node: ESTree.Node) => ESTree.Node} transform + * @returns {ESTree.Statement[]} + */ +function transform_async_node(node, transform) { + if (node.type === 'VariableDeclarator') { + const visited = /** @type {ESTree.VariableDeclaration | ESTree.EmptyStatement} */ ( + transform(b.var(node.id, node.init)) + ); + + return visited.type === 'VariableDeclaration' + ? visited.declarations.map((node) => { + if ( + node.id.type === 'Identifier' && + (node.id.name.startsWith('$$d') || node.id.name.startsWith('$$array')) + ) { + // This intermediate declaration is created in VariableDeclaration.js; + // subsequent statements may depend on it. + return b.var(node.id, node.init); + } + + return b.stmt(b.assignment('=', node.id, node.init ?? b.void0)); + }) + : []; + } + + if (node.type === 'ClassDeclaration') { + return [ + b.stmt( + b.assignment( + '=', + node.id, + /** @type {ESTree.ClassExpression} */ ({ ...node, type: 'ClassExpression' }) + ) + ) + ]; + } + + if (node.type === 'ExpressionStatement') { + // The expression may be a $inspect call, which will be transformed into an empty statement. + const expression = /** @type {ESTree.Expression | ESTree.EmptyStatement} */ ( + transform(node.expression) + ); + + if (expression.type === 'EmptyStatement') { + return []; + } + + if (expression.type === 'AwaitExpression') { + return [b.stmt(expression)]; + } + + return [b.stmt(b.unary('void', expression))]; + } + + const statement = /** @type {ESTree.Statement | ESTree.EmptyStatement} */ (transform(node)); + return statement.type === 'EmptyStatement' ? [] : [statement]; +} diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index 5397ea45f9..a1a85ce145 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -131,7 +131,7 @@ export interface ComponentAnalysis extends Analysis { instance_body: { hoisted: Array; sync: Array; - async: Array<{ node: Statement | VariableDeclarator; has_await: boolean }>; + async: Array<{ nodes: Array; has_await: boolean }>; declarations: Array; }; } diff --git a/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/client/index.svelte.js index a8b1a43495..4f06d9ddbf 100644 --- a/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/client/index.svelte.js @@ -10,13 +10,15 @@ export default function Async_in_derived($$anchor, $$props) { var $$promises = $.run([ async () => yes1 = await $.async_derived(() => 1), async () => yes2 = await $.async_derived(async () => foo(await 1)), - () => no1 = $.derived(async () => { - return await 1; - }), - - () => no2 = $.derived(() => async () => { - return await 1; - }) + () => { + no1 = $.derived(async () => { + return await 1; + }); + + no2 = $.derived(() => async () => { + return await 1; + }); + } ]); var fragment = $.comment(); diff --git a/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/server/index.svelte.js b/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/server/index.svelte.js index 1697e3adc6..3a53475944 100644 --- a/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/server/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/async-in-derived/_expected/server/index.svelte.js @@ -8,13 +8,15 @@ export default function Async_in_derived($$renderer, $$props) { var $$promises = $$renderer.run([ async () => yes1 = await $.async_derived(() => 1), async () => yes2 = await $.async_derived(async () => foo(await 1)), - () => no1 = $.derived(async () => { - return await 1; - }), - - () => no2 = $.derived(() => async () => { - return await 1; - }) + () => { + no1 = $.derived(async () => { + return await 1; + }); + + no2 = $.derived(() => async () => { + return await 1; + }); + } ]); if (true) { diff --git a/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_config.js b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_config.js new file mode 100644 index 0000000000..2e30bbeb16 --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_config.js @@ -0,0 +1,3 @@ +import { test } from '../../test'; + +export default test({ compileOptions: { experimental: { async: true } } }); diff --git a/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/client/index.svelte.js new file mode 100644 index 0000000000..8fb09fadd2 --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/client/index.svelte.js @@ -0,0 +1,26 @@ +import 'svelte/internal/disclose-version'; +import 'svelte/internal/flags/async'; +import * as $ from 'svelte/internal/client'; + +export default function Async_top_level_group_sync_run($$anchor) { + var a, + // these should be grouped into one, having an async tick inbetween + // would change how the code runs and could introduce subtle timing bugs + b, + c; + + var $$promises = $.run([ + async () => a = await Promise.resolve(1), + () => { + b = a + 1; + c = b + 1; + } + ]); + + $.next(); + + var text = $.text(); + + $.template_effect(() => $.set_text(text, c), void 0, void 0, [$$promises[1]]); + $.append($$anchor, text); +} \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/server/index.svelte.js b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/server/index.svelte.js new file mode 100644 index 0000000000..43db129746 --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/_expected/server/index.svelte.js @@ -0,0 +1,21 @@ +import 'svelte/internal/flags/async'; +import * as $ from 'svelte/internal/server'; + +export default function Async_top_level_group_sync_run($$renderer) { + var a, + // these should be grouped into one, having an async tick inbetween + // would change how the code runs and could introduce subtle timing bugs + b, + c; + + var $$promises = $$renderer.run([ + async () => a = await Promise.resolve(1), + () => { + b = a + 1; + c = b + 1; + } + ]); + + $$renderer.push(``); + $$renderer.async([$$promises[1]], ($$renderer) => $$renderer.push(() => $.escape(c))); +} \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte new file mode 100644 index 0000000000..98f602312e --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte @@ -0,0 +1,9 @@ + + +{c}