fix: compute teardown values more correctly

packages/svelte/src/compiler/phases/3-transform/client/types.d.ts

@@ -26,6 +26,8 @@ export interface ClientTransformState extends TransformState {
 		{
 			/** turn `foo` into e.g. `$.get(foo)` */
 			read: (id: Identifier) => Expression;
+			/** turn `foo` into e.g. `$$props.foo` inside the template */
+			read_template?: (id: Identifier) => Expression;
 			/** turn `foo = bar` into e.g. `$.set(foo, bar)` */
 			assign?: (node: Identifier, value: Expression, proxy?: boolean) => Expression;
 			/** turn `foo.bar = baz` into e.g. `$.mutate(foo, $.get(foo).bar = baz);` */

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

@@ -37,7 +37,10 @@ export function build_getter(node, state) {
 
 		// don't transform the declaration itself
 		if (node !== binding?.node) {
-			return state.transform[node.name].read(node);
+			var transform = state.transform[node.name];
+			return state.is_instance || transform.read_template === undefined
+				? transform.read(node)
+				: transform.read_template(node);
 		}
 	}
 

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

@@ -16,30 +16,6 @@ export function Identifier(node, context) {
 			return b.id('$$sanitized_props');
 		}
 
-		// Optimize prop access: If it's a member read access, we can use the $$props object directly
-		const binding = context.state.scope.get(node.name);
-		if (
-			context.state.analysis.runes && // can't do this in legacy mode because the proxy does more than just read/write
-			binding !== null &&
-			node !== binding.node &&
-			binding.kind === 'rest_prop'
-		) {
-			const grand_parent = context.path.at(-2);
-
-			if (
-				parent?.type === 'MemberExpression' &&
-				!parent.computed &&
-				grand_parent?.type !== 'AssignmentExpression' &&
-				grand_parent?.type !== 'UpdateExpression'
-			) {
-				const key = /** @type {Identifier} */ (parent.property);
-
-				if (!binding.metadata?.exclude_props?.includes(key.name)) {
-					return b.id('$$props');
-				}
-			}
-		}
-
 		return build_getter(node, context.state);
 	}
 }

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

@@ -7,6 +7,28 @@ import * as b from '#compiler/builders';
  * @param {Context} context
  */
 export function MemberExpression(node, context) {
+	if (
+		context.state.analysis.runes &&
+		node.object.type === 'Identifier' &&
+		node.property.type === 'Identifier' &&
+		!node.computed
+	) {
+		const binding = context.state.scope.get(node.object.name);
+		const parent = context.path.at(-1);
+
+		if (
+			binding?.kind === 'rest_prop' &&
+			node.object !== binding.node &&
+			parent?.type !== 'AssignmentExpression' &&
+			parent?.type !== 'UpdateExpression' &&
+			!binding.metadata?.exclude_props?.includes(node.property.name)
+		) {
+			return context.state.is_instance
+				? b.call('$.get_prop_value', b.id('$$props'), b.literal(node.property.name))
+				: b.member(b.id('$$props'), node.property);
+		}
+	}
+
 	// rewrite `this.#foo` as `this.#foo.v` inside a constructor
 	if (node.property.type === 'PrivateIdentifier') {
 		const field = context.state.state_fields.get('#' + node.property.name);

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

@@ -126,11 +126,13 @@ export function Program(node, context) {
 				const key = b.key(binding.prop_alias);
 
 				context.state.transform[name] = {
-					read: (_) => b.member(b.id('$$props'), key, key.type === 'Literal')
+					read: (_) => b.call('$.get_prop_value', b.id('$$props'), b.literal(binding.prop_alias)),
+					read_template: (_) => b.member(b.id('$$props'), key, key.type === 'Literal')
 				};
 			} else {
 				context.state.transform[name] = {
-					read: (node) => b.member(b.id('$$props'), node)
+					read: (_) => b.call('$.get_prop_value', b.id('$$props'), b.literal(name)),
+					read_template: (node) => b.member(b.id('$$props'), node)
 				};
 			}
 		}

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

@@ -63,7 +63,11 @@ export function SnippetBlock(node, context) {
 			// we need to eagerly evaluate the expression in order to hit any
 			// 'Cannot access x before initialization' errors
 			if (dev) {
-				declarations.push(b.stmt(transform[name].read(b.id(name))));
+				var read =
+					context.state.is_instance || transform[name].read_template === undefined
+						? transform[name].read
+						: transform[name].read_template;
+				declarations.push(b.stmt(read(b.id(name))));
 			}
 		}
 	}

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

@@ -416,7 +416,11 @@ export function validate_mutation(node, context, expression) {
 				? context.state.transform[left.property.name]
 				: null;
 			if (left.computed) {
-				path.unshift(transform?.read ? transform.read(left.property) : left.property);
+				var read =
+					state.is_instance || transform?.read_template === undefined
+						? transform?.read
+						: transform.read_template;
+				path.unshift(read ? read(left.property) : left.property);
 			} else {
 				path.unshift(b.literal(left.property.name));
 			}

packages/svelte/src/internal/client/dom/elements/bindings/this.js

@@ -2,7 +2,7 @@
 import { DESTROYING, STATE_SYMBOL } from '#client/constants';
 import { component_context, mark_as_component } from '../../../context.js';
 import { effect, render_effect } from '../../../reactivity/effects.js';
-import { active_effect, untrack } from '../../../runtime.js';
+import { active_effect, untrack, with_old_values } from '../../../runtime.js';
 
 /**
  * @param {any} bound_value
@@ -67,9 +67,11 @@ export function bind_this(
 				p = p.parent;
 			}
 			const teardown = () => {
-				if (parts && is_bound_this(get_value(...parts), element_or_component)) {
+				with_old_values(() => {
-					update(null, ...parts);
+					if (parts && is_bound_this(get_value(...parts), element_or_component)) {
-				}
+						update(null, ...parts);
+					}
+				});
 			};
 			const original_teardown = p.teardown;
 			p.teardown = () => {

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

@@ -156,6 +156,7 @@ export { invalidate_inner_signals } from './legacy.js';
 export { set_text } from './render.js';
 export {
 	get,
+	get_prop_value,
 	safe_get,
 	tick,
 	untrack,

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

@@ -3,12 +3,13 @@ import {
 	is_dirty,
 	active_effect,
 	active_reaction,
+	destroying_effect,
 	update_effect,
 	get,
 	is_destroying_effect,
 	remove_reactions,
 	set_active_reaction,
-	set_is_destroying_effect,
+	set_destroying_effect,
 	untrack,
 	untracking,
 	set_active_effect
@@ -445,9 +446,9 @@ export function branch(fn) {
 export function execute_effect_teardown(effect) {
 	var teardown = effect.teardown;
 	if (teardown !== null) {
-		const previously_destroying_effect = is_destroying_effect;
+		const previous_destroying_effect = destroying_effect;
 		const previous_reaction = active_reaction;
-		set_is_destroying_effect(true);
+		set_destroying_effect(effect);
 		set_active_reaction(null);
 		try {
 			teardown.call(null);
@@ -457,7 +458,7 @@ export function execute_effect_teardown(effect) {
 			// themselves mid-teardown are skipped by invoke_error_boundary.
 			invoke_error_boundary(error, effect.parent);
 		} finally {
-			set_is_destroying_effect(previously_destroying_effect);
+			set_destroying_effect(previous_destroying_effect);
 			set_active_reaction(previous_reaction);
 		}
 	}

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

@@ -15,7 +15,8 @@ import {
 	get,
 	is_destroying_effect,
 	set_active_effect,
-	untrack
+	untrack,
+	with_old_values
 } from '../runtime.js';
 import * as e from '../errors.js';
 import { DESTROYED, LEGACY_PROPS, STATE_SYMBOL } from '#client/constants';
@@ -54,7 +55,7 @@ export function update_pre_prop(fn, d = 1) {
 const rest_props_handler = {
 	get(target, key) {
 		if (target.exclude.has(key)) return;
-		return target.props[key];
+		return with_old_values(() => target.props[key]);
 	},
 	set(target, key) {
 		if (DEV) {
@@ -70,7 +71,7 @@ const rest_props_handler = {
 			return {
 				enumerable: true,
 				configurable: true,
-				value: target.props[key]
+				value: with_old_values(() => target.props[key])
 			};
 		}
 	},
@@ -359,7 +360,7 @@ export function prop(props, key, flags, fallback) {
 
 	// prop is never written to — we only need a getter
 	if (runes && (flags & PROPS_IS_UPDATED) === 0) {
-		return getter;
+		return () => with_old_values(getter);
 	}
 
 	// prop is written to, but the parent component had `bind:foo` which
@@ -380,7 +381,7 @@ export function prop(props, key, flags, fallback) {
 					return value;
 				}
 
-				return getter();
+				return with_old_values(getter);
 			}
 		);
 	}
@@ -404,6 +405,7 @@ export function prop(props, key, flags, fallback) {
 	if (bindable) get(d);
 
 	var parent_effect = /** @type {Effect} */ (active_effect);
+	var get_derived = () => get(d);
 
 	return /** @type {() => V} */ (
 		function (/** @type {any} */ value, /** @type {boolean} */ mutation) {
@@ -427,7 +429,7 @@ export function prop(props, key, flags, fallback) {
 				return d.v;
 			}
 
-			return get(d);
+			return with_old_values(get_derived);
 		}
 	);
 }

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

@@ -68,9 +68,51 @@ let is_updating_effect = false;
 
 export let is_destroying_effect = false;
 
-/** @param {boolean} value */
+/** @type {Effect | null} */
-export function set_is_destroying_effect(value) {
+export let destroying_effect = null;
-	is_destroying_effect = value;
+
+let is_reading_old_value = false;
+let old_value_read_version = 0;
+
+/** @param {Effect | null} effect */
+export function set_destroying_effect(effect) {
+	destroying_effect = effect;
+	is_destroying_effect = effect !== null;
+}
+
+/**
+ * @template V
+ * @param {() => V} fn
+ * @returns {V}
+ */
+export function with_old_values(fn) {
+	if (!is_destroying_effect) return fn();
+
+	var previous_is_reading_old_value = is_reading_old_value;
+	is_reading_old_value = true;
+
+	try {
+		return fn();
+	} finally {
+		is_reading_old_value = previous_is_reading_old_value;
+	}
+}
+
+/**
+ * @param {Record<string, any>} props
+ * @param {string} key
+ */
+export function get_prop_value(props, key) {
+	if (!is_destroying_effect) return props[key];
+
+	var previous_is_reading_old_value = is_reading_old_value;
+	is_reading_old_value = true;
+
+	try {
+		return props[key];
+	} finally {
+		is_reading_old_value = previous_is_reading_old_value;
+	}
 }
 
 /** @type {null | Reaction} */
@@ -660,7 +702,12 @@ export function get(signal) {
 		}
 	}
 
-	if (is_destroying_effect && old_values.has(signal)) {
+	if (
+		is_destroying_effect &&
+		old_values.has(signal) &&
+		(is_reading_old_value || reaction_depends_on(/** @type {Effect} */ (destroying_effect), signal))
+	) {
+		old_value_read_version += 1;
 		return old_values.get(signal);
 	}
 
@@ -669,6 +716,7 @@ export function get(signal) {
 
 		if (is_destroying_effect) {
 			var value = derived.v;
+			var previous_old_value_read_version = old_value_read_version;
 
 			// if the derived is dirty and has reactions, or depends on the values that just changed, re-execute
 			// (a derived can be maybe_dirty due to the effect destroy removing its last reaction)
@@ -679,7 +727,11 @@ export function get(signal) {
 				value = execute_derived(derived);
 			}
 
-			old_values.set(derived, value);
+			// Don't let a current value calculated for one teardown become the old value
+			// observed by a later teardown in the same flush.
+			if (old_value_read_version !== previous_old_value_read_version) {
+				old_values.set(derived, value);
+			}
 
 			return value;
 		}
@@ -759,6 +811,30 @@ function depends_on_old_values(derived) {
 	return false;
 }
 
+/**
+ * @param {Reaction} reaction
+ * @param {Value} signal
+ * @param {Set<Reaction>} checked
+ */
+function reaction_depends_on(reaction, signal, checked = new Set()) {
+	if (reaction.deps === null || checked.has(reaction)) return false;
+
+	checked.add(reaction);
+
+	for (const dep of reaction.deps) {
+		if (dep === signal) return true;
+
+		if (
+			(dep.f & DERIVED) !== 0 &&
+			reaction_depends_on(/** @type {Derived} */ (dep), signal, checked)
+		) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
 /**
  * Like `get`, but checks for `undefined`. Used for `var` declarations because they can be accessed before being declared
  * @template V

packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/Child.svelte

@@ -0,0 +1,9 @@
+<script>
+	let { track, value, ...rest } = $props();
+
+	$effect(() => {
+		track;
+
+		return () => console.log(`value = ${value}, other = ${rest.other}`);
+	});
+</script>

packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/_config.js

@@ -0,0 +1,19 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	compileOptions: {
+		accessors: false
+	},
+	test({ assert, target, logs }) {
+		const button = target.querySelector('button');
+		flushSync(() => button?.click());
+
+		assert.deepEqual(logs, [
+			'value = true, other = true',
+			'track = 0, thing1 = false, thing2 = false, derived = false',
+			'tracked derived = true',
+			'tracked derived = false'
+		]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/main.svelte

@@ -0,0 +1,43 @@
+<script>
+	import Child from './Child.svelte';
+
+	let track = $state(0);
+	let value = $state(true);
+	let other = $state(true);
+	let thing1 = $state(true);
+	let thing2 = true;
+	let derived = $derived(thing1);
+
+	$effect(() => {
+		track;
+
+		return () =>
+			console.log(
+				`track = ${track}, thing1 = ${thing1}, thing2 = ${thing2}, derived = ${derived}`
+			);
+	});
+
+	$effect(() => {
+		derived;
+
+		return () => console.log(`tracked derived = ${derived}`);
+	});
+
+	$effect(() => {
+		track;
+
+		return () => console.log(`tracked derived = ${derived}`);
+	});
+</script>
+
+<button onclick={() => {
+	value = !value;
+	other = !other;
+	thing1 = !thing1;
+	thing2 = !thing2;
+	track++;
+}}>Re-run effect</button>
+
+{thing1} {thing2}
+
+<Child {track} {value} {other} />

packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js

@@ -10,6 +10,6 @@ export default test({
 		});
 
 		await Promise.resolve();
-		assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 0]);
+		assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 1]);
 	}
 });

packages/svelte/tests/snapshot/samples/props-identifier/_expected/client/index.svelte.js

@@ -8,10 +8,10 @@ export default function Props_identifier($$anchor, $$props) {
 
 	let props = $.rest_props($$props, rest_excludes);
 
-	$$props.a;
+	$.get_prop_value($$props, 'a');
 	props[a];
-	$$props.a.b;
+	$.get_prop_value($$props, 'a').b;
-	$$props.a.b = true;
+	$.get_prop_value($$props, 'a').b = true;
 	props.a = true;
 	props[a] = true;
 	props;