fix: rework binding ownership validation

Previously we were doing stack-based retrieval of the owner, which while catching more cases was costly (performance-wise) and prone to errors (as shown by many issues over the months).

This drastically simplifies the ownership validation - we now only do simple static analysis to check which props are mutated and wrap them with runtime checks to see if a binding was established.

Besides making the implementation simpler and more performant, this also follows an insight we had over the months: Most people don't really know what to do with this warning when it's shown beyond very simple cases. Either it's not actionable because they don't really know how to fix it or they question if they should at all (in some cases rightfully so). Now that the warning is only shown in simple and easy-to-reason-about cases, it has a much better signal-to-noise-ratio and will hopefully guide people in the right direction early on (learn from the obvious cases to not write spaghetti code in more complex cases).

closes #15532
closes #15210
closes #14893
closes #13607
closes #13139
closes #11861

Author: dummdidumm

.changeset/sweet-ants-care.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: rework binding ownership validation

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -432,6 +432,7 @@ export function analyze_component(root, source, options) {
 		uses_component_bindings: false,
 		uses_render_tags: false,
 		needs_context: false,
+		needs_mutation_validation: false,
 		needs_props: false,
 		event_directive_node: null,
 		uses_event_attributes: false,

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

@@ -402,6 +402,20 @@ export function client_component(analysis, options) {
 	// we want the cleanup function for the stores to run as the very last thing
 	// so that it can effectively clean up the store subscription even after the user effects runs
 	if (should_inject_context) {
+		if (analysis.needs_mutation_validation) {
+			// It's easier to have the mutation validator instance be on the module level because of hoisted events
+			state.hoisted.push(b.let('$$ownership_validator'));
+			component_block.body.unshift(
+				b.stmt(
+					b.assignment(
+						'=',
+						b.id('$$ownership_validator'),
+						b.call('$.create_ownership_validator', b.id('$$props'))
+					)
+				)
+			);
+		}
+
 		component_block.body.unshift(b.stmt(b.call('$.push', ...push_args)));
 
 		let to_push;

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

@@ -10,15 +10,18 @@ import {
 import { dev, locate_node } from '../../../../state.js';
 import { should_proxy } from '../utils.js';
 import { visit_assignment_expression } from '../../shared/assignments.js';
+import { validate_mutation } from './shared/utils.js';
 
 /**
  * @param {AssignmentExpression} node
  * @param {Context} context
  */
 export function AssignmentExpression(node, context) {
-	return /** @type {Expression} */ (
+	const expression = /** @type {Expression} */ (
 		visit_assignment_expression(node, context, build_assignment) ?? context.next()
 	);
+
+	return validate_mutation(node, context, expression);
 }
 
 /**

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

@@ -2,6 +2,7 @@
 /** @import { Context } from '../types' */
 import { object } from '../../../../utils/ast.js';
 import * as b from '../../../../utils/builders.js';
+import { validate_mutation } from './shared/utils.js';
 
 /**
  * @param {UpdateExpression} node
@@ -50,5 +51,5 @@ export function UpdateExpression(node, context) {
 		);
 	}
 
-	return update;
+	return validate_mutation(node, context, update);
 }

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

@@ -178,6 +178,23 @@ export function build_component(node, component_name, context, anchor = context.
 			}
 		} else if (attribute.type === 'BindDirective') {
 			const expression = /** @type {Expression} */ (context.visit(attribute.expression));
+
+			if (dev && attribute.name !== 'this' && !is_ignored(node, 'ownership_invalid_binding')) {
+				context.state.analysis.needs_mutation_validation = true;
+				binding_initializers.push(
+					b.stmt(
+						b.call(
+							b.id('$$ownership_validator.binding'),
+							b.literal(attribute.name),
+							b.id(component_name),
+							expression.type === 'SequenceExpression'
+								? expression.expressions[0]
+								: b.thunk(expression)
+						)
+					)
+				);
+			}
+
 			if (expression.type === 'SequenceExpression') {
 				if (attribute.name === 'this') {
 					bind_this = attribute.expression;

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

@@ -1,13 +1,13 @@
-/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, SequenceExpression, Statement, Super } from 'estree' */
+/** @import { AssignmentExpression, Expression, ExpressionStatement, Identifier, MemberExpression, SequenceExpression, Literal, Super, UpdateExpression } from 'estree' */
 /** @import { AST, ExpressionMetadata } from '#compiler' */
-/** @import { ComponentClientTransformState } from '../../types' */
+/** @import { ComponentClientTransformState, Context } from '../../types' */
 import { walk } from 'zimmerframe';
 import { object } from '../../../../../utils/ast.js';
 import * as b from '../../../../../utils/builders.js';
 import { sanitize_template_string } from '../../../../../utils/sanitize_template_string.js';
 import { regex_is_valid_identifier } from '../../../../patterns.js';
 import is_reference from 'is-reference';
-import { locator } from '../../../../../state.js';
+import { is_ignored, locator } from '../../../../../state.js';
 import { create_derived } from '../../utils.js';
 
 /**
@@ -295,3 +295,55 @@ export function validate_binding(state, binding, expression) {
 		)
 	);
 }
+
+/**
+ * In dev mode validate mutations to props
+ * @param {AssignmentExpression | UpdateExpression} node
+ * @param {Context} context
+ * @param {Expression} expression
+ */
+export function validate_mutation(node, context, expression) {
+	const left = node.type === 'AssignmentExpression' ? node.left : node.argument;
+
+	if (
+		!context.state.options.dev ||
+		is_ignored(node, 'ownership_invalid_mutation') ||
+		(left.type !== 'Identifier' && left.type !== 'MemberExpression')
+	) {
+		return expression;
+	}
+
+	const name = object(left);
+	if (!name) return expression;
+
+	const binding = context.state.scope.get(name.name);
+	if (binding?.kind !== 'prop' && binding?.kind !== 'bindable_prop') return expression;
+
+	const state = /** @type {ComponentClientTransformState} */ (context.state);
+
+	state.analysis.needs_mutation_validation = true;
+
+	/** @type {Array<Identifier | Literal>} */
+	const path = [];
+	/** @type {Expression | Super} */
+	let current = left;
+
+	while (current.type === 'MemberExpression') {
+		if (current.property.type === 'Literal') {
+			path.unshift(current.property);
+		} else if (current.property.type === 'Identifier') {
+			if (current.computed) {
+				path.unshift(current.property);
+			} else {
+				path.unshift(b.literal(current.property.name));
+			}
+		} else {
+			return expression;
+		}
+		current = current.object;
+	}
+
+	path.unshift(b.literal(name.name));
+
+	return b.call('$$ownership_validator.mutation', b.array(path), expression);
+}

packages/svelte/src/compiler/phases/types.d.ts

@@ -53,6 +53,7 @@ export interface ComponentAnalysis extends Analysis {
 	uses_component_bindings: boolean;
 	uses_render_tags: boolean;
 	needs_context: boolean;
+	needs_mutation_validation: boolean;
 	needs_props: boolean;
 	/** Set to the first event directive (on:x) found on a DOM element in the code */
 	event_directive_node: AST.OnDirective | null;

packages/svelte/src/internal/client/dev/ownership.js

@@ -1,5 +1,11 @@
 /** @typedef {{ file: string, line: number, column: number }} Location */
 
+import { get_descriptor } from '../../shared/utils.js';
+import { LEGACY_PROPS, STATE_SYMBOL } from '../constants.js';
+import { FILENAME } from '../../../constants.js';
+import { component_context } from '../context.js';
+import * as w from '../warnings.js';
+
 /** @type {Record<string, Array<{ start: Location, end: Location, component: Function }>>} */
 const boundaries = {};
 
@@ -96,3 +102,64 @@ export function mark_module_end(component) {
 		boundary.component = component;
 	}
 }
+
+/**
+ * Sets up a validator that
+ * - traverses the path of a prop to find out if it is allowed to be mutated
+ * - checks that the binding chain is not interrupted
+ * @param {Record<string, any>} props
+ */
+export function create_ownership_validator(props) {
+	const component = component_context?.function;
+	const parent = component_context?.p?.function;
+
+	/** @param {string} prop_name */
+	function is_bound(prop_name) {
+		// Can be the case when someone does `mount(Component, props)` with `let props = $state({...})`
+		// or `createClassComponent(Component, props)`
+		const is_entry_props = STATE_SYMBOL in props || LEGACY_PROPS in props;
+		const is_bound =
+			!!get_descriptor(props, prop_name)?.set || (is_entry_props && prop_name in props);
+		return is_bound;
+	}
+
+	return {
+		/**
+		 * @param {any[]} path
+		 * @param {any} result
+		 */
+		mutation: (path, result) => {
+			const prop_name = path[0];
+			if (is_bound(prop_name)) {
+				return result;
+			}
+
+			let prop = props[prop_name];
+
+			for (let i = 1; i < path.length - 1; i++) {
+				if (!prop?.[STATE_SYMBOL]) {
+					return result;
+				}
+				prop = prop[path[i]];
+			}
+
+			w.ownership_invalid_mutation(component[FILENAME], parent[FILENAME]);
+
+			return result;
+		},
+		/**
+		 * @param {any} key
+		 * @param {any} child_component
+		 * @param {() => any} value
+		 */
+		binding: (key, child_component, value) => {
+			if (!is_bound(key) && parent && value()?.[STATE_SYMBOL]) {
+				w.ownership_invalid_binding(
+					component[FILENAME],
+					child_component[FILENAME],
+					parent[FILENAME]
+				);
+			}
+		}
+	};
+}

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

@@ -4,7 +4,7 @@ export { assign, assign_and, assign_or, assign_nullish } from './dev/assign.js';
 export { cleanup_styles } from './dev/css.js';
 export { add_locations } from './dev/elements.js';
 export { hmr } from './dev/hmr.js';
-export { mark_module_start, mark_module_end } from './dev/ownership.js';
+export { mark_module_start, mark_module_end, create_ownership_validator } from './dev/ownership.js';
 export { check_target, legacy_api } from './dev/legacy.js';
 export { trace } from './dev/tracing.js';
 export { inspect } from './dev/inspect.js';