fix: invoke parent boundary of deriveds that throw (#16091)

* remove unused handle_error arguments

* move error handling code into separate module

* tidy up

* simplify

* remove unnecessary handle_error invocation

* WIP

* WIP

* WIP

* note to self

* WIP

* WIP

* unused

* WIP

* turns out we need this

* fix

* all tests passing

* unused

* unused

* tidy up

* unused

* unused

* tweak

* WIP

* tweak

* asked and answered

* tweak

* unused

* changeset

* lint

Author: Rich-Harris

.changeset/early-tips-prove.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: invoke parent boundary of deriveds that throw

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

@@ -1,4 +1,4 @@
-/** @import { BlockStatement, Statement, Expression } from 'estree' */
+/** @import { BlockStatement, Statement, Expression, FunctionDeclaration, VariableDeclaration, ArrowFunctionExpression } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
 import { dev } from '../../../../state.js';
@@ -34,62 +34,61 @@ export function SvelteBoundary(node, context) {
 	const nodes = [];
 
 	/** @type {Statement[]} */
-	const external_statements = [];
+	const const_tags = [];
 
 	/** @type {Statement[]} */
-	const internal_statements = [];
+	const hoisted = [];
 
-	const snippets_visits = [];
+	// const tags need to live inside the boundary, but might also be referenced in hoisted snippets.
+	// to resolve this we cheat: we duplicate const tags inside snippets
+	for (const child of node.fragment.nodes) {
+		if (child.type === 'ConstTag') {
+			context.visit(child, { ...context.state, init: const_tags });
+		}
+	}
 
-	// Capture the `failed` implicit snippet prop
 	for (const child of node.fragment.nodes) {
-		if (child.type === 'SnippetBlock' && child.expression.name === 'failed') {
-			// we need to delay the visit of the snippets in case they access a ConstTag that is declared
-			// after the snippets so that the visitor for the const tag can be updated
-			snippets_visits.push(() => {
-				/** @type {Statement[]} */
-				const init = [];
-				context.visit(child, { ...context.state, init });
-				props.properties.push(b.prop('init', child.expression, child.expression));
-				external_statements.push(...init);
-			});
-		} else if (child.type === 'ConstTag') {
+		if (child.type === 'ConstTag') {
+			continue;
+		}
+
+		if (child.type === 'SnippetBlock') {
 			/** @type {Statement[]} */
-			const init = [];
-			context.visit(child, { ...context.state, init });
-
-			if (dev) {
-				// In dev we must separate the declarations from the code
-				// that eagerly evaluate the expression...
-				for (const statement of init) {
-					if (statement.type === 'VariableDeclaration') {
-						external_statements.push(statement);
-					} else {
-						internal_statements.push(statement);
-					}
-				}
-			} else {
-				external_statements.push(...init);
+			const statements = [];
+
+			context.visit(child, { ...context.state, init: statements });
+
+			const snippet = /** @type {VariableDeclaration} */ (statements[0]);
+
+			const snippet_fn = dev
+				? // @ts-expect-error we know this shape is correct
+					snippet.declarations[0].init.arguments[1]
+				: snippet.declarations[0].init;
+
+			snippet_fn.body.body.unshift(
+				...const_tags.filter((node) => node.type === 'VariableDeclaration')
+			);
+
+			hoisted.push(snippet);
+
+			if (child.expression.name === 'failed') {
+				props.properties.push(b.prop('init', child.expression, child.expression));
 			}
-		} else {
-			nodes.push(child);
+
+			continue;
 		}
-	}
 
-	snippets_visits.forEach((visit) => visit());
+		nodes.push(child);
+	}
 
 	const block = /** @type {BlockStatement} */ (context.visit({ ...node.fragment, nodes }));
 
-	if (dev && internal_statements.length) {
-		block.body.unshift(...internal_statements);
-	}
+	block.body.unshift(...const_tags);
 
 	const boundary = b.stmt(
 		b.call('$.boundary', context.state.node, props, b.arrow([b.id('$$anchor')], block))
 	);
 
 	context.state.template.push_comment();
-	context.state.init.push(
-		external_statements.length > 0 ? b.block([...external_statements, boundary]) : boundary
-	);
+	context.state.init.push(hoisted.length > 0 ? b.block([...hoisted, boundary]) : boundary);
 }

packages/svelte/src/internal/client/dom/blocks/boundary.js

@@ -2,14 +2,13 @@
 
 import { BOUNDARY_EFFECT, EFFECT_TRANSPARENT } from '#client/constants';
 import { component_context, set_component_context } from '../../context.js';
+import { invoke_error_boundary } from '../../error-handling.js';
 import { block, branch, destroy_effect, pause_effect } from '../../reactivity/effects.js';
 import {
 	active_effect,
 	active_reaction,
-	handle_error,
 	set_active_effect,
-	set_active_reaction,
-	reset_is_throwing_error
+	set_active_reaction
 } from '../../runtime.js';
 import {
 	hydrate_next,
@@ -80,11 +79,17 @@ export function boundary(node, props, boundary_fn) {
 				with_boundary(boundary, () => {
 					is_creating_fallback = false;
 					boundary_effect = branch(() => boundary_fn(anchor));
-					reset_is_throwing_error();
 				});
 			};
 
-			onerror?.(error, reset);
+			var previous_reaction = active_reaction;
+
+			try {
+				set_active_reaction(null);
+				onerror?.(error, reset);
+			} finally {
+				set_active_reaction(previous_reaction);
+			}
 
 			if (boundary_effect) {
 				destroy_effect(boundary_effect);
@@ -109,10 +114,9 @@ export function boundary(node, props, boundary_fn) {
 								);
 							});
 						} catch (error) {
-							handle_error(error, boundary, null, boundary.ctx);
+							invoke_error_boundary(error, /** @type {Effect} */ (boundary.parent));
 						}
 
-						reset_is_throwing_error();
 						is_creating_fallback = false;
 					});
 				});
@@ -124,7 +128,6 @@ export function boundary(node, props, boundary_fn) {
 		}
 
 		boundary_effect = branch(() => boundary_fn(anchor));
-		reset_is_throwing_error();
 	}, EFFECT_TRANSPARENT | BOUNDARY_EFFECT);
 
 	if (hydrating) {

packages/svelte/src/internal/client/error-handling.js

@@ -0,0 +1,88 @@
+/** @import { Effect } from '#client' */
+import { DEV } from 'esm-env';
+import { FILENAME } from '../../constants.js';
+import { is_firefox } from './dom/operations.js';
+import { BOUNDARY_EFFECT, EFFECT_RAN } from './constants.js';
+import { define_property } from '../shared/utils.js';
+import { active_effect } from './runtime.js';
+
+/**
+ * @param {unknown} error
+ */
+export function handle_error(error) {
+	var effect = /** @type {Effect} */ (active_effect);
+
+	if (DEV && error instanceof Error) {
+		adjust_error(error, effect);
+	}
+
+	if ((effect.f & EFFECT_RAN) === 0) {
+		// if the error occurred while creating this subtree, we let it
+		// bubble up until it hits a boundary that can handle it
+		if ((effect.f & BOUNDARY_EFFECT) === 0) {
+			throw error;
+		}
+
+		// @ts-expect-error
+		effect.fn(error);
+	} else {
+		// otherwise we bubble up the effect tree ourselves
+		invoke_error_boundary(error, effect);
+	}
+}
+
+/**
+ * @param {unknown} error
+ * @param {Effect | null} effect
+ */
+export function invoke_error_boundary(error, effect) {
+	while (effect !== null) {
+		if ((effect.f & BOUNDARY_EFFECT) !== 0) {
+			try {
+				// @ts-expect-error
+				effect.fn(error);
+				return;
+			} catch {}
+		}
+
+		effect = effect.parent;
+	}
+
+	throw error;
+}
+
+/** @type {WeakSet<Error>} */
+const adjusted_errors = new WeakSet();
+
+/**
+ * Add useful information to the error message/stack in development
+ * @param {Error} error
+ * @param {Effect} effect
+ */
+function adjust_error(error, effect) {
+	if (adjusted_errors.has(error)) return;
+	adjusted_errors.add(error);
+
+	var indent = is_firefox ? '  ' : '\t';
+	var component_stack = `\n${indent}in ${effect.fn?.name || '<unknown>'}`;
+	var context = effect.ctx;
+
+	while (context !== null) {
+		component_stack += `\n${indent}in ${context.function?.[FILENAME].split('/').pop()}`;
+		context = context.p;
+	}
+
+	define_property(error, 'message', {
+		value: error.message + `\n${component_stack}\n`
+	});
+
+	if (error.stack) {
+		// Filter out internal modules
+		define_property(error, 'stack', {
+			value: error.stack
+				.split('\n')
+				.filter((line) => !line.includes('svelte/src/internal'))
+				.join('\n')
+		});
+	}
+}

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

@@ -332,16 +332,23 @@ export function render_effect(fn) {
  * @returns {Effect}
  */
 export function template_effect(fn, thunks = [], d = derived) {
-	const deriveds = thunks.map(d);
-	const effect = () => fn(...deriveds.map(get));
-
 	if (DEV) {
-		define_property(effect, 'name', {
-			value: '{expression}'
+		// wrap the effect so that we can decorate stack trace with `in {expression}`
+		// (TODO maybe there's a better approach?)
+		return render_effect(() => {
+			var outer = /** @type {Effect} */ (active_effect);
+			var inner = () => fn(...deriveds.map(get));
+
+			define_property(outer.fn, 'name', { value: '{expression}' });
+			define_property(inner, 'name', { value: '{expression}' });
+
+			const deriveds = thunks.map(d);
+			block(inner);
 		});
 	}
 
-	return block(effect);
+	const deriveds = thunks.map(d);
+	return block(() => fn(...deriveds.map(get)));
 }
 
 /**
@@ -426,7 +433,11 @@ export function destroy_block_effect_children(signal) {
 export function destroy_effect(effect, remove_dom = true) {
 	var removed = false;
 
-	if ((remove_dom || (effect.f & HEAD_EFFECT) !== 0) && effect.nodes_start !== null) {
+	if (
+		(remove_dom || (effect.f & HEAD_EFFECT) !== 0) &&
+		effect.nodes_start !== null &&
+		effect.nodes_end !== null
+	) {
 		remove_effect_dom(effect.nodes_start, /** @type {TemplateNode} */ (effect.nodes_end));
 		removed = true;
 	}