Skip to content

Commit 7826eba

Browse files
Ocean-OSRich-Harris
Ocean-OS
and
Rich-Harris
authored
fix: rewrite destructuring logic to handle iterators (#15813)
* fix: wrap array destructuring in spread to avoid iterator edge case * spread at `tmp` declaration * completely rewrite destructuring handling * only wrap in iife if necessary * oops * minor style tweaks * separate visitors * tweak --------- Co-authored-by: Rich Harris <[email protected]>
1 parent 8fc8bc7 commit 7826eba

File tree

7 files changed

+141
-74
lines changed

7 files changed

+141
-74
lines changed

‎.changeset/serious-moles-yell.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
fix: rewrite destructuring logic to handle iterators

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

Lines changed: 37 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
/** @import { Binding } from '#compiler' */
33
/** @import { ComponentClientTransformState, ComponentContext } from '../types' */
44
import { dev } from '../../../../state.js';
5-
import { extract_paths } from '../../../../utils/ast.js';
5+
import { build_pattern, extract_paths } from '../../../../utils/ast.js';
66
import * as b from '#compiler/builders';
77
import * as assert from '../../../../utils/assert.js';
88
import { get_rune } from '../../../scope.js';
@@ -141,20 +141,20 @@ export function VariableDeclaration(node, context) {
141141
b.declarator(declarator.id, create_state_declarator(declarator.id, value))
142142
);
143143
} else {
144-
const tmp = context.state.scope.generate('tmp');
145-
const paths = extract_paths(declarator.id);
144+
const [pattern, replacements] = build_pattern(declarator.id, context.state.scope);
146145
declarations.push(
147-
b.declarator(b.id(tmp), value),
148-
...paths.map((path) => {
149-
const value = path.expression?.(b.id(tmp));
150-
const binding = context.state.scope.get(/** @type {Identifier} */ (path.node).name);
151-
return b.declarator(
152-
path.node,
153-
binding?.kind === 'state' || binding?.kind === 'raw_state'
154-
? create_state_declarator(binding.node, value)
155-
: value
156-
);
157-
})
146+
b.declarator(pattern, value),
147+
.../** @type {[Identifier, Identifier][]} */ ([...replacements]).map(
148+
([original, replacement]) => {
149+
const binding = context.state.scope.get(original.name);
150+
return b.declarator(
151+
original,
152+
binding?.kind === 'state' || binding?.kind === 'raw_state'
153+
? create_state_declarator(binding.node, replacement)
154+
: replacement
155+
);
156+
}
157+
)
158158
);
159159
}
160160

@@ -170,8 +170,7 @@ export function VariableDeclaration(node, context) {
170170
)
171171
);
172172
} else {
173-
const bindings = extract_paths(declarator.id);
174-
173+
const [pattern, replacements] = build_pattern(declarator.id, context.state.scope);
175174
const init = /** @type {CallExpression} */ (declarator.init);
176175

177176
/** @type {Identifier} */
@@ -189,10 +188,16 @@ export function VariableDeclaration(node, context) {
189188
);
190189
}
191190

192-
for (let i = 0; i < bindings.length; i++) {
193-
const binding = bindings[i];
191+
for (let i = 0; i < replacements.size; i++) {
192+
const [original, replacement] = [...replacements][i];
194193
declarations.push(
195-
b.declarator(binding.node, b.call('$.derived', b.thunk(binding.expression(rhs))))
194+
b.declarator(
195+
original,
196+
b.call(
197+
'$.derived',
198+
b.arrow([], b.block([b.let(pattern, rhs), b.return(replacement)]))
199+
)
200+
)
196201
);
197202
}
198203
}
@@ -304,19 +309,19 @@ function create_state_declarators(declarator, { scope, analysis }, value) {
304309
];
305310
}
306311

307-
const tmp = scope.generate('tmp');
308-
const paths = extract_paths(declarator.id);
312+
const [pattern, replacements] = build_pattern(declarator.id, scope);
309313
return [
310-
b.declarator(b.id(tmp), value),
311-
...paths.map((path) => {
312-
const value = path.expression?.(b.id(tmp));
313-
const binding = scope.get(/** @type {Identifier} */ (path.node).name);
314-
return b.declarator(
315-
path.node,
316-
binding?.kind === 'state'
317-
? b.call('$.mutable_source', value, analysis.immutable ? b.true : undefined)
318-
: value
319-
);
320-
})
314+
b.declarator(pattern, value),
315+
.../** @type {[Identifier, Identifier][]} */ ([...replacements]).map(
316+
([original, replacement]) => {
317+
const binding = scope.get(original.name);
318+
return b.declarator(
319+
original,
320+
binding?.kind === 'state'
321+
? b.call('$.mutable_source', replacement, analysis.immutable ? b.true : undefined)
322+
: replacement
323+
);
324+
}
325+
)
321326
];
322327
}

‎packages/svelte/src/compiler/phases/3-transform/server/visitors/VariableDeclaration.js

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
/** @import { Context } from '../types.js' */
44
/** @import { ComponentAnalysis } from '../../../types.js' */
55
/** @import { Scope } from '../../../scope.js' */
6-
import { build_fallback, extract_paths } from '../../../../utils/ast.js';
6+
import { build_pattern, build_fallback, extract_paths } from '../../../../utils/ast.js';
77
import * as b from '#compiler/builders';
88
import { get_rune } from '../../../scope.js';
99
import { walk } from 'zimmerframe';
@@ -188,13 +188,10 @@ function create_state_declarators(declarator, scope, value) {
188188
return [b.declarator(declarator.id, value)];
189189
}
190190

191-
const tmp = scope.generate('tmp');
192-
const paths = extract_paths(declarator.id);
191+
const [pattern, replacements] = build_pattern(declarator.id, scope);
193192
return [
194-
b.declarator(b.id(tmp), value), // TODO inject declarator for opts, so we can use it below
195-
...paths.map((path) => {
196-
const value = path.expression?.(b.id(tmp));
197-
return b.declarator(path.node, value);
198-
})
193+
b.declarator(pattern, value),
194+
// TODO inject declarator for opts, so we can use it below
195+
...[...replacements].map(([original, replacement]) => b.declarator(original, replacement))
199196
];
200197
}

‎packages/svelte/src/compiler/phases/3-transform/shared/assignments.js

Lines changed: 40 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
/** @import { AssignmentExpression, AssignmentOperator, Expression, Node, Pattern } from 'estree' */
1+
/** @import { AssignmentExpression, AssignmentOperator, Expression, Identifier, Node, Pattern } from 'estree' */
22
/** @import { Context as ClientContext } from '../client/types.js' */
33
/** @import { Context as ServerContext } from '../server/types.js' */
4-
import { extract_paths, is_expression_async } from '../../../utils/ast.js';
4+
import { build_pattern, is_expression_async } from '../../../utils/ast.js';
55
import * as b from '#compiler/builders';
66

77
/**
@@ -23,47 +23,60 @@ export function visit_assignment_expression(node, context, build_assignment) {
2323

2424
let changed = false;
2525

26-
const assignments = extract_paths(node.left).map((path) => {
27-
const value = path.expression?.(rhs);
26+
const [pattern, replacements] = build_pattern(node.left, context.state.scope);
2827

29-
let assignment = build_assignment('=', path.node, value, context);
30-
if (assignment !== null) changed = true;
31-
32-
return (
33-
assignment ??
34-
b.assignment(
35-
'=',
36-
/** @type {Pattern} */ (context.visit(path.node)),
37-
/** @type {Expression} */ (context.visit(value))
38-
)
39-
);
40-
});
28+
const assignments = [
29+
b.let(pattern, rhs),
30+
...[...replacements].map(([original, replacement]) => {
31+
let assignment = build_assignment(node.operator, original, replacement, context);
32+
if (assignment !== null) changed = true;
33+
return b.stmt(
34+
assignment ??
35+
b.assignment(
36+
node.operator,
37+
/** @type {Identifier} */ (context.visit(original)),
38+
/** @type {Expression} */ (context.visit(replacement))
39+
)
40+
);
41+
})
42+
];
4143

4244
if (!changed) {
4345
// No change to output -> nothing to transform -> we can keep the original assignment
4446
return null;
4547
}
4648

4749
const is_standalone = /** @type {Node} */ (context.path.at(-1)).type.endsWith('Statement');
48-
const sequence = b.sequence(assignments);
50+
const block = b.block(assignments);
4951

5052
if (!is_standalone) {
5153
// this is part of an expression, we need the sequence to end with the value
52-
sequence.expressions.push(rhs);
54+
block.body.push(b.return(rhs));
5355
}
5456

55-
if (should_cache) {
56-
// the right hand side is a complex expression, wrap in an IIFE to cache it
57-
const iife = b.arrow([rhs], sequence);
57+
if (is_standalone && !should_cache) {
58+
return block;
59+
}
5860

59-
const iife_is_async =
60-
is_expression_async(value) ||
61-
assignments.some((assignment) => is_expression_async(assignment));
61+
const iife = b.arrow(should_cache ? [rhs] : [], block);
6262

63-
return iife_is_async ? b.await(b.call(b.async(iife), value)) : b.call(iife, value);
64-
}
63+
const iife_is_async =
64+
is_expression_async(value) ||
65+
assignments.some(
66+
(assignment) =>
67+
(assignment.type === 'ExpressionStatement' &&
68+
is_expression_async(assignment.expression)) ||
69+
(assignment.type === 'VariableDeclaration' &&
70+
assignment.declarations.some(
71+
(declaration) =>
72+
is_expression_async(declaration.id) ||
73+
(declaration.init && is_expression_async(declaration.init))
74+
))
75+
);
6576

66-
return sequence;
77+
return iife_is_async
78+
? b.await(b.call(b.async(iife), should_cache ? value : undefined))
79+
: b.call(iife, should_cache ? value : undefined);
6780
}
6881

6982
if (node.left.type !== 'Identifier' && node.left.type !== 'MemberExpression') {

‎packages/svelte/src/compiler/phases/scope.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -630,9 +630,10 @@ export class Scope {
630630

631631
/**
632632
* @param {string} preferred_name
633+
* @param {(name: string, counter: number) => string} [generator]
633634
* @returns {string}
634635
*/
635-
generate(preferred_name) {
636+
generate(preferred_name, generator = (name, counter) => `${name}_${counter}`) {
636637
if (this.#porous) {
637638
return /** @type {Scope} */ (this.parent).generate(preferred_name);
638639
}
@@ -647,7 +648,7 @@ export class Scope {
647648
this.root.conflicts.has(name) ||
648649
is_reserved(name)
649650
) {
650-
name = `${preferred_name}_${n++}`;
651+
name = generator(preferred_name, n++);
651652
}
652653

653654
this.references.set(name, []);

‎packages/svelte/src/compiler/utils/ast.js

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
/** @import { AST } from '#compiler' */
1+
/** @import { AST, Scope } from '#compiler' */
22
/** @import * as ESTree from 'estree' */
33
import { walk } from 'zimmerframe';
44
import * as b from '#compiler/builders';
5+
import is_reference from 'is-reference';
56

67
/**
78
* Gets the left-most identifier of a member expression or identifier.
@@ -128,6 +129,49 @@ export function unwrap_pattern(pattern, nodes = []) {
128129
return nodes;
129130
}
130131

132+
/**
133+
* @param {ESTree.Pattern} id
134+
* @param {Scope} scope
135+
* @returns {[ESTree.Pattern, Map<ESTree.Identifier | ESTree.MemberExpression, ESTree.Identifier>]}
136+
*/
137+
export function build_pattern(id, scope) {
138+
/** @type {Map<ESTree.Identifier | ESTree.MemberExpression, ESTree.Identifier>} */
139+
const map = new Map();
140+
141+
/** @type {Map<string, string>} */
142+
const names = new Map();
143+
144+
let counter = 0;
145+
146+
for (const node of unwrap_pattern(id)) {
147+
const name = scope.generate(`$$${++counter}`, (_, counter) => `$$${counter}`);
148+
149+
map.set(node, b.id(name));
150+
151+
if (node.type === 'Identifier') {
152+
names.set(node.name, name);
153+
}
154+
}
155+
156+
const pattern = walk(id, null, {
157+
Identifier(node, context) {
158+
if (is_reference(node, /** @type {ESTree.Pattern} */ (context.path.at(-1)))) {
159+
const name = names.get(node.name);
160+
if (name) return b.id(name);
161+
}
162+
},
163+
164+
MemberExpression(node, context) {
165+
const n = map.get(node);
166+
if (n) return n;
167+
168+
context.next();
169+
}
170+
});
171+
172+
return [pattern, map];
173+
}
174+
131175
/**
132176
* Extracts all identifiers from a pattern.
133177
* @param {ESTree.Pattern} pattern

‎packages/svelte/tests/snapshot/samples/destructured-assignments/_expected/client/index.svelte.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@ let c = 3;
77
let d = 4;
88

99
export function update(array) {
10-
(
11-
$.set(a, array[0], true),
12-
$.set(b, array[1], true)
13-
);
10+
{
11+
let [$$1, $$2] = array;
12+
13+
$.set(a, $$1, true);
14+
$.set(b, $$2, true);
15+
};
1416

1517
[c, d] = array;
1618
}

0 commit comments

Comments
 (0)