Skip to content

Commit 2ddb117

Browse files
Ocean-OSOcean-OS
committed
completely rewrite destructuring handling
1 parent 608a17f commit 2ddb117

File tree

7 files changed

+133
-150
lines changed

7 files changed

+133
-150
lines changed

‎.changeset/serious-moles-yell.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'svelte': patch
33
---
44

5-
fix: wrap array destructuring in spread to avoid iterator edge case
5+
fix: rewrite destructuring logic to handle iterators

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

Lines changed: 37 additions & 40 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, is_array } 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, is_array(value, context.state.scope));
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,11 +170,7 @@ export function VariableDeclaration(node, context) {
170170
)
171171
);
172172
} else {
173-
const bindings = extract_paths(
174-
declarator.id,
175-
rune === '$derived' ? is_array(value, context.state.scope) : false
176-
);
177-
173+
const [pattern, replacements] = build_pattern(declarator.id, context.state.scope);
178174
const init = /** @type {CallExpression} */ (declarator.init);
179175

180176
/** @type {Identifier} */
@@ -192,10 +188,16 @@ export function VariableDeclaration(node, context) {
192188
);
193189
}
194190

195-
for (let i = 0; i < bindings.length; i++) {
196-
const binding = bindings[i];
191+
for (let i = 0; i < replacements.size; i++) {
192+
const [original, replacement] = [...replacements][i];
197193
declarations.push(
198-
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+
)
199201
);
200202
}
201203
}
@@ -307,24 +309,19 @@ function create_state_declarators(declarator, { scope, analysis }, value) {
307309
];
308310
}
309311

310-
const tmp = scope.generate('tmp');
311-
const paths = extract_paths(declarator.id, true);
312+
const [pattern, replacements] = build_pattern(declarator.id, scope);
312313
return [
313-
b.declarator(
314-
b.id(tmp),
315-
declarator.id.type === 'ArrayPattern' && !is_array(value, scope)
316-
? b.array([b.spread(value)])
317-
: value
318-
),
319-
...paths.map((path) => {
320-
const value = path.expression?.(b.id(tmp));
321-
const binding = scope.get(/** @type {Identifier} */ (path.node).name);
322-
return b.declarator(
323-
path.node,
324-
binding?.kind === 'state'
325-
? b.call('$.mutable_source', value, analysis.immutable ? b.true : undefined)
326-
: value
327-
);
328-
})
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+
)
329326
];
330327
}

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

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
/** @import { Binding } from '#compiler' */
33
/** @import { Context } from '../types.js' */
44
/** @import { Scope } from '../../../scope.js' */
5-
import { build_fallback, extract_paths, is_array } from '../../../../utils/ast.js';
5+
import { build_pattern, build_fallback, extract_paths } from '../../../../utils/ast.js';
66
import * as b from '#compiler/builders';
77
import { get_rune } from '../../../scope.js';
88
import { walk } from 'zimmerframe';
@@ -181,18 +181,10 @@ function create_state_declarators(declarator, scope, value) {
181181
return [b.declarator(declarator.id, value)];
182182
}
183183

184-
const tmp = scope.generate('tmp');
185-
const paths = extract_paths(declarator.id, true);
184+
const [pattern, replacements] = build_pattern(declarator.id, scope);
186185
return [
187-
b.declarator(
188-
b.id(tmp),
189-
declarator.id.type === 'ArrayPattern' && !is_array(value, scope)
190-
? b.array([b.spread(value)])
191-
: value
192-
), // TODO inject declarator for opts, so we can use it below
193-
...paths.map((path) => {
194-
const value = path.expression?.(b.id(tmp));
195-
return b.declarator(path.node, value);
196-
})
186+
b.declarator(pattern, value),
187+
// TODO inject declarator for opts, so we can use it below
188+
...[...replacements].map(([original, replacement]) => b.declarator(original, replacement))
197189
];
198190
}

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

Lines changed: 37 additions & 38 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_array, 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,57 +23,56 @@ export function visit_assignment_expression(node, context, build_assignment) {
2323

2424
let changed = false;
2525

26-
const assignments = extract_paths(
27-
node.left,
28-
!should_cache && is_array(value, context.state.scope)
29-
).map((path) => {
30-
const value = path.expression?.(rhs);
26+
const [pattern, replacements] = build_pattern(node.left, context.state.scope);
3127

32-
let assignment = build_assignment('=', path.node, value, context);
33-
if (assignment !== null) changed = true;
34-
35-
return (
36-
assignment ??
37-
b.assignment(
38-
'=',
39-
/** @type {Pattern} */ (context.visit(path.node)),
40-
/** @type {Expression} */ (context.visit(value))
41-
)
42-
);
43-
});
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+
'=',
37+
/** @type {Identifier} */ (context.visit(original)),
38+
/** @type {Expression} */ (context.visit(replacement))
39+
)
40+
);
41+
})
42+
];
4443

4544
if (!changed) {
4645
// No change to output -> nothing to transform -> we can keep the original assignment
4746
return null;
4847
}
4948

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

5352
if (!is_standalone) {
5453
// this is part of an expression, we need the sequence to end with the value
55-
sequence.expressions.push(rhs);
54+
block.body.push(b.return(rhs));
5655
}
5756

58-
if (should_cache) {
59-
// the right hand side is a complex expression, wrap in an IIFE to cache it
60-
const iife = b.arrow(
61-
[
62-
!is_array(value, context.state.scope) && node.left.type === 'ArrayPattern'
63-
? b.array_pattern([b.rest(rhs)])
64-
: rhs
65-
],
66-
sequence
67-
);
68-
69-
const iife_is_async =
70-
is_expression_async(value) ||
71-
assignments.some((assignment) => is_expression_async(assignment));
57+
const iife = b.arrow(should_cache ? [rhs] : [], block);
7258

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

76-
return sequence;
73+
return iife_is_async
74+
? b.await(b.call(b.async(iife), should_cache ? value : undefined))
75+
: b.call(iife, should_cache ? value : undefined);
7776
}
7877

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

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ const globals = {
2828
'Math.max': [NUMBER, Math.max],
2929
'Math.random': [NUMBER],
3030
'Math.floor': [NUMBER, Math.floor],
31-
// @ts-expect-error
31+
// @ts-ignore
3232
'Math.f16round': [NUMBER, Math.f16round],
3333
'Math.round': [NUMBER, Math.round],
3434
'Math.abs': [NUMBER, Math.abs],
@@ -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, []);

0 commit comments

Comments
 (0)