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

Author: dummdidumm

.changeset/puny-masks-run.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: group sync statements

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -1074,6 +1074,9 @@ function calculate_blockers(instance, analysis) {
 
 	let awaited = false;
 
+	/** @type {Array<ESTree.Statement | ESTree.VariableDeclarator>} */
+	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<ESTree.FunctionDeclaration | ESTree.VariableDeclarator>}
@@ -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<Binding['blocker']>} */ (
 						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<Binding['blocker']>} */ (
 				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<Binding>} */
 		const reads_writes = new Set();

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];
+}

packages/svelte/src/compiler/phases/types.d.ts

@@ -131,7 +131,7 @@ export interface ComponentAnalysis extends Analysis {
 	instance_body: {
 		hoisted: Array<Statement | ModuleDeclaration>;
 		sync: Array<Statement | ModuleDeclaration | VariableDeclaration>;
-		async: Array<{ node: Statement | VariableDeclarator; has_await: boolean }>;
+		async: Array<{ nodes: Array<Statement | VariableDeclarator>; has_await: boolean }>;
 		declarations: Array<Identifier>;
 	};
 }

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();

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) {

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 } } });

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);
+}

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)));
+}

packages/svelte/tests/snapshot/samples/async-top-level-group-sync-run/index.svelte

@@ -0,0 +1,9 @@
+<script>
+	let a = await Promise.resolve(1);
+	// these should be grouped into one, having an async tick inbetween
+	// would change how the code runs and could introduce subtle timing bugs
+	let b = a + 1;
+	let c = b + 1;
+</script>
+
+{c}