Merge 491fe89bf1 into ce89035ecb

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

@@ -13,6 +13,7 @@ import {
 	PROPS_IS_UPDATED,
 	PROPS_IS_BINDABLE
 } from '../../../../constants.js';
+import { get_rune } from '../../scope.js';
 
 /**
  * @param {Binding} binding
@@ -44,6 +45,185 @@ export function build_getter(node, state) {
 	return node;
 }
 
+/**
+ * @param {Binding} binding
+ * @param {Identifier} node
+ * @param {ClientTransformState} state
+ */
+export function prop_read_may_be_in_teardown(binding, node, state) {
+	const reference = binding.references.find((reference) => reference.node === node);
+	if (reference === undefined) return false;
+
+	return reference_may_be_in_teardown(reference, state, new Set());
+}
+
+/**
+ * @param {Binding['references'][number]} reference
+ * @param {ClientTransformState} state
+ * @param {Set<Binding>} checked
+ */
+function reference_may_be_in_teardown(reference, state, checked) {
+	const { path } = reference;
+
+	for (let i = path.length - 1; i >= 0; i -= 1) {
+		const fn = path[i];
+
+		if (
+			fn.type !== 'ArrowFunctionExpression' &&
+			fn.type !== 'FunctionExpression' &&
+			fn.type !== 'FunctionDeclaration'
+		) {
+			continue;
+		}
+
+		const parent = path[i - 1];
+
+		if (parent?.type === 'CallExpression') {
+			// An IIFE executes in the same context as the function containing it.
+			if (parent.callee === fn) continue;
+
+			if (parent.arguments.includes(/** @type {Expression} */ (fn))) {
+				const rune = get_rune(parent, get_scope(path, i - 1, state));
+
+				if (is_effect_rune(rune)) return false;
+				if (rune === '$derived.by') {
+					return derived_may_be_read_in_teardown(parent, path, i - 1, state, checked);
+				}
+			}
+		}
+
+		const function_binding = get_function_binding(fn, path, i, state);
+		return function_binding === null
+			? true
+			: function_may_be_called_in_teardown(function_binding, state, checked);
+	}
+
+	for (let i = path.length - 1; i >= 0; i -= 1) {
+		const node = path[i];
+		if (node.type !== 'CallExpression') continue;
+
+		const rune = get_rune(node, get_scope(path, i, state));
+		if (rune === '$derived' || rune === '$derived.by') {
+			return derived_may_be_read_in_teardown(node, path, i, state, checked);
+		}
+	}
+
+	return false;
+}
+
+/**
+ * @param {Binding} binding
+ * @param {ClientTransformState} state
+ * @param {Set<Binding>} checked
+ */
+function function_may_be_called_in_teardown(binding, state, checked) {
+	if (checked.has(binding)) return false;
+	checked.add(binding);
+
+	for (const reference of binding.references) {
+		if (reference.node === binding.node) continue;
+
+		const parent = reference.path.at(-1);
+
+		if (parent?.type !== 'CallExpression') return true;
+
+		if (parent.callee === reference.node) {
+			if (reference_may_be_in_teardown(reference, state, checked)) return true;
+			continue;
+		}
+
+		if (!parent.arguments.includes(reference.node)) return true;
+
+		const rune = get_rune(parent, get_scope(reference.path, reference.path.length - 1, state));
+
+		if (is_effect_rune(rune)) continue;
+		if (
+			rune === '$derived.by' &&
+			!derived_may_be_read_in_teardown(
+				parent,
+				reference.path,
+				reference.path.length - 1,
+				state,
+				checked
+			)
+		) {
+			continue;
+		}
+
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * @param {import('estree').CallExpression} call
+ * @param {import('#compiler').AST.SvelteNode[]} path
+ * @param {number} index
+ * @param {ClientTransformState} state
+ * @param {Set<Binding>} checked
+ */
+function derived_may_be_read_in_teardown(call, path, index, state, checked) {
+	const parent = path[index - 1];
+	if (parent?.type !== 'VariableDeclarator' || parent.init !== call) return true;
+	if (parent.id.type !== 'Identifier') return true;
+
+	const binding = get_scope(path, index - 1, state).get(parent.id.name);
+	if (binding === null || checked.has(binding)) return false;
+
+	checked.add(binding);
+
+	for (const reference of binding.references) {
+		if (reference.node === binding.node) continue;
+		if (reference_may_be_in_teardown(reference, state, checked)) return true;
+	}
+
+	return false;
+}
+
+/**
+ * @param {import('estree').FunctionDeclaration | import('estree').FunctionExpression | import('estree').ArrowFunctionExpression} fn
+ * @param {import('#compiler').AST.SvelteNode[]} path
+ * @param {number} index
+ * @param {ClientTransformState} state
+ * @returns {Binding | null}
+ */
+function get_function_binding(fn, path, index, state) {
+	if (fn.type === 'FunctionDeclaration' && fn.id !== null) {
+		return get_scope(path, index - 1, state).get(fn.id.name);
+	}
+
+	const parent = path[index - 1];
+	if (
+		parent?.type === 'VariableDeclarator' &&
+		parent.init === fn &&
+		parent.id.type === 'Identifier'
+	) {
+		return get_scope(path, index - 1, state).get(parent.id.name);
+	}
+
+	return null;
+}
+
+/**
+ * @param {import('#compiler').AST.SvelteNode[]} path
+ * @param {number} index
+ * @param {ClientTransformState} state
+ */
+function get_scope(path, index, state) {
+	for (let i = index; i >= 0; i -= 1) {
+		const scope = state.scopes.get(path[i]);
+		if (scope !== undefined) return scope;
+	}
+
+	return state.scope;
+}
+
+/** @param {string | null} rune */
+function is_effect_rune(rune) {
+	return rune === '$effect' || rune === '$effect.pre' || rune === '$effect.root';
+}
+
 /**
  * @param {Binding} binding
  * @param {ComponentClientTransformState} state

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

@@ -2,7 +2,7 @@
 /** @import { Context } from '../types' */
 import is_reference from 'is-reference';
 import * as b from '#compiler/builders';
-import { build_getter } from '../utils.js';
+import { build_getter, is_prop_source, prop_read_may_be_in_teardown } from '../utils.js';
 
 /**
  * @param {Identifier} node
@@ -16,28 +16,20 @@ 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
+			context.state.is_instance &&
 			binding !== null &&
 			node !== binding.node &&
-			binding.kind === 'rest_prop'
+			(binding.kind === 'prop' || binding.kind === 'bindable_prop') &&
+			!is_prop_source(binding, context.state) &&
+			prop_read_may_be_in_teardown(binding, node, context.state)
 		) {
-			const grand_parent = context.path.at(-2);
+			return b.call(
-
+				'$.get_prop_value',
-			if (
+				b.id('$$props'),
-				parent?.type === 'MemberExpression' &&
+				b.literal(binding.prop_alias ?? node.name)
-				!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

@@ -1,12 +1,36 @@
 /** @import { MemberExpression } from 'estree' */
 /** @import { Context } from '../types' */
 import * as b from '#compiler/builders';
+import { prop_read_may_be_in_teardown } from '../utils.js';
 
 /**
  * @param {MemberExpression} node
  * @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 &&
+				prop_read_may_be_in_teardown(binding, node.object, context.state)
+				? 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/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 = () => {
+				with_old_values(() => {
 					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

@@ -67,9 +67,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} */
@@ -656,7 +698,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);
 	}
 
@@ -665,6 +712,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)
@@ -675,7 +723,11 @@ export function get(signal) {
 				value = execute_derived(derived);
 			}
 
+			// 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;
 		}
@@ -755,6 +807,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,10 @@
+<script>
+	let { track, value, ...rest } = $props();
+	let indirect = $derived(rest.other);
+
+	$effect(() => {
+		track;
+
+		return () => console.log(`value = ${value}, other = ${indirect}`);
+	});
+</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-teardown-optimization/_expected/client/index.svelte.js

@@ -0,0 +1,25 @@
+import 'svelte/internal/disclose-version';
+import * as $ from 'svelte/internal/client';
+
+export default function Props_teardown_optimization($$anchor, $$props) {
+	$.push($$props, true);
+
+	let doubled = $.derived(() => $$props.derived_prop * 2);
+	let indirect = $.derived(() => $.get_prop_value($$props, 'indirect_prop'));
+	let read_function_prop = () => $$props.function_prop;
+	let read_function_cleanup_prop = () => $.get_prop_value($$props, 'function_cleanup_prop');
+
+	$.user_effect(() => console.log($$props.effect_prop));
+	$.user_effect(() => console.log(read_function_prop()));
+	$.user_effect(() => () => console.log($.get_prop_value($$props, 'cleanup_prop')));
+	$.user_effect(() => () => console.log($.get(indirect)));
+	$.user_effect(() => () => console.log(read_function_cleanup_prop()));
+	someFunction(() => $.get_prop_value($$props, 'unknown_prop'));
+	$.next();
+
+	var text = $.text();
+
+	$.template_effect(() => $.set_text(text, $.get(doubled)));
+	$.append($$anchor, text);
+	$.pop();
+}

packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/server/index.svelte.js

@@ -0,0 +1,23 @@
+import * as $ from 'svelte/internal/server';
+
+export default function Props_teardown_optimization($$renderer, $$props) {
+	$$renderer.component(($$renderer) => {
+		let {
+			derived_prop,
+			indirect_prop,
+			function_prop,
+			function_cleanup_prop,
+			effect_prop,
+			unknown_prop,
+			cleanup_prop
+		} = $$props;
+
+		let doubled = $.derived(() => derived_prop * 2);
+		let indirect = $.derived(() => indirect_prop);
+		let read_function_prop = () => function_prop;
+		let read_function_cleanup_prop = () => function_cleanup_prop;
+
+		someFunction(() => unknown_prop);
+		$$renderer.push(`<!---->${$.escape(doubled())}`);
+	});
+}

packages/svelte/tests/snapshot/samples/props-teardown-optimization/index.svelte

@@ -0,0 +1,26 @@
+<script>
+	let {
+		derived_prop,
+		indirect_prop,
+		function_prop,
+		function_cleanup_prop,
+		effect_prop,
+		unknown_prop,
+		cleanup_prop
+	} = $props();
+
+	let doubled = $derived(derived_prop * 2);
+	let indirect = $derived(indirect_prop);
+	let read_function_prop = () => function_prop;
+	let read_function_cleanup_prop = () => function_cleanup_prop;
+
+	$effect(() => console.log(effect_prop));
+	$effect(() => console.log(read_function_prop()));
+	$effect(() => () => console.log(cleanup_prop));
+	$effect(() => () => console.log(indirect));
+	$effect(() => () => console.log(read_function_cleanup_prop()));
+
+	someFunction(() => unknown_prop);
+</script>
+
+{doubled}