feat: warn on invalid event handlers (#12818)

* feat: warn on invalid event handlers

* handle assignments etc

* handle component events too where possible

* lint

Author: Rich-Harris

.changeset/brown-months-flow.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+feat: warn on invalid event handlers

packages/svelte/messages/client-warnings/warnings.md

@@ -4,6 +4,10 @@
 
 > `%binding%` (%location%) is binding to a non-reactive property
 
+## event_handler_invalid
+
+> %handler% should be a function. Did you mean to %suggestion%?
+
 ## hydration_attribute_changed
 
 > The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value

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

@@ -1,7 +1,8 @@
 /** @import { Expression } from 'estree' */
-/** @import { Attribute, ExpressionMetadata, ExpressionTag, OnDirective, SvelteNode } from '#compiler' */
+/** @import { Attribute, ExpressionMetadata, ExpressionTag, SvelteNode } from '#compiler' */
 /** @import { ComponentContext } from '../../types' */
-import { is_capture_event, is_passive_event } from '../../../../../../utils.js';
+import { is_capture_event } from '../../../../../../utils.js';
+import { dev, locator } from '../../../../../state.js';
 import * as b from '../../../../../utils/builders.js';
 
 /**
@@ -136,9 +137,47 @@ export function build_event_handler(node, metadata, context) {
 	}
 
 	// wrap the handler in a function, so the expression is re-evaluated for each event
-	return b.function(
-		null,
-		[b.rest(b.id('$$args'))],
-		b.block([b.stmt(b.call(b.member(handler, b.id('apply'), false, true), b.this, b.id('$$args')))])
-	);
+	let call = b.call(b.member(handler, b.id('apply'), false, true), b.this, b.id('$$args'));
+
+	if (dev) {
+		const loc = locator(/** @type {number} */ (node.start));
+
+		const remove_parens =
+			node.type === 'CallExpression' &&
+			node.arguments.length === 0 &&
+			node.callee.type === 'Identifier';
+
+		call = b.call(
+			'$.apply',
+			b.thunk(handler),
+			b.this,
+			b.id('$$args'),
+			b.id(context.state.analysis.name),
+			loc && b.array([b.literal(loc.line), b.literal(loc.column)]),
+			has_side_effects(node) && b.true,
+			remove_parens && b.true
+		);
+	}
+
+	return b.function(null, [b.rest(b.id('$$args'))], b.block([b.stmt(call)]));
+}
+
+/**
+ * @param {Expression} node
+ */
+function has_side_effects(node) {
+	if (
+		node.type === 'CallExpression' ||
+		node.type === 'NewExpression' ||
+		node.type === 'AssignmentExpression' ||
+		node.type === 'UpdateExpression'
+	) {
+		return true;
+	}
+
+	if (node.type === 'SequenceExpression') {
+		return node.expressions.some(has_side_effects);
+	}
+
+	return false;
 }

packages/svelte/src/internal/client/dom/elements/events.js

@@ -1,7 +1,11 @@
+/** @import { Location } from 'locate-character' */
 import { teardown } from '../../reactivity/effects.js';
 import { define_property, is_array } from '../../../shared/utils.js';
 import { hydrating } from '../hydration.js';
 import { queue_micro_task } from '../task.js';
+import { dev_current_component_function } from '../../runtime.js';
+import { FILENAME } from '../../../../constants.js';
+import * as w from '../../warnings.js';
 
 /** @type {Set<string>} */
 export const all_registered_events = new Set();
@@ -273,3 +277,53 @@ export function handle_event_propagation(event) {
 		current_target = handler_element;
 	}
 }
+
+/**
+ * In dev, warn if an event handler is not a function, as it means the
+ * user probably called the handler or forgot to add a `() =>`
+ * @param {() => (event: Event, ...args: any) => void} thunk
+ * @param {EventTarget} element
+ * @param {[Event, ...any]} args
+ * @param {any} component
+ * @param {[number, number]} [loc]
+ * @param {boolean} [remove_parens]
+ */
+export function apply(
+	thunk,
+	element,
+	args,
+	component,
+	loc,
+	has_side_effects = false,
+	remove_parens = false
+) {
+	let handler;
+	let error;
+
+	try {
+		handler = thunk();
+	} catch (e) {
+		error = e;
+	}
+
+	if (typeof handler === 'function') {
+		handler.apply(element, args);
+	} else if (has_side_effects || handler != null) {
+		const filename = component?.[FILENAME];
+		const location = filename
+			? loc
+				? ` at ${filename}:${loc[0]}:${loc[1]}`
+				: ` in ${filename}`
+			: '';
+
+		const event_name = args[0].type;
+		const description = `\`${event_name}\` handler${location}`;
+		const suggestion = remove_parens ? 'remove the trailing `()`' : 'add a leading `() =>`';
+
+		w.event_handler_invalid(description, suggestion);
+
+		if (error) {
+			throw error;
+		}
+	}
+}

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

@@ -36,7 +36,7 @@ export {
 	set_checked
 } from './dom/elements/attributes.js';
 export { set_class, set_svg_class, set_mathml_class, toggle_class } from './dom/elements/class.js';
-export { event, delegate, replay_events } from './dom/elements/events.js';
+export { apply, event, delegate, replay_events } from './dom/elements/events.js';
 export { autofocus, remove_textarea_child } from './dom/elements/misc.js';
 export { set_style } from './dom/elements/style.js';
 export { animation, transition } from './dom/elements/transitions.js';

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

@@ -91,19 +91,24 @@ let stack = [];
  * @returns {void}
  */
 export function update_derived(derived) {
-	if (DEV) {
-		if (stack.includes(derived)) {
-			e.derived_references_self();
-		}
+	var value;
 
-		stack.push(derived);
-	}
+	if (DEV) {
+		try {
+			if (stack.includes(derived)) {
+				e.derived_references_self();
+			}
 
-	destroy_derived_children(derived);
-	var value = update_reaction(derived);
+			stack.push(derived);
 
-	if (DEV) {
-		stack.pop();
+			destroy_derived_children(derived);
+			value = update_reaction(derived);
+		} finally {
+			stack.pop();
+		}
+	} else {
+		destroy_derived_children(derived);
+		value = update_reaction(derived);
 	}
 
 	var status =

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

@@ -19,6 +19,20 @@ export function binding_property_non_reactive(binding, location) {
 	}
 }
 
+/**
+ * %handler% should be a function. Did you mean to %suggestion%?
+ * @param {string} handler
+ * @param {string} suggestion
+ */
+export function event_handler_invalid(handler, suggestion) {
+	if (DEV) {
+		console.warn(`%c[svelte] event_handler_invalid\n%c${handler} should be a function. Did you mean to ${suggestion}?`, bold, normal);
+	} else {
+		// TODO print a link to the documentation
+		console.warn("event_handler_invalid");
+	}
+}
+
 /**
  * The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value
  * @param {string} attribute

packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/Button.svelte

@@ -0,0 +1,7 @@
+<script>
+	let { children, onclick } = $props();
+</script>
+
+<button {onclick}>
+	{@render children()}
+</button>

packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/_config.js

@@ -0,0 +1,17 @@
+import { test } from '../../test';
+
+export default test({
+	mode: ['client'],
+
+	compileOptions: {
+		dev: true
+	},
+
+	test({ assert, target, warnings }) {
+		target.querySelector('button')?.click();
+
+		assert.deepEqual(warnings, [
+			'`click` handler at Button.svelte:5:9 should be a function. Did you mean to add a leading `() =>`?'
+		]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/main.svelte

@@ -0,0 +1,9 @@
+<script>
+	import Button from './Button.svelte';
+
+	let count = $state(0);
+</script>
+
+<Button onclick={count++}>
+	clicks: {count}
+</Button>