fix: ensure assignments to state field inside constructor trigger effect (#12985)

* fix: ensure assignments to state field inside constructor trigger effects

* address feedback

* address feedback

* address feedback

* add test

* alternative approach

* lint

* error on reading local source in derived

* build

* add changeset for self-dependency error

* revert unused changes

* revert unused changes

* Update packages/svelte/messages/client-errors/errors.md

* regenerate

* tweak

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>

.changeset/clean-shirts-yawn.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+breaking: throw error if derived creates state and then depends on it

.changeset/five-maps-reflect.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure assignments to state field inside constructor trigger effects

packages/svelte/messages/client-errors/errors.md

@@ -72,6 +72,10 @@
 
 > Cannot set prototype of `$state` object
 
+## state_unsafe_local_read
+
+> Reading state that was created inside the same derived is forbidden. Consider using `untrack` to read locally created state
+
 ## state_unsafe_mutation
 
 > Updating state inside a derived is forbidden. If the value should not be reactive, declare it without `$state`

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

@@ -57,20 +57,6 @@ function build_assignment(operator, left, right, context) {
 					return b.assignment(operator, /** @type {Pattern} */ (context.visit(left)), value);
 				}
 			}
-		} else if (left.property.type === 'Identifier' && context.state.in_constructor) {
-			const public_state = context.state.public_state.get(left.property.name);
-
-			if (public_state !== undefined && should_proxy(right, context.state.scope)) {
-				const value = /** @type {Expression} */ (context.visit(right));
-
-				return b.assignment(
-					operator,
-					/** @type {Pattern} */ (context.visit(left)),
-					public_state.kind === 'raw_state'
-						? value
-						: build_proxy_reassignment(value, public_state.id)
-				);
-			}
 		}
 	}
 

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

@@ -13,15 +13,6 @@ export function MemberExpression(node, context) {
 		if (field) {
 			return context.state.in_constructor ? b.member(node, 'v') : b.call('$.get', node);
 		}
-	} else if (node.object.type === 'ThisExpression') {
-		// rewrite `this.foo` as `this.#foo.v` inside a constructor
-		if (node.property.type === 'Identifier' && !node.computed) {
-			const field = context.state.public_state.get(node.property.name);
-
-			if (field && context.state.in_constructor) {
-				return b.member(b.member(b.this, field.id), 'v');
-			}
-		}
 	}
 
 	context.next();

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

@@ -310,6 +310,22 @@ export function state_prototype_fixed() {
 	}
 }
 
+/**
+ * Reading state that was created inside the same derived is forbidden. Consider using `untrack` to read locally created state
+ * @returns {never}
+ */
+export function state_unsafe_local_read() {
+	if (DEV) {
+		const error = new Error(`state_unsafe_local_read\nReading state that was created inside the same derived is forbidden. Consider using \`untrack\` to read locally created state`);
+
+		error.name = 'Svelte error';
+		throw error;
+	} else {
+		// TODO print a link to the documentation
+		throw new Error("state_unsafe_local_read");
+	}
+}
+
 /**
  * Updating state inside a derived is forbidden. If the value should not be reactive, declare it without `$state`
  * @returns {never}

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

@@ -118,7 +118,7 @@ export function proxy(value, parent = null, prev) {
 
 			// create a source, but only if it's an own property and not a prototype property
 			if (s === undefined && (!exists || get_descriptor(target, prop)?.writable)) {
-				s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata));
+				s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata), null);
 				sources.set(prop, s);
 			}
 
@@ -170,7 +170,7 @@ export function proxy(value, parent = null, prev) {
 				(current_effect !== null && (!has || get_descriptor(target, prop)?.writable))
 			) {
 				if (s === undefined) {
-					s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED);
+					s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED, null);
 					sources.set(prop, s);
 				}
 

packages/svelte/src/internal/client/reactivity/sources.js

@@ -1,4 +1,4 @@
-/** @import { Derived, Effect, Source, Value } from '#client' */
+/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */
 import { DEV } from 'esm-env';
 import {
 	current_component_context,
@@ -13,7 +13,9 @@ import {
 	set_signal_status,
 	untrack,
 	increment_version,
-	update_effect
+	update_effect,
+	derived_sources,
+	set_derived_sources
 } from '../runtime.js';
 import { equals, safe_equals } from './equality.js';
 import {
@@ -32,27 +34,39 @@ let inspect_effects = new Set();
 /**
  * @template V
  * @param {V} v
+ * @param {Reaction | null} [owner]
  * @returns {Source<V>}
  */
 /*#__NO_SIDE_EFFECTS__*/
-export function source(v) {
-	return {
+export function source(v, owner = current_reaction) {
+	var source = {
 		f: 0, // TODO ideally we could skip this altogether, but it causes type errors
 		v,
 		reactions: null,
 		equals,
 		version: 0
 	};
+
+	if (owner !== null && (owner.f & DERIVED) !== 0) {
+		if (derived_sources === null) {
+			set_derived_sources([source]);
+		} else {
+			derived_sources.push(source);
+		}
+	}
+
+	return source;
 }
 
 /**
  * @template V
  * @param {V} initial_value
+ * @param {Reaction | null} [owner]
  * @returns {Source<V>}
  */
 /*#__NO_SIDE_EFFECTS__*/
-export function mutable_source(initial_value) {
-	const s = source(initial_value);
+export function mutable_source(initial_value, owner) {
+	const s = source(initial_value, owner);
 	s.equals = safe_equals;
 
 	// bind the signal to the component context, in case we need to
@@ -84,7 +98,14 @@ export function mutate(source, value) {
  * @returns {V}
  */
 export function set(source, value) {
-	if (current_reaction !== null && is_runes() && (current_reaction.f & DERIVED) !== 0) {
+	if (
+		current_reaction !== null &&
+		is_runes() &&
+		(current_reaction.f & DERIVED) !== 0 &&
+		// If the source was created locally within the current derived, then
+		// we allow the mutation.
+		(derived_sources === null || !derived_sources.includes(source))
+	) {
 		e.state_unsafe_mutation();
 	}
 

packages/svelte/src/internal/client/reactivity/store.js

@@ -19,7 +19,7 @@ import { mutable_source, set } from './sources.js';
 export function store_get(store, store_name, stores) {
 	const entry = (stores[store_name] ??= {
 		store: null,
-		source: mutable_source(undefined),
+		source: mutable_source(undefined, null),
 		unsubscribe: noop
 	});
 

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

@@ -80,6 +80,20 @@ export function set_current_effect(effect) {
 	current_effect = effect;
 }
 
+/**
+ * When sources are created within a derived, we record them so that we can safely allow
+ * local mutations to these sources without the side-effect error being invoked unnecessarily.
+ * @type {null | Source[]}
+ */
+export let derived_sources = null;
+
+/**
+ * @param {Source[] | null} sources
+ */
+export function set_derived_sources(sources) {
+	derived_sources = sources;
+}
+
 /**
  * The dependencies of the reaction that is currently being executed. In many cases,
  * the dependencies are unchanged between runs, and so this will be `null` unless
@@ -281,12 +295,14 @@ export function update_reaction(reaction) {
 	var previous_untracked_writes = current_untracked_writes;
 	var previous_reaction = current_reaction;
 	var previous_skip_reaction = current_skip_reaction;
+	var prev_derived_sources = derived_sources;
 
 	new_deps = /** @type {null | Value[]} */ (null);
 	skipped_deps = 0;
 	current_untracked_writes = null;
 	current_reaction = (reaction.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
 	current_skip_reaction = !is_flushing_effect && (reaction.f & UNOWNED) !== 0;
+	derived_sources = null;
 
 	try {
 		var result = /** @type {Function} */ (0, reaction.fn)();
@@ -323,6 +339,7 @@ export function update_reaction(reaction) {
 		current_untracked_writes = previous_untracked_writes;
 		current_reaction = previous_reaction;
 		current_skip_reaction = previous_skip_reaction;
+		derived_sources = prev_derived_sources;
 	}
 }
 
@@ -700,6 +717,9 @@ export function get(signal) {
 
 	// Register the dependency on the current reaction signal.
 	if (current_reaction !== null) {
+		if (derived_sources !== null && derived_sources.includes(signal)) {
+			e.state_unsafe_local_read();
+		}
 		var deps = current_reaction.deps;
 
 		// If the signal is accessing the same dependencies in the same

packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js

@@ -0,0 +1,20 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	html: `<button>10</button>`,
+
+	test({ assert, target, logs }) {
+		const btn = target.querySelector('button');
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>11</button>`);
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>12</button>`);
+
+		assert.deepEqual(logs, [0, 10, 11, 12]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte

@@ -0,0 +1,16 @@
+<script>
+	class Counter {
+		count = $state(0);
+		constructor(initial) {
+			$effect.pre(() => {
+				console.log(this.count);
+			});
+			this.count = initial;
+		}
+	}
+	const counter = $derived(new Counter(10));
+
+	counter;
+</script>
+
+<button onclick={() => counter.count++}>{counter.count}</button>

packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js

@@ -0,0 +1,20 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	html: `<button>10</button>`,
+
+	test({ assert, target, logs }) {
+		const btn = target.querySelector('button');
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>11</button>`);
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>12</button>`);
+
+		assert.deepEqual(logs, [0, 10, 11, 12]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte

@@ -0,0 +1,14 @@
+<script>
+	class Counter {
+		count = $state(0);
+		constructor(initial) {
+			$effect.pre(() => {
+				console.log(this.count);
+			});
+			this.count = initial;
+		}
+	}
+	const counter = new Counter(10);
+</script>
+
+<button onclick={() => counter.count++}>{counter.count}</button>

packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js

@@ -0,0 +1,20 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	html: `<button>10</button>`,
+
+	test({ assert, target, logs }) {
+		const btn = target.querySelector('button');
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>11</button>`);
+
+		btn?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, `<button>12</button>`);
+
+		assert.deepEqual(logs, [0, 10, 11, 12]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte

@@ -0,0 +1,20 @@
+<script>
+	class Base {
+		count = $state(0);
+	}
+
+	class Counter extends Base {
+		constructor(initial) {
+			super();
+			$effect.pre(() => {
+				console.log(this.count);
+			});
+			this.count = initial;
+		}
+	}
+	const counter = $derived(new Counter(10));
+
+	counter;
+</script>
+
+<button onclick={() => counter.count++}>{counter.count}</button>

packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js

@@ -18,7 +18,7 @@ export default function Class_state_field_constructor_assignment($$anchor, $$pro
 		#b = $.source();
 
 		constructor() {
-			this.#a.v = 1;
+			this.a = 1;
 			this.#b.v = 2;
 		}
 	}