fix: detect mutations within assignments expressions (alternative approach) (#12429)

This enhances our "variable was mutated" detection to also recognize that `foo` in `[foo[1]] = [..]` was mutated. This allows us to be more granular about detecting mutations to each expressions in legacy mode, which fixes #12169

---------

Co-authored-by: Simon Holthausen <[email protected]>

Author: Rich-Harris

.changeset/thirty-dogs-whisper.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: detect mutations within assignment expressions

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

@@ -772,8 +772,7 @@ const legacy_scope_tweaker = {
 			}
 
 			if (
-				binding !== null &&
-				binding.kind === 'normal' &&
+				binding?.kind === 'normal' &&
 				((binding.scope === state.instance_scope && binding.declaration_kind !== 'function') ||
 					binding.declaration_kind === 'import')
 			) {
@@ -798,22 +797,22 @@ const legacy_scope_tweaker = {
 					parent.left === binding.node
 				) {
 					binding.kind = 'derived';
-				} else {
-					let idx = -1;
-					let ancestor = path.at(idx);
-					while (ancestor) {
-						if (ancestor.type === 'EachBlock') {
-							// Ensures that the array is reactive when only its entries are mutated
-							// TODO: this doesn't seem correct. We should be checking at the points where
-							// the identifier (the each expression) is used in a way that makes it reactive.
-							// This just forces the collection identifier to always be reactive even if it's
-							// not.
-							if (ancestor.expression === (idx === -1 ? node : path.at(idx + 1))) {
+				}
+			} else if (binding?.kind === 'each' && binding.mutated) {
+				// Ensure that the array is marked as reactive even when only its entries are mutated
+				let i = path.length;
+				while (i--) {
+					const ancestor = path[i];
+					if (
+						ancestor.type === 'EachBlock' &&
+						state.analysis.template.scopes.get(ancestor)?.declarations.get(node.name) === binding
+					) {
+						for (const binding of ancestor.metadata.references) {
+							if (binding.kind === 'normal') {
 								binding.kind = 'state';
-								break;
 							}
 						}
-						ancestor = path.at(--idx);
+						break;
 					}
 				}
 			}

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

@@ -3,7 +3,12 @@ import { walk } from 'zimmerframe';
 import { is_element_node } from './nodes.js';
 import * as b from '../utils/builders.js';
 import * as e from '../errors.js';
-import { extract_identifiers, extract_identifiers_from_destructuring } from '../utils/ast.js';
+import {
+	extract_identifiers,
+	extract_identifiers_from_destructuring,
+	object,
+	unwrap_pattern
+} from '../utils/ast.js';
 import { JsKeywords, Runes } from './constants.js';
 
 export class Scope {
@@ -713,11 +718,14 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 			const binding = scope.get(/** @type {import('estree').Identifier} */ (object).name);
 			if (binding) binding.mutated = true;
 		} else {
-			extract_identifiers(node).forEach((identifier) => {
-				const binding = scope.get(identifier.name);
-				if (binding && identifier !== binding.node) {
+			unwrap_pattern(node).forEach((node) => {
+				let id = node.type === 'Identifier' ? node : object(node);
+				if (id === null) return;
+
+				const binding = scope.get(id.name);
+				if (binding && id !== binding.node) {
 					binding.mutated = true;
-					binding.reassigned = true;
+					binding.reassigned = node.type === 'Identifier';
 				}
 			});
 		}

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

@@ -54,47 +54,62 @@ export function is_event_attribute(attribute) {
 }
 
 /**
- * Extracts all identifiers from a pattern.
- * @param {ESTree.Pattern} param
- * @param {ESTree.Identifier[]} [nodes]
- * @returns {ESTree.Identifier[]}
+ * Extracts all identifiers and member expressions from a pattern.
+ * @param {ESTree.Pattern} pattern
+ * @param {Array<ESTree.Identifier | ESTree.MemberExpression>} [nodes]
+ * @returns {Array<ESTree.Identifier | ESTree.MemberExpression>}
  */
-export function extract_identifiers(param, nodes = []) {
-	switch (param.type) {
+export function unwrap_pattern(pattern, nodes = []) {
+	switch (pattern.type) {
 		case 'Identifier':
-			nodes.push(param);
+			nodes.push(pattern);
+			break;
+
+		case 'MemberExpression':
+			// member expressions can be part of an assignment pattern, but not a binding pattern
+			// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#binding_and_assignment
+			nodes.push(pattern);
 			break;
 
 		case 'ObjectPattern':
-			for (const prop of param.properties) {
+			for (const prop of pattern.properties) {
 				if (prop.type === 'RestElement') {
-					extract_identifiers(prop.argument, nodes);
+					unwrap_pattern(prop.argument, nodes);
 				} else {
-					extract_identifiers(prop.value, nodes);
+					unwrap_pattern(prop.value, nodes);
 				}
 			}
 
 			break;
 
 		case 'ArrayPattern':
-			for (const element of param.elements) {
-				if (element) extract_identifiers(element, nodes);
+			for (const element of pattern.elements) {
+				if (element) unwrap_pattern(element, nodes);
 			}
 
 			break;
 
 		case 'RestElement':
-			extract_identifiers(param.argument, nodes);
+			unwrap_pattern(pattern.argument, nodes);
 			break;
 
 		case 'AssignmentPattern':
-			extract_identifiers(param.left, nodes);
+			unwrap_pattern(pattern.left, nodes);
 			break;
 	}
 
 	return nodes;
 }
 
+/**
+ * Extracts all identifiers from a pattern.
+ * @param {ESTree.Pattern} pattern
+ * @returns {ESTree.Identifier[]}
+ */
+export function extract_identifiers(pattern) {
+	return unwrap_pattern(pattern, []).filter((node) => node.type === 'Identifier');
+}
+
 /**
  * Extracts all identifiers and a stringified keypath from an expression.
  * @param {ESTree.Expression} expr

packages/svelte/tests/migrate/samples/each-block-const/input.svelte

@@ -0,0 +1,5 @@
+<script>
+	const { foo } = x();
+</script>
+
+{#each foo as f}{/each}

packages/svelte/tests/migrate/samples/each-block-const/output.svelte

@@ -0,0 +1,5 @@
+<script>
+	const { foo } = x();
+</script>
+
+{#each foo as f}{/each}