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

trueadm · Rich-Harris · web-flow · commit 975918c60299 · 2024-08-24T11:48:49.000-04:00
* 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 &lt;rich.harris@vercel.com&gt;
diff --git a/.changeset/clean-shirts-yawn.md b/.changeset/clean-shirts-yawn.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+breaking: throw error if derived creates state and then depends on it
diff --git a/.changeset/five-maps-reflect.md b/.changeset/five-maps-reflect.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure assignments to state field inside constructor trigger effects
diff --git a/packages/svelte/messages/client-errors/errors.md b/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`
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js b/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)
-				);
-			}
 		}
 	}
 
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js b/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();
diff --git a/packages/svelte/src/internal/client/errors.js b/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}
diff --git a/packages/svelte/src/internal/client/proxy.js b/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);
 				}
 
diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/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();
 	}
 
diff --git a/packages/svelte/src/internal/client/reactivity/store.js b/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
 	});
 
diff --git a/packages/svelte/src/internal/client/runtime.js b/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
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js b/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]);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte b/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>
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js b/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]);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte b/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>
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js b/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]);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte b/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>
diff --git a/packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js b/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;
 		}
 	}

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'svelte': patch
 +---
++
 +breaking: throw error if derived creates state and then depends on it
Original file line number	Diff line number	Diff line change
`@@ -57,20 +57,6 @@ function build_assignment(operator, left, right, context) {`
`57`	`57`	`return b.assignment(operator, /** @type {Pattern} */ (context.visit(left)), value);`
`58`	`58`	`}`
`59`	`59`	`}`
`60`		`- } else if (left.property.type === 'Identifier' && context.state.in_constructor) {`
`61`		`- const public_state = context.state.public_state.get(left.property.name);`
`62`		`-`
`63`		`- if (public_state !== undefined && should_proxy(right, context.state.scope)) {`
`64`		`- const value = /** @type {Expression} */ (context.visit(right));`
`65`		`-`
`66`		`- return b.assignment(`
`67`		`- operator,`
`68`		`- /** @type {Pattern} */ (context.visit(left)),`
`69`		`- public_state.kind === 'raw_state'`
`70`		`- ? value`
`71`		`- : build_proxy_reassignment(value, public_state.id)`
`72`		`- );`
`73`		`- }`
`74`	`60`	`}`
`75`	`61`	`}`
`76`	`62`
Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@ export function proxy(value, parent = null, prev) {`
`118`	`118`
`119`	`119`	`// create a source, but only if it's an own property and not a prototype property`
`120`	`120`	`if (s === undefined && (!exists \|\| get_descriptor(target, prop)?.writable)) {`
`121`		`- s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata));`
	`121`	`+ s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata), null);`
`122`	`122`	`sources.set(prop, s);`
`123`	`123`	`}`
`124`	`124`
`@@ -170,7 +170,7 @@ export function proxy(value, parent = null, prev) {`
`170`	`170`	`(current_effect !== null && (!has \|\| get_descriptor(target, prop)?.writable))`
`171`	`171`	`) {`
`172`	`172`	`if (s === undefined) {`
`173`		`- s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED);`
	`173`	`+ s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED, null);`
`174`	`174`	`sources.set(prop, s);`
`175`	`175`	`}`
`176`	`176`