fix: make bind_this implementation more robust (#10598)

The previous `bind_this` implementation had flaws around timing and didn't work correctly when the binding wasn't `$state` in runes mode.
This PR reimplements `bind_this` by aligning it more with how Svelte 4 bindings work, namely reacting to each block context variables updates properly and introducing a mechanism to get the previous binding destination using said variables.

Author: dummdidumm

.changeset/moody-sheep-type.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: make `bind_this` implementation more robust

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

@@ -35,6 +35,7 @@ import {
 import { regex_is_valid_identifier } from '../../../patterns.js';
 import { javascript_visitors_runes } from './javascript-runes.js';
 import { sanitize_template_string } from '../../../../utils/sanitize_template_string.js';
+import { walk } from 'zimmerframe';
 
 /**
  * @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} element
@@ -978,24 +979,11 @@ function serialize_inline_component(node, component_name, context) {
 
 	if (bind_this !== null) {
 		const prev = fn;
-		const assignment = b.assignment('=', bind_this, b.id('$$value'));
-		const bind_this_id = /** @type {import('estree').Expression} */ (
-			// if expression is not an identifier, we know it can't be a signal
-			bind_this.type === 'Identifier'
-				? bind_this
-				: bind_this.type === 'MemberExpression' && bind_this.object.type === 'Identifier'
-					? bind_this.object
-					: undefined
-		);
 		fn = (node_id) =>
-			b.call(
-				'$.bind_this',
-				prev(node_id),
-				b.arrow(
-					[b.id('$$value')],
-					serialize_set_binding(assignment, context, () => context.visit(assignment))
-				),
-				bind_this_id
+			serialize_bind_this(
+				/** @type {import('estree').Identifier | import('estree').MemberExpression} */ (bind_this),
+				context,
+				prev(node_id)
 			);
 	}
 
@@ -1022,6 +1010,66 @@ function serialize_inline_component(node, component_name, context) {
 	return statements.length > 1 ? b.block(statements) : statements[0];
 }
 
+/**
+ * Serializes `bind:this` for components and elements.
+ * @param {import('estree').Identifier | import('estree').MemberExpression} bind_this
+ * @param {import('zimmerframe').Context<import('#compiler').SvelteNode, import('../types.js').ComponentClientTransformState>} context
+ * @param {import('estree').Expression} node
+ * @returns
+ */
+function serialize_bind_this(bind_this, context, node) {
+	let i = 0;
+	/** @type {Map<import('#compiler').Binding, [arg_idx: number, transformed: import('estree').Expression, expression: import('#compiler').Binding['expression']]>} */
+	const each_ids = new Map();
+	// Transform each reference to an each block context variable into a $$value_<i> variable
+	// by temporarily changing the `expression` of the corresponding binding.
+	// These $$value_<i> variables will be filled in by the bind_this runtime function through its last argument.
+	// Note that we only do this for each context variables, the consequence is that the value might be stale in
+	// some scenarios where the value is a member expression with changing computed parts or using a combination of multiple
+	// variables, but that was the same case in Svelte 4, too. Once legacy mode is gone completely, we can revisit this.
+	walk(
+		bind_this,
+		{},
+		{
+			Identifier(node) {
+				const binding = context.state.scope.get(node.name);
+				if (!binding || each_ids.has(binding)) return;
+
+				const associated_node = Array.from(context.state.scopes.entries()).find(
+					([_, scope]) => scope === binding?.scope
+				)?.[0];
+				if (associated_node?.type === 'EachBlock') {
+					each_ids.set(binding, [
+						i,
+						/** @type {import('estree').Expression} */ (context.visit(node)),
+						binding.expression
+					]);
+					binding.expression = b.id('$$value_' + i);
+					i++;
+				}
+			}
+		}
+	);
+
+	const bind_this_id = /** @type {import('estree').Expression} */ (context.visit(bind_this));
+	const ids = Array.from(each_ids.values()).map((id) => b.id('$$value_' + id[0]));
+	const assignment = b.assignment('=', bind_this, b.id('$$value'));
+	const update = serialize_set_binding(assignment, context, () => context.visit(assignment));
+
+	for (const [binding, [, , expression]] of each_ids) {
+		// reset expressions to what they were before
+		binding.expression = expression;
+	}
+
+	/** @type {import('estree').Expression[]} */
+	const args = [node, b.arrow([b.id('$$value'), ...ids], update), b.arrow([...ids], bind_this_id)];
+	if (each_ids.size) {
+		args.push(b.thunk(b.array(Array.from(each_ids.values()).map((id) => id[1]))));
+	}
+
+	return b.call('$.bind_this', ...args);
+}
+
 /**
  * Creates a new block which looks roughly like this:
  * ```js
@@ -2749,19 +2797,7 @@ export const template_visitors = {
 				}
 
 				case 'this':
-					call_expr = b.call(
-						`$.bind_this`,
-						state.node,
-						setter,
-						/** @type {import('estree').Expression} */ (
-							// if expression is not an identifier, we know it can't be a signal
-							expression.type === 'Identifier'
-								? expression
-								: expression.type === 'MemberExpression' && expression.object.type === 'Identifier'
-									? expression.object
-									: undefined
-						)
-					);
+					call_expr = serialize_bind_this(node.expression, context, state.node);
 					break;
 				case 'textContent':
 				case 'innerHTML':

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

@@ -36,7 +36,6 @@ import {
 } from './reconciler.js';
 import {
 	destroy_signal,
-	is_signal,
 	push_destroy_fn,
 	execute_effect,
 	untrack,
@@ -79,7 +78,6 @@ import { run } from '../common.js';
 import { bind_transition, trigger_transitions } from './transitions.js';
 import { mutable_source, source } from './reactivity/sources.js';
 import { safe_equal, safe_not_equal } from './reactivity/equality.js';
-import { STATE_SYMBOL } from './constants.js';
 
 /** @type {Set<string>} */
 const all_registerd_events = new Set();
@@ -1321,39 +1319,48 @@ export function bind_prop(props, prop, value) {
 	}
 }
 
-/**
- * @param {unknown} value
- */
-function is_state_object(value) {
-	return value != null && typeof value === 'object' && STATE_SYMBOL in value;
-}
-
 /**
  * @param {Element} element_or_component
- * @param {(value: unknown) => void} update
- * @param {import('./types.js').MaybeSignal} binding
+ * @param {(value: unknown, ...parts: unknown[]) => void} update
+ * @param {(...parts: unknown[]) => unknown} get_value
+ * @param {() => unknown[]} [get_parts] Set if the this binding is used inside an each block,
+ * 										returns all the parts of the each block context that are used in the expression
  * @returns {void}
  */
-export function bind_this(element_or_component, update, binding) {
-	render_effect(() => {
-		// If we are reading from a proxied state binding, then we don't need to untrack
-		// the update function as it will be fine-grain.
-		if (is_state_object(binding) || (is_signal(binding) && is_state_object(binding.v))) {
-			update(element_or_component);
-		} else {
-			untrack(() => update(element_or_component));
-		}
-		return () => {
-			// Defer to the next tick so that all updates can be reconciled first.
-			// This solves the case where one variable is shared across multiple this-bindings.
-			render_effect(() => {
-				untrack(() => {
-					if (!is_signal(binding) || binding.v === element_or_component) {
-						update(null);
-					}
-				});
-			});
-		};
+export function bind_this(element_or_component, update, get_value, get_parts) {
+	/** @type {unknown[]} */
+	let old_parts;
+	/** @type {unknown[]} */
+	let parts;
+
+	const e = effect(() => {
+		old_parts = parts;
+		// We only track changes to the parts, not the value itself to avoid unnecessary reruns.
+		parts = get_parts?.() || [];
+
+		untrack(() => {
+			if (element_or_component !== get_value(...parts)) {
+				update(element_or_component, ...parts);
+				// If this is an effect rerun (cause: each block context changes), then nullfiy the binding at
+				// the previous position if it isn't already taken over by a different effect.
+				if (old_parts && get_value(...old_parts) === element_or_component) {
+					update(null, ...old_parts);
+				}
+			}
+		});
+	});
+
+	// Add effect teardown (likely causes: if block became false, each item removed, component unmounted).
+	// In these cases we need to nullify the binding only if we detect that the value is still the same.
+	// If not, that means that another effect has now taken over the binding.
+	push_destroy_fn(e, () => {
+		// Defer to the next tick so that all updates can be reconciled first.
+		// This solves the case where one variable is shared across multiple this-bindings.
+		effect(() => {
+			if (get_value(...parts) === element_or_component) {
+				update(null, ...parts);
+			}
+		});
 	});
 }
 

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

@@ -434,6 +434,7 @@ export function execute_effect(signal) {
 
 function infinite_loop_guard() {
 	if (flush_count > 100) {
+		flush_count = 0;
 		throw new Error(
 			'ERR_SVELTE_TOO_MANY_UPDATES' +
 				(DEV

packages/svelte/tests/runtime-legacy/samples/await-then-catch-event/_config.js

@@ -1,3 +1,4 @@
+import { tick } from 'svelte';
 import { test } from '../../test';
 import { create_deferred } from '../../../helpers.js';
 
@@ -22,7 +23,8 @@ export default test({
 		deferred.resolve(42);
 
 		return deferred.promise
-			.then(() => {
+			.then(async () => {
+				await tick();
 				assert.htmlEqual(target.innerHTML, '<button>click me</button>');
 
 				const { button } = component;

packages/svelte/tests/runtime-legacy/samples/binding-this-with-context/_config.js

@@ -46,10 +46,8 @@ export default test({
 		component.items = ['foo', 'baz'];
 		assert.equal(component.divs.length, 3, 'the divs array is still 3 long');
 		assert.equal(component.divs[2], null, 'the last div is unregistered');
-		// Different from Svelte 3
-		assert.equal(component.ps[1], null, 'the second p is unregistered');
-		// Different from Svelte 3
-		assert.equal(component.spans['-baz2'], null, 'the baz span is unregistered');
+		assert.equal(component.ps[2], null, 'the last p is unregistered');
+		assert.equal(component.spans['-bar1'], null, 'the bar span is unregistered');
 		assert.equal(component.hrs.bar, null, 'the bar hr is unregistered');
 
 		divs = target.querySelectorAll('div');
@@ -58,22 +56,17 @@ export default test({
 			assert.equal(e, divs[i], `div ${i} is still correct`);
 		});
 
-		// TODO: unsure if need these two tests to pass, as the logic between Svelte 3
-		// and Svelte 5 is different for these cases.
-
-		// elems = target.querySelectorAll('span');
-		// component.items.forEach((e, i) => {
-		//   assert.equal(
-		//     component.spans[`-${e}${i}`],
-		//     elems[i],
-		//     `span -${e}${i} is still correct`,
-		//   );
-		// });
+		spans = target.querySelectorAll('span');
+		// @ts-ignore
+		component.items.forEach((e, i) => {
+			assert.equal(component.spans[`-${e}${i}`], spans[i], `span -${e}${i} is still correct`);
+		});
 
-		// elems = target.querySelectorAll('p');
-		// component.ps.forEach((e, i) => {
-		//   assert.equal(e, elems[i], `p ${i} is still correct`);
-		// });
+		ps = target.querySelectorAll('p');
+		// @ts-ignore
+		component.ps.forEach((e, i) => {
+			assert.equal(e, ps[i], `p ${i} is still correct`);
+		});
 
 		hrs = target.querySelectorAll('hr');
 		// @ts-ignore

packages/svelte/tests/runtime-legacy/samples/transition-js-if-block-outro-timeout/_config.js

@@ -1,3 +1,4 @@
+import { flushSync } from 'svelte';
 import { test } from '../../test';
 
 export default test({
@@ -15,7 +16,8 @@ export default test({
 		assert.equal(window.getComputedStyle(div).opacity, '0');
 
 		raf.tick(600);
-		assert.equal(component.div, undefined);
 		assert.equal(target.querySelector('div'), undefined);
+		flushSync();
+		assert.equal(component.div, undefined);
 	}
 });

packages/svelte/tests/runtime-runes/samples/bind-this-no-state/_config.js

@@ -0,0 +1,43 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+/** @param {number | null} selected */
+function get_html(selected) {
+	return `
+		<button>1</button>
+		<button>2</button>
+		<button>3</button>
+
+		<hr></hr>
+
+		${selected !== null ? `<div>${selected}</div>` : ''}
+
+		<hr></hr>
+
+		<p>${selected ?? '...'}</p>
+	`;
+}
+
+export default test({
+	html: get_html(null),
+
+	async test({ assert, target }) {
+		const [btn1, btn2, btn3] = target.querySelectorAll('button');
+
+		await btn1?.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, get_html(1));
+
+		await btn2?.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, get_html(2));
+
+		await btn1?.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, get_html(1));
+
+		await btn3?.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, get_html(3));
+	}
+});

packages/svelte/tests/runtime-runes/samples/bind-this-no-state/main.svelte

@@ -0,0 +1,30 @@
+<script>
+	import { tick } from 'svelte';
+	
+	let selected = $state(-1);
+	let current = $state();
+
+	let div; // explicitly no $state
+</script>
+
+{#each [1, 2, 3] as n, i}
+	<button
+		onclick={async () => {
+			selected = i;
+			await tick();
+			current = div?.textContent;
+		}}
+	>{n}</button>
+{/each}
+
+<hr />
+
+{#each [1, 2, 3] as n, i}
+	{#if selected === i}
+		<div bind:this={div}>{n}</div>
+	{/if}
+{/each}
+
+<hr />
+
+<p>{current ?? '...'}</p>

packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js

@@ -11,7 +11,7 @@ export default function Bind_this($$anchor, $$props) {
 	var fragment = $.comment($$anchor);
 	var node = $.child_frag(fragment);
 
-	$.bind_this(Foo(node, {}), ($$value) => foo = $$value, foo);
+	$.bind_this(Foo(node, {}), ($$value) => foo = $$value, () => foo);
 	$.close_frag($$anchor, fragment);
 	$.pop();
 }