fix: prevent false positive ownership warning when reassigning state

When a proxy is reassigned, we call `$.proxy` again. There are cases where there's a component context set but the reassignment actually happens for variable that is ownerless within shared state or somewhere else. In that case we get false positives right now. The inverse is also true where reassigning can delete owners (because no component context exists) and result in false negatives. The fix is to pass the previous value in to copy over the owners from it.

Fixes #11525

Author: dummdidumm

.changeset/modern-apricots-promise.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: prevent buggy ownership warning when reassigning state

packages/svelte/src/compiler/phases/3-transform/client/utils.js

@@ -203,7 +203,7 @@ export function serialize_set_binding(node, context, fallback, options) {
 							assignment.right =
 								private_state.kind === 'frozen_state'
 									? b.call('$.freeze', value)
-									: b.call('$.proxy', value);
+									: serialize_proxy_reassignment(value, private_state.id, state);
 							return assignment;
 						}
 					}
@@ -216,7 +216,7 @@ export function serialize_set_binding(node, context, fallback, options) {
 							should_proxy_or_freeze(value, context.state.scope)
 							? private_state.kind === 'frozen_state'
 								? b.call('$.freeze', value)
-								: b.call('$.proxy', value)
+								: serialize_proxy_reassignment(value, private_state.id, state)
 							: value
 					);
 				}
@@ -240,7 +240,7 @@ export function serialize_set_binding(node, context, fallback, options) {
 					assignment.right =
 						public_state.kind === 'frozen_state'
 							? b.call('$.freeze', value)
-							: b.call('$.proxy', value);
+							: serialize_proxy_reassignment(value, public_state.id, state);
 					return assignment;
 				}
 			}
@@ -305,7 +305,7 @@ export function serialize_set_binding(node, context, fallback, options) {
 						context.state.analysis.runes &&
 							!options?.skip_proxy_and_freeze &&
 							should_proxy_or_freeze(value, context.state.scope)
-							? b.call('$.proxy', value)
+							? serialize_proxy_reassignment(value, left_name, state)
 							: value
 					);
 				} else if (binding.kind === 'frozen_state') {
@@ -410,6 +410,25 @@ export function serialize_set_binding(node, context, fallback, options) {
 	return serialize();
 }
 
+/**
+ * @param {import('estree').Expression} value
+ * @param {import('estree').PrivateIdentifier | string} proxy_reference
+ * @param {import('./types').ClientTransformState} state
+ */
+export function serialize_proxy_reassignment(value, proxy_reference, state) {
+	return state.options.dev
+		? b.call(
+				'$.proxy',
+				value,
+				b.true,
+				b.null,
+				typeof proxy_reference === 'string'
+					? b.id(proxy_reference)
+					: b.member(b.this, proxy_reference)
+			)
+		: b.call('$.proxy', value);
+}
+
 /**
  * @param {import('estree').ArrowFunctionExpression | import('estree').FunctionExpression} node
  * @param {import('./types').ComponentContext} context

packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js

@@ -2,7 +2,12 @@ import { get_rune } from '../../../scope.js';
 import { is_hoistable_function, transform_inspect_rune } from '../../utils.js';
 import * as b from '../../../../utils/builders.js';
 import * as assert from '../../../../utils/assert.js';
-import { get_prop_source, is_state_source, should_proxy_or_freeze } from '../utils.js';
+import {
+	get_prop_source,
+	is_state_source,
+	serialize_proxy_reassignment,
+	should_proxy_or_freeze
+} from '../utils.js';
 import { extract_paths } from '../../../../utils/ast.js';
 import { regex_invalid_identifier_chars } from '../../../patterns.js';
 
@@ -139,7 +144,11 @@ export const javascript_visitors_runes = {
 									'set',
 									definition.key,
 									[value],
-									[b.stmt(b.call('$.set', member, b.call('$.proxy', value)))]
+									[
+										b.stmt(
+											b.call('$.set', member, serialize_proxy_reassignment(value, field.id, state))
+										)
+									]
 								)
 							);
 						}

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

@@ -469,6 +469,7 @@ export function do_while(test, body) {
 
 const true_instance = literal(true);
 const false_instance = literal(false);
+const null_instane = literal(null);
 
 /** @type {import('estree').DebuggerStatement} */
 const debugger_builder = {
@@ -630,6 +631,7 @@ export {
 	return_builder as return,
 	if_builder as if,
 	this_instance as this,
+	null_instane as null,
 	debugger_builder as debugger
 };
 

packages/svelte/src/internal/client/proxy.js

@@ -27,9 +27,10 @@ import * as e from './errors.js';
  * @param {T} value
  * @param {boolean} [immutable]
  * @param {import('#client').ProxyMetadata | null} [parent]
+ * @param {import('#client').Source<T>} [prev] dev mode only
  * @returns {import('#client').ProxyStateObject<T> | T}
  */
-export function proxy(value, immutable = true, parent = null) {
+export function proxy(value, immutable = true, parent = null, prev) {
 	if (typeof value === 'object' && value != null && !is_frozen(value)) {
 		// If we have an existing proxy, return it...
 		if (STATE_SYMBOL in value) {
@@ -71,13 +72,22 @@ export function proxy(value, immutable = true, parent = null) {
 				// @ts-expect-error
 				value[STATE_SYMBOL].parent = parent;
 
-				// @ts-expect-error
-				value[STATE_SYMBOL].owners =
-					parent === null
-						? current_component_context !== null
-							? new Set([current_component_context.function])
-							: null
-						: new Set();
+				if (prev) {
+					// Reuse owners from previous state; necessary because reassignment is not guaranteed to have correct component context.
+					// If no previous proxy exists we play it safe and assume ownerless state
+					// @ts-expect-error
+					const prev_owners = prev?.v?.[STATE_SYMBOL]?.owners;
+					// @ts-expect-error
+					value[STATE_SYMBOL].owners = prev_owners ? new Set(prev_owners) : null;
+				} else {
+					// @ts-expect-error
+					value[STATE_SYMBOL].owners =
+						parent === null
+							? current_component_context !== null
+								? new Set([current_component_context.function])
+								: null
+							: new Set();
+				}
 			}
 
 			return proxy;

packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte

@@ -1,8 +1,8 @@
 <script>
-	/** @type {{ object: { count: number }}} */
-	let { object = $bindable() } = $props();
+	let { object = $bindable(), reset } = $props();
 </script>
 
 <button onclick={() => object.count += 1}>
 	clicks: {object.count}
 </button>
+<button onclick={reset}>reset</button>

packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js

@@ -1,36 +1,30 @@
 import { flushSync } from 'svelte';
 import { test } from '../../test';
 
-/** @type {typeof console.trace} */
-let trace;
-
 export default test({
-	html: `<button>clicks: 0</button>`,
+	html: `<button>clicks: 0</button> <button>reset</button>`,
 
 	compileOptions: {
 		dev: true
 	},
 
-	before_test: () => {
-		trace = console.trace;
-		console.trace = () => {};
-	},
-
-	after_test: () => {
-		console.trace = trace;
-	},
-
 	test({ assert, target, warnings }) {
-		const btn = target.querySelector('button');
-
-		flushSync(() => {
-			btn?.click();
-		});
-
-		assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button>`);
-
-		assert.deepEqual(warnings, [
-			'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead'
-		]);
+		const warning =
+			'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead';
+		const [btn1, btn2] = target.querySelectorAll('button');
+
+		btn1.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button> <button>reset</button>`);
+		assert.deepEqual(warnings, [warning]);
+
+		btn2.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>clicks: 0</button> <button>reset</button>`);
+
+		btn1.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>clicks: 1</button> <button>reset</button>`);
+		assert.deepEqual(warnings, [warning, warning]);
 	}
 });

packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/main.svelte

@@ -1,7 +1,7 @@
 <script>
 	import Counter from './Counter.svelte';
 
-	const object = $state({ count: 0 });
+	let object = $state({ count: 0 });
 </script>
 
-<Counter {object} />
+<Counter {object} reset={() => object = {count: 0}} />

packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js

@@ -0,0 +1,11 @@
+import { test } from '../../test';
+
+export default test({
+	compileOptions: {
+		dev: true
+	},
+
+	async test({ assert, warnings }) {
+		assert.deepEqual(warnings, []);
+	}
+});

packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte

@@ -0,0 +1,18 @@
+<script context="module">
+	let toast1 = $state();
+	let toast2 = $state({});
+
+	export async function show_toast() {
+		toast1 = {
+			message: 'foo',
+			show: true
+		};
+		toast1.show = false;
+		
+		toast2 = {
+			message: 'foo',
+			show: true
+		};
+		toast2.show = false;
+	}
+</script>

packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte

@@ -0,0 +1,5 @@
+<script>
+	import { show_toast } from "./child.svelte";
+
+	show_toast();
+</script>