fix: recurse into $derived for ownership validation

.changeset/thick-carrots-arrive.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: recurse into `$derived` for ownership validation

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

@@ -190,22 +190,21 @@ export function ClassBody(node, context) {
 				'method',
 				b.id('$.ADD_OWNER'),
 				[b.id('owner')],
-				Array.from(public_state)
-					// Only run ownership addition on $state fields.
-					// Theoretically someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`,
-					// but that feels so much of an edge case that it doesn't warrant a perf hit for the common case.
-					.filter(([_, { kind }]) => kind === 'state')
-					.map(([name]) =>
-						b.stmt(
-							b.call(
-								'$.add_owner',
-								b.call('$.get', b.member(b.this, b.private_id(name))),
-								b.id('owner'),
-								b.literal(false),
-								is_ignored(node, 'ownership_invalid_binding') && b.true
-							)
+				[
+					b.stmt(
+						b.call(
+							'$.add_owner_to_class',
+							b.this,
+							b.id('owner'),
+							b.array(
+								Array.from(public_state).map(([name]) =>
+									b.thunk(b.call('$.get', b.member(b.this, b.private_id(name))))
+								)
+							),
+							is_ignored(node, 'ownership_invalid_binding') && b.true
 						)
-					),
+					)
+				],
 				true
 			)
 		);

packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js

@@ -180,37 +180,18 @@ export function build_component(node, component_name, context, anchor = context.
 			const expression = /** @type {Expression} */ (context.visit(attribute.expression));
 
 			if (dev && attribute.name !== 'this') {
-				let should_add_owner = true;
-
-				if (attribute.expression.type !== 'SequenceExpression') {
-					const left = object(attribute.expression);
-
-					if (left?.type === 'Identifier') {
-						const binding = context.state.scope.get(left.name);
-
-						// Only run ownership addition on $state fields.
-						// Theoretically someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`,
-						// but that feels so much of an edge case that it doesn't warrant a perf hit for the common case.
-						if (binding?.kind === 'derived' || binding?.kind === 'raw_state') {
-							should_add_owner = false;
-						}
-					}
-				}
-
-				if (should_add_owner) {
-					binding_initializers.push(
-						b.stmt(
-							b.call(
-								b.id('$.add_owner_effect'),
-								expression.type === 'SequenceExpression'
-									? expression.expressions[0]
-									: b.thunk(expression),
-								b.id(component_name),
-								is_ignored(node, 'ownership_invalid_binding') && b.true
-							)
+				binding_initializers.push(
+					b.stmt(
+						b.call(
+							b.id('$.add_owner_effect'),
+							expression.type === 'SequenceExpression'
+								? expression.expressions[0]
+								: b.thunk(expression),
+							b.id(component_name),
+							is_ignored(node, 'ownership_invalid_binding') && b.true
 						)
-					);
-				}
+					)
+				);
 			}
 
 			if (expression.type === 'SequenceExpression') {

packages/svelte/src/internal/client/dev/ownership.js

@@ -6,7 +6,7 @@ import { render_effect, user_pre_effect } from '../reactivity/effects.js';
 import { dev_current_component_function } from '../context.js';
 import { get_prototype_of } from '../../shared/utils.js';
 import * as w from '../warnings.js';
-import { FILENAME } from '../../../constants.js';
+import { FILENAME, UNINITIALIZED } from '../../../constants.js';
 
 /** @type {Record<string, Array<{ start: Location, end: Location, component: Function }>>} */
 const boundaries = {};
@@ -140,6 +140,25 @@ export function add_owner_effect(get_object, Component, skip_warning = false) {
 	});
 }
 
+/**
+ * @param {any} _this
+ * @param {Function} owner
+ * @param {Array<() => any>} getters
+ * @param {boolean} skip_warning
+ */
+export function add_owner_to_class(_this, owner, getters, skip_warning) {
+	_this[ADD_OWNER].current ||= getters.map(() => UNINITIALIZED);
+
+	for (let i = 0; i < getters.length; i += 1) {
+		const current = getters[i]();
+		// For performance reasons we only re-add the owner if the state has changed
+		if (current !== _this[ADD_OWNER][i]) {
+			_this[ADD_OWNER].current[i] = current;
+			add_owner(current, owner, false, skip_warning);
+		}
+	}
+}
+
 /**
  * @param {ProxyMetadata | null} from
  * @param {ProxyMetadata} to
@@ -196,7 +215,19 @@ function add_owner_to_object(object, owner, seen) {
 			if (proto === Object.prototype) {
 				// recurse until we find a state proxy
 				for (const key in object) {
-					add_owner_to_object(object[key], owner, seen);
+					if (Object.getOwnPropertyDescriptor(object, key)?.get) {
+						// Similar to the class case; the getter could update with a new state
+						let current = UNINITIALIZED;
+						render_effect(() => {
+							const next = object[key];
+							if (current !== next) {
+								current = next;
+								add_owner_to_object(next, owner, seen);
+							}
+						});
+					} else {
+						add_owner_to_object(object[key], owner, seen);
+					}
 				}
 			} else if (proto === Array.prototype) {
 				// recurse until we find a state proxy
@@ -221,9 +252,10 @@ function has_owner(metadata, component) {
 	return (
 		metadata.owners.has(component) ||
 		// This helps avoid false positives when using HMR, where the component function is replaced
-		[...metadata.owners].some(
-			(owner) => /** @type {any} */ (owner)[FILENAME] === /** @type {any} */ (component)?.[FILENAME]
-		) ||
+		(FILENAME in component &&
+			[...metadata.owners].some(
+				(owner) => /** @type {any} */ (owner)[FILENAME] === component[FILENAME]
+			)) ||
 		(metadata.parent !== null && has_owner(metadata.parent, component))
 	);
 }

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

@@ -10,6 +10,7 @@ export {
 	mark_module_start,
 	mark_module_end,
 	add_owner_effect,
+	add_owner_to_class,
 	skip_ownership_validation
 } from './dev/ownership.js';
 export { check_target, legacy_api } from './dev/legacy.js';

packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte

@@ -0,0 +1,7 @@
+<script>
+	let { linked3 = $bindable(), linked4 = $bindable() } = $props();
+</script>
+
+<p>Binding</p>
+<button onclick={() => linked3.count++}>Increment Linked 1 ({linked3.count})</button>
+<button onclick={() => linked4.count++}>Increment Linked 2 ({linked4.count})</button>

packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte

@@ -0,0 +1,13 @@
+<script>
+	import { getContext } from 'svelte';
+	const linked1 = getContext('linked1');
+	const linked2 = getContext('linked2');
+</script>
+
+<p>Context</p>
+<button onclick={() => linked1.linked.current.count++}
+	>Increment Linked 1 ({linked1.linked.current.count})</button
+>
+<button onclick={() => linked2.linked.current.count++}
+	>Increment Linked 2 ({linked2.linked.current.count})</button
+>

packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js

@@ -0,0 +1,34 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+// Tests that ownership is widened with $derived (on class or on its own) that contains $state
+export default test({
+	compileOptions: {
+		dev: true
+	},
+
+	test({ assert, target, warnings }) {
+		const [root, counter_context1, counter_context2, counter_binding1, counter_binding2] =
+			target.querySelectorAll('button');
+
+		counter_context1.click();
+		counter_context2.click();
+		counter_binding1.click();
+		counter_binding2.click();
+		flushSync();
+
+		assert.equal(warnings.length, 0);
+
+		root.click();
+		flushSync();
+		counter_context1.click();
+		counter_context2.click();
+		counter_binding1.click();
+		counter_binding2.click();
+		flushSync();
+
+		assert.equal(warnings.length, 0);
+	},
+
+	warnings: []
+});

packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte

@@ -0,0 +1,82 @@
+<script>
+	import CounterBinding from './CounterBinding.svelte';
+	import CounterContext from './CounterContext.svelte';
+	import { setContext } from 'svelte';
+
+	let counter = $state({ count: 0 });
+
+	class Linked {
+		#getter;
+		linked = $derived.by(() => {
+			const state = $state({ current: $state.snapshot(this.#getter()) });
+			return state;
+		});
+
+		constructor(fn) {
+			this.#getter = fn;
+		}
+	}
+
+	const linked1 = $derived.by(() => {
+		const state = $state({ current: $state.snapshot(counter) });
+		return state;
+	});
+	const linked2 = new Linked(() => counter);
+
+	setContext('linked1', {
+		get linked() {
+			return linked1;
+		}
+	});
+	setContext('linked2', linked2);
+
+	const linked3 = $derived.by(() => {
+		const state = $state({ current: $state.snapshot(counter) });
+		return state;
+	});
+	const linked4 = new Linked(() => counter);
+</script>
+
+<p>Parent</p>
+<button onclick={() => counter.count++}>
+	Increment Original ({counter.count})
+</button>
+
+<CounterContext />
+<CounterBinding bind:linked3={linked3.current} bind:linked4={linked4.linked.current} />
+
+<!-- <script>
+    import { createPortalKey } from 'svelte';
+    let x = createPortalKey();
+    let count = $state(0);
+    let root = $state();
+
+    $effect(() => {
+        root = document.querySelector('#root');
+    })
+</script>
+
+<div>
+    <svelte:portal for={x}></svelte:portal>
+</div>
+
+<button onclick={() => count++}>increment</button>
+
+<svelte:portal target={x}>
+    hello {count}
+</svelte:portal>
+
+{#if count < 5}
+    <svelte:portal target={x}>
+        <span>hello2 {count}</span>
+    </svelte:portal>
+{/if}
+
+<p></p>
+
+<svelte:portal target="{root}">
+    I'm rendered in the
+    <button onclick={() => root = document.querySelector('p')}>
+        {root === document.querySelector('#root') ? 'root' : 'p'}
+    </button>
+</svelte:portal> -->