From 82f3be2a66728154451bc2e3fae6531e6e58a894 Mon Sep 17 00:00:00 2001
From: Simon Holthausen <simon.holthausen@vercel.com>
Date: Tue, 28 Nov 2023 14:39:48 +0100
Subject: [PATCH 1/3] fix: untrack reads of mutated object

prevents infinite loops when mutation happens inside an effect
came up in #9639
---
 .changeset/beige-donkeys-remain.md            |  5 ++
 .../compiler/phases/2-analyze/validation.js   | 10 ++--
 .../phases/3-transform/client/utils.js        | 34 ++++++------
 .../3-transform/client/visitors/template.js   |  8 +--
 .../3-transform/server/transform-server.js    | 17 +++---
 .../svelte/src/internal/client/runtime.js     | 27 +++++-----
 .../samples/mutating-state/_config.js         | 54 +++++++++++++++++++
 .../samples/mutating-state/main.svelte        | 10 ++++
 .../samples/mutating-store-state/_config.js   | 54 +++++++++++++++++++
 .../samples/mutating-store-state/main.svelte  | 12 +++++
 10 files changed, 180 insertions(+), 51 deletions(-)
 create mode 100644 .changeset/beige-donkeys-remain.md
 create mode 100644 packages/svelte/tests/runtime-runes/samples/mutating-state/_config.js
 create mode 100644 packages/svelte/tests/runtime-runes/samples/mutating-state/main.svelte
 create mode 100644 packages/svelte/tests/runtime-runes/samples/mutating-store-state/_config.js
 create mode 100644 packages/svelte/tests/runtime-runes/samples/mutating-store-state/main.svelte

diff --git a/.changeset/beige-donkeys-remain.md b/.changeset/beige-donkeys-remain.md
new file mode 100644
index 000000000000..c01b1b5300d5
--- /dev/null
+++ b/.changeset/beige-donkeys-remain.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: untrack reads of mutated object
diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js
index e871808c2cc0..b1fe43087410 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/validation.js
+++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js
@@ -1,5 +1,5 @@
 import { error } from '../../errors.js';
-import { extract_identifiers, is_text_attribute } from '../../utils/ast.js';
+import { extract_identifiers, is_text_attribute, object } from '../../utils/ast.js';
 import { warn } from '../../warnings.js';
 import fuzzymatch from '../1-parse/utils/fuzzymatch.js';
 import { binding_properties } from '../bindings.js';
@@ -248,12 +248,8 @@ export const validation = {
 	BindDirective(node, context) {
 		validate_no_const_assignment(node, node.expression, context.state.scope, true);
 
-		let left = node.expression;
-		while (left.type === 'MemberExpression') {
-			left = /** @type {import('estree').MemberExpression} */ (left.object);
-		}
-
-		if (left.type !== 'Identifier') {
+		const left = object(node.expression);
+		if (left === null) {
 			error(node, 'invalid-binding-expression');
 		}
 
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js
index 1194cffcde46..c1d053c809dd 100644
--- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js
+++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js
@@ -205,27 +205,31 @@ export function serialize_set_binding(node, context, fallback) {
 				return b.call('$.set', b.id(left_name), value);
 			}
 		} else {
+			const left = /** @type {import('estree').MemberExpression} */ (visit(node.left));
+			// When reading the object that's mutated we need to untrack that read in order
+			// to avoid infinite loops. $.mutate(_store) does that by accepting a callback
+			// function it hands the value to. For this, we need to adjust the code to
+			// reference that passed value instead of reading the object again.
+			let id = left;
+			while (id.type === 'MemberExpression') {
+				if (id.object.type !== 'MemberExpression') {
+					id.object = b.id('$$to_mutate');
+					break;
+				} else {
+					id = id.object;
+				}
+			}
+			const update = b.arrow([b.id('$$to_mutate')], b.assignment(node.operator, left, value));
+
 			if (is_store) {
 				return b.call(
 					'$.mutate_store',
 					serialize_get_binding(b.id(left_name), state),
-					b.assignment(
-						node.operator,
-						/** @type {import('estree').Pattern} */ (visit(node.left)),
-						value
-					),
-					b.call('$' + left_name)
+					b.id('$' + left_name),
+					update
 				);
 			} else {
-				return b.call(
-					'$.mutate',
-					b.id(left_name),
-					b.assignment(
-						node.operator,
-						/** @type {import('estree').Pattern} */ (visit(node.left)),
-						value
-					)
-				);
+				return b.call('$.mutate', b.id(left_name), update);
 			}
 		}
 	};
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
index 855a7952844d..45360de59921 100644
--- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
+++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
@@ -206,13 +206,7 @@ function collect_transitive_dependencies(binding, seen = new Set()) {
  * @param {import('../types.js').ComponentContext} context
  */
 function setup_select_synchronization(value_binding, context) {
-	let bound = value_binding.expression;
-	while (bound.type === 'MemberExpression') {
-		bound = /** @type {import('estree').Identifier | import('estree').MemberExpression} */ (
-			bound.object
-		);
-	}
-
+	const bound = /** @type {import('estree').Identifier} */ (object(value_binding.expression));
 	/** @type {string[]} */
 	const names = [];
 
diff --git a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js
index a3fff51495a4..64ad644e8a57 100644
--- a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js
+++ b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js
@@ -1,6 +1,11 @@
 import { walk } from 'zimmerframe';
 import { set_scope, get_rune } from '../../scope.js';
-import { extract_identifiers, extract_paths, is_event_attribute } from '../../../utils/ast.js';
+import {
+	extract_identifiers,
+	extract_paths,
+	is_event_attribute,
+	object
+} from '../../../utils/ast.js';
 import * as b from '../../../utils/builders.js';
 import is_reference from 'is-reference';
 import {
@@ -417,14 +422,8 @@ function serialize_set_binding(node, context, fallback) {
 		error(node, 'INTERNAL', `Unexpected assignment type ${node.left.type}`);
 	}
 
-	let left = node.left;
-
-	while (left.type === 'MemberExpression') {
-		// @ts-expect-error
-		left = left.object;
-	}
-
-	if (left.type !== 'Identifier') {
+	const left = object(node.left);
+	if (left === null) {
 		return fallback();
 	}
 
diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
index 961051556355..23c6850a97c3 100644
--- a/packages/svelte/src/internal/client/runtime.js
+++ b/packages/svelte/src/internal/client/runtime.js
@@ -862,7 +862,7 @@ export function invalidate_inner_signals(fn) {
 	}
 	let signal;
 	for (signal of captured_signals) {
-		mutate(signal, null /* doesnt matter */);
+		mutate(signal, () => {} /* doesnt matter */);
 	}
 	return captured_signals;
 }
@@ -870,26 +870,27 @@ export function invalidate_inner_signals(fn) {
 /**
  * @template V
  * @param {import('./types.js').Signal<V>} source
- * @param {V} value
+ * @param {(v: V) => any} update
  */
-export function mutate(source, value) {
-	set_signal_value(
-		source,
-		untrack(() => get(source))
-	);
-	return value;
+export function mutate(source, update) {
+	var value = untrack(() => get(source));
+	var updated = update(value);
+	set_signal_value(source, value);
+	return updated;
 }
 
 /**
  * Updates a store with a new value.
  * @param {import('./types.js').Store<V>} store  the store to update
- * @param {any} expression  the expression that mutates the store
- * @param {V} new_value  the new store value
+ * @param {() => V} get_value  function to retrieve the current store value
+ * @param {(v: V) => any} update  the expression that mutates the store
  * @template V
  */
-export function mutate_store(store, expression, new_value) {
-	store.set(new_value);
-	return expression;
+export function mutate_store(store, get_value, update) {
+	var updated = update(untrack(get_value));
+	// mutation could result in a new store being tracked, therefore call get_value again
+	store.set(untrack(get_value));
+	return updated;
 }
 
 /**
diff --git a/packages/svelte/tests/runtime-runes/samples/mutating-state/_config.js b/packages/svelte/tests/runtime-runes/samples/mutating-state/_config.js
new file mode 100644
index 000000000000..7f8e782105a5
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/mutating-state/_config.js
@@ -0,0 +1,54 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+// Test ensures that reading the state that's mutated is done in a manner
+// so that the read is untracked so it doesn't trigger infinite loops
+export default test({
+	html: `
+		<input>
+		<input>
+		<p>text: foo</p>
+		<p>wrapped.contents: foo</p>
+	`,
+	skip_if_ssr: true,
+
+	test({ assert, target }) {
+		const [input1, input2] = target.querySelectorAll('input');
+
+		input1.value = 'bar';
+		flushSync(() => input1.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: bar</p>
+			<p>wrapped.contents: bar</p>
+		`
+		);
+
+		input2.value = 'baz';
+		flushSync(() => input2.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: bar</p>
+			<p>wrapped.contents: baz</p>
+		`
+		);
+
+		input1.value = 'foo';
+		flushSync(() => input1.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: foo</p>
+			<p>wrapped.contents: foo</p>
+		`
+		);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/mutating-state/main.svelte b/packages/svelte/tests/runtime-runes/samples/mutating-state/main.svelte
new file mode 100644
index 000000000000..84d7c3e4cc52
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/mutating-state/main.svelte
@@ -0,0 +1,10 @@
+<script>
+	let text = $state('foo');
+	let wrapped = $state({});
+	$effect(() => { wrapped.contents = text; })
+</script>
+
+<input bind:value={text} />
+<input bind:value={wrapped.contents} />
+<p>text: {text}</p>
+<p>wrapped.contents: {wrapped.contents}</p> 
diff --git a/packages/svelte/tests/runtime-runes/samples/mutating-store-state/_config.js b/packages/svelte/tests/runtime-runes/samples/mutating-store-state/_config.js
new file mode 100644
index 000000000000..7f8e782105a5
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/mutating-store-state/_config.js
@@ -0,0 +1,54 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+// Test ensures that reading the state that's mutated is done in a manner
+// so that the read is untracked so it doesn't trigger infinite loops
+export default test({
+	html: `
+		<input>
+		<input>
+		<p>text: foo</p>
+		<p>wrapped.contents: foo</p>
+	`,
+	skip_if_ssr: true,
+
+	test({ assert, target }) {
+		const [input1, input2] = target.querySelectorAll('input');
+
+		input1.value = 'bar';
+		flushSync(() => input1.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: bar</p>
+			<p>wrapped.contents: bar</p>
+		`
+		);
+
+		input2.value = 'baz';
+		flushSync(() => input2.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: bar</p>
+			<p>wrapped.contents: baz</p>
+		`
+		);
+
+		input1.value = 'foo';
+		flushSync(() => input1.dispatchEvent(new Event('input')));
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<input>
+			<input>
+			<p>text: foo</p>
+			<p>wrapped.contents: foo</p>
+		`
+		);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/mutating-store-state/main.svelte b/packages/svelte/tests/runtime-runes/samples/mutating-store-state/main.svelte
new file mode 100644
index 000000000000..3e297d3118cb
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/mutating-store-state/main.svelte
@@ -0,0 +1,12 @@
+<script>
+	import { writable } from "svelte/store";
+
+	let text = writable('foo');
+	let wrapped = writable({});
+	$effect(() => { $wrapped.contents = $text; })
+</script>
+
+<input bind:value={$text} />
+<input bind:value={$wrapped.contents} />
+<p>text: {$text}</p>
+<p>wrapped.contents: {$wrapped.contents}</p> 

From 84e2a407ea0543af78795abbf76ef862cc771487 Mon Sep 17 00:00:00 2001
From: Simon Holthausen <simon.holthausen@vercel.com>
Date: Tue, 28 Nov 2023 14:53:44 +0100
Subject: [PATCH 2/3] simplify

---
 packages/svelte/src/internal/client/runtime.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
index 23c6850a97c3..21093fda560b 100644
--- a/packages/svelte/src/internal/client/runtime.js
+++ b/packages/svelte/src/internal/client/runtime.js
@@ -887,9 +887,9 @@ export function mutate(source, update) {
  * @template V
  */
 export function mutate_store(store, get_value, update) {
-	var updated = update(untrack(get_value));
-	// mutation could result in a new store being tracked, therefore call get_value again
-	store.set(untrack(get_value));
+	const value = untrack(get_value);
+	var updated = update(value);
+	store.set(value);
 	return updated;
 }
 

From be2c34b3b9b8f0b331e54186ffd632c67f6264d9 Mon Sep 17 00:00:00 2001
From: Simon Holthausen <simon.holthausen@vercel.com>
Date: Tue, 28 Nov 2023 14:53:59 +0100
Subject: [PATCH 3/3] var

---
 packages/svelte/src/internal/client/runtime.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
index 21093fda560b..126a43cd5d02 100644
--- a/packages/svelte/src/internal/client/runtime.js
+++ b/packages/svelte/src/internal/client/runtime.js
@@ -887,7 +887,7 @@ export function mutate(source, update) {
  * @template V
  */
 export function mutate_store(store, get_value, update) {
-	const value = untrack(get_value);
+	var value = untrack(get_value);
 	var updated = update(value);
 	store.set(value);
 	return updated;