fix: handle derived destructured iterators

[Rich-Harris]

Closes #15996

I've sunk way too much time into this and can't justify sinking any more right now, but basically this reverts #15813 (sorry @Ocean-OS, I should have been more careful before merging it), tweaks some of the extract_paths logic, and adds a failing test.

Right now this is broken...

function* count(offset) {
  let i = offset;
  while (true) yield i++;
}
	
let [a, b, c] = $derived(count(offset));

...because the let [$$1, $$2, $$3] = $.get($$d) gets generated for each of a, b and c. But after the first one, the generator is 'spent', and everything after is undefined.

We instead need to do the destructuring once to create the array, and reference members of that array (or, in the case of a rest element, $$array.slice(index)). Or as an alternative to destructuring we could probably do something like this:

// for a case like `let [a, b, c] = $derived(...)`
let $$array = $.derived(() => $.slice(maybe_iterator, 3));

// for a case like `let [a, b, c, ...rest] = $derived(...)`
let $$array = $.derived(() => $.slice(maybe_iterator));

Another issue with #15813 is that it only changes the logic in the VariableDeclaration visitor, but if we want to be rigorous we need similar handling for other places where extract_paths is used, e.g. each blocks.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: 02f48a1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-svelte-16015-svelte.vercel.app/

[github-actions]

Playground

pnpm add https://pkg.pr.new/svelte@16015

[Ocean-OS]

I see, I'm sorry for the problems #15813 caused... I had a feeling something like #15996 might be an issue but I wasn't sure how to solve it, and I hadn't thought of the issues you're describing here...

[Rich-Harris]

Nothing to apologise for! It seemed like a totally reasonable solution. I personally haven't yet formed the right mental habits around generators, so I tend to miss these sorts of issues

[dummdidumm]

Looks good, hence approving, but can't help but wonder if we should generate a name within extract_paths that is deduplicated at the root scope level instead. That would save us from having to do id.name = ... at all call sites, and also makes this less prone to bugs where the id node is somehow switched out inside some visitor. You could also pass in the scope id generator function and/or a preferred variable name, if it's about readability.

Oh and this needs a changeset

[Rich-Harris]

can't help but wonder if we should generate a name within extract_paths that is deduplicated at the root scope level instead

I did wonder about that, but the fact that we're using extract_paths in the analysis phase would mean we'd be polluting the scope with an unused name. It would still work, of course, but it feels a little messy. Agree that it feels a little duplicative though. Part of me almost wonders if we should skip the upfront deconflicting and do it in a final AST walk, but that would obviously be a separate job. I'm not too concerned about nodes being switched out, if we started doing that the tests would start failing pretty quickly

[dummdidumm]

I think all the call sites use a well-known scope function, so as an alternative you could pass the id creator function into extract_paths

-		const { inserts, paths } = extract_paths(node.context, unwrapped);
+		const { inserts, paths } = extract_paths(node.context, unwrapped, context.state.scope.generate);

		for (const { id, value } of inserts) {
-			id.name = context.state.scope.generate('$$array');
			child_state.transform[id.name] = { read: get_value };

			const expression = /** @type {Expression} */ (context.visit(b.thunk(value), child_state));
			declarations.push(b.var(id, b.call('$.derived', expression)));
		}

but not too concerned about the current state either; your choice

[Rich-Harris]

Thing is you'd need to pass an arrow function, since it's not bound, so the diff would actually look like this...

-		const { inserts, paths } = extract_paths(node.context, unwrapped);
+		const { inserts, paths } = extract_paths(
+			node.context,
+			unwrapped,
+			(name) => context.state.scope.generate(name)
+		);

		for (const { id, value } of inserts) {
-			id.name = context.state.scope.generate('$$array');
			child_state.transform[id.name] = { read: get_value };

			const expression = /** @type {Expression} */ (context.visit(b.thunk(value), child_state));
			declarations.push(b.var(id, b.call('$.derived', expression)));
		}

...so you don't really gain anything except indirection

[kkarikos]

It seems this commit caused our builds to start failing. We haven't yet migrated all our components to rune based syntax so one of these older components had something like:

<script lang="ts">
  // ...
  export let data;
  let [timeseriesData, minValue, maxValue] = createChartData(data);
</script>

This started causing pretty weird: Expected ident (Note that you need plugins to import files that are not JavaScript).

Stack trace:

at getRollupError (file:///project/node_modules/rollup/dist/es/shared/parseAst.js:397:41)
at ParseError.initialise (file:///project/node_modules/rollup/dist/es/shared/node-entry.js:14168:28)
at convertNode (file:///project/node_modules/rollup/dist/es/shared/node-entry.js:16062:10)
at convertProgram (file:///project/node_modules/rollup/dist/es/shared/node-entry.js:15305:12)
at Module.setSource (file:///project/node_modules/rollup/dist/es/shared/node-entry.js:17049:24)
at async ModuleLoader.addModuleSource (file:///project/node_modules/rollup/dist/es/shared/node-entry.js:20958:13)

I was able to get past this by changing the implementation to:

<script lang="ts">
  // ...
  export let data;
  const initialData = createChartData(data);
  let timeSeriesData = initialData[0];
  let minValue = initialData[1];
  let maxValue = initialData[2];
</script>

Breaking version is 5.33.5.

[paoloricciuti]

@kkarikos please open a new issue with an actual reproduction...the generated code from this snippet you posted is not different between 5.33.4 and 5.33.5 so while it might have broken something for real we need a repro to fix it. Thanks for reporting! 🧡

[rChaoz]

I believe this PR breaks Zag JS. I've found that it breaks their menu component; it seems like a proxied state set (or get) doesn't work correctly anymore, see chakra-ui/zag#2502.

It might also be the other change in 5.33.5 (#15961) but I think it's unlikely as it's attachment-related (not used by Zag).

[paoloricciuti]

I believe this PR breaks Zag JS. I've found that it breaks their menu component; it seems like a proxied state set (or get) doesn't work correctly anymore, see chakra-ui/zag#2502.

It might also be the other change in 5.33.5 (#15961) but I think it's unlikely as it's attachment-related (not used by Zag).

It's already fixed in the latest version of svelte

[rChaoz]

No, it's not, wait... Oh, dang, it's a Firefox-only issue. This spices things up a bit.