fix: rework binding ownership validation (#15678)

* remove old validation

* 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

* remove some now obsolete tests

* fix

* better warnings now that we have more info

* fix

* hoist

* we only care about mutation, not reassignment

* tidy

* handle prop aliases

* mutation validation is only tangentially linked to context requirement

* no need for two vars, one will do

* update warning, include mutation location

* tweak

---------

Co-authored-by: Rich Harris <[email protected]>

Author: dummdidumm

.changeset/sweet-ants-care.md

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

documentation/docs/98-reference/.generated/client-warnings.md

@@ -161,7 +161,7 @@ Tried to unmount a component that was not mounted
 ### ownership_invalid_binding
 
 ```
-%parent% passed a value to %child% with `bind:`, but the value is owned by %owner%. Consider creating a binding between %owner% and %parent%
+%parent% passed property `%prop%` to %child% with `bind:`, but its parent component %owner% did not declare `%prop%` as a binding. Consider creating a binding between %owner% and %parent% (e.g. `bind:%prop%={...}` instead of `%prop%={...}`)
 ```
 
 Consider three components `GrandParent`, `Parent` and `Child`. If you do `<GrandParent bind:value>`, inside `GrandParent` pass on the variable via `<Parent {value} />` (note the missing `bind:`) and then do `<Child bind:value>` inside `Parent`, this warning is thrown.
@@ -171,11 +171,7 @@ To fix it, `bind:` to the value instead of just passing a property (i.e. in this
 ### ownership_invalid_mutation
 
 ```
-Mutating a value outside the component that created it is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
-```
-
-```
-%component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
+Mutating unbound props (`%name%`, at %location%) is strongly discouraged. Consider using `bind:%prop%={...}` in %parent% (or using a callback) instead
 ```
 
 Consider the following code:

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

@@ -132,17 +132,15 @@ During development, this error is often preceeded by a `console.error` detailing
 
 ## ownership_invalid_binding
 
-> %parent% passed a value to %child% with `bind:`, but the value is owned by %owner%. Consider creating a binding between %owner% and %parent%
+> %parent% passed property `%prop%` to %child% with `bind:`, but its parent component %owner% did not declare `%prop%` as a binding. Consider creating a binding between %owner% and %parent% (e.g. `bind:%prop%={...}` instead of `%prop%={...}`)
 
 Consider three components `GrandParent`, `Parent` and `Child`. If you do `<GrandParent bind:value>`, inside `GrandParent` pass on the variable via `<Parent {value} />` (note the missing `bind:`) and then do `<Child bind:value>` inside `Parent`, this warning is thrown.
 
 To fix it, `bind:` to the value instead of just passing a property (i.e. in this example do `<Parent bind:value />`).
 
 ## ownership_invalid_mutation
 
-> Mutating a value outside the component that created it is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
-
-> %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
+> Mutating unbound props (`%name%`, at %location%) is strongly discouraged. Consider using `bind:%prop%={...}` in %parent% (or using a callback) instead
 
 Consider the following code:
 

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

@@ -393,6 +393,12 @@ export function client_component(analysis, options) {
 		);
 	}
 
+	if (analysis.needs_mutation_validation) {
+		component_block.body.unshift(
+			b.var('$$ownership_validator', b.call('$.create_ownership_validator', b.id('$$props')))
+		);
+	}
+
 	const should_inject_context =
 		dev ||
 		analysis.needs_context ||

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

@@ -1,10 +1,10 @@
-/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */
-/** @import { AST, Binding } from '#compiler' */
+/** @import { ArrowFunctionExpression, AssignmentExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Node, Pattern, UpdateExpression } from 'estree' */
+/** @import { Binding } from '#compiler' */
 /** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
 /** @import { Analysis } from '../../types.js' */
 /** @import { Scope } from '../../scope.js' */
 import * as b from '../../../utils/builders.js';
-import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js';
+import { is_simple_expression } from '../../../utils/ast.js';
 import {
 	PROPS_IS_LAZY_INITIAL,
 	PROPS_IS_IMMUTABLE,
@@ -13,7 +13,8 @@ import {
 	PROPS_IS_BINDABLE
 } from '../../../../constants.js';
 import { dev } from '../../../state.js';
-import { get_value } from './visitors/shared/declarations.js';
+import { walk } from 'zimmerframe';
+import { validate_mutation } from './visitors/shared/utils.js';
 
 /**
  * @param {Binding} binding
@@ -110,6 +111,30 @@ function get_hoisted_params(node, context) {
 			}
 		}
 	}
+
+	if (dev) {
+		// this is a little hacky, but necessary for ownership validation
+		// to work inside hoisted event handlers
+
+		/**
+		 * @param {AssignmentExpression | UpdateExpression} node
+		 * @param {{ next: () => void, stop: () => void }} context
+		 */
+		function visit(node, { next, stop }) {
+			if (validate_mutation(node, /** @type {any} */ (context), node) !== node) {
+				params.push(b.id('$$ownership_validator'));
+				stop();
+			} else {
+				next();
+			}
+		}
+
+		walk(/** @type {Node} */ (node), null, {
+			AssignmentExpression: visit,
+			UpdateExpression: visit
+		});
+	}
+
 	return params;
 }
 

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

@@ -7,9 +7,10 @@ import {
 	get_attribute_expression,
 	is_event_attribute
 } from '../../../../utils/ast.js';
-import { dev, is_ignored, locate_node } from '../../../../state.js';
+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
@@ -20,9 +21,7 @@ export function AssignmentExpression(node, context) {
 		visit_assignment_expression(node, context, build_assignment) ?? context.next()
 	);
 
-	return is_ignored(node, 'ownership_invalid_mutation')
-		? b.call('$.skip_ownership_validation', b.thunk(expression))
-		: expression;
+	return validate_mutation(node, context, expression);
 }
 
 /**

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

@@ -161,33 +161,6 @@ export function ClassBody(node, context) {
 		body.push(/** @type {MethodDefinition} **/ (context.visit(definition, child_state)));
 	}
 
-	if (dev && public_state.size > 0) {
-		// add an `[$.ADD_OWNER]` method so that a class with state fields can widen ownership
-		body.push(
-			b.method(
-				'method',
-				b.id('$.ADD_OWNER'),
-				[b.id('owner')],
-				[
-					b.stmt(
-						b.call(
-							'$.add_owner_to_class',
-							b.this,
-							b.id('owner'),
-							b.array(
-								Array.from(public_state).map(([name]) =>
-									b.thunk(b.call('$.get', b.member(b.this, b.private_id(name))))
-								)
-							),
-							is_ignored(node, 'ownership_invalid_binding') && b.true
-						)
-					)
-				],
-				true
-			)
-		);
-	}
-
 	return { ...node, body };
 }
 

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

@@ -1,8 +1,8 @@
 /** @import { AssignmentExpression, Expression, UpdateExpression } from 'estree' */
 /** @import { Context } from '../types' */
-import { is_ignored } from '../../../../state.js';
 import { object } from '../../../../utils/ast.js';
 import * as b from '../../../../utils/builders.js';
+import { validate_mutation } from './shared/utils.js';
 
 /**
  * @param {UpdateExpression} node
@@ -51,7 +51,5 @@ export function UpdateExpression(node, context) {
 		);
 	}
 
-	return is_ignored(node, 'ownership_invalid_mutation')
-		? b.call('$.skip_ownership_validation', b.thunk(update))
-		: update;
+	return validate_mutation(node, context, update);
 }

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

@@ -179,19 +179,29 @@ 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') {
-				binding_initializers.push(
-					b.stmt(
-						b.call(
-							b.id('$.add_owner_effect'),
-							expression.type === 'SequenceExpression'
-								? expression.expressions[0]
-								: b.thunk(expression),
-							b.id(component_name),
-							is_ignored(node, 'ownership_invalid_binding') && b.true
+			if (
+				dev &&
+				attribute.name !== 'this' &&
+				!is_ignored(node, 'ownership_invalid_binding') &&
+				// bind:x={() => x.y, y => x.y = y} will be handled by the assignment expression binding validation
+				attribute.expression.type !== 'SequenceExpression'
+			) {
+				const left = object(attribute.expression);
+				const binding = left && context.state.scope.get(left.name);
+
+				if (binding?.kind === 'bindable_prop' || binding?.kind === 'prop') {
+					context.state.analysis.needs_mutation_validation = true;
+					binding_initializers.push(
+						b.stmt(
+							b.call(
+								'$$ownership_validator.binding',
+								b.literal(binding.node.name),
+								b.id(component_name),
+								b.thunk(expression)
+							)
 						)
-					)
-				);
+					);
+				}
 			}
 
 			if (expression.type === 'SequenceExpression') {

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 { dev, is_ignored, locator } from '../../../../../state.js';
 import { create_derived } from '../../utils.js';
 
 /**
@@ -295,3 +295,60 @@ 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) {
+	let left = /** @type {Expression | Super} */ (
+		node.type === 'AssignmentExpression' ? node.left : node.argument
+	);
+
+	if (!dev || left.type !== 'MemberExpression' || is_ignored(node, 'ownership_invalid_mutation')) {
+		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 = [];
+
+	while (left.type === 'MemberExpression') {
+		if (left.property.type === 'Literal') {
+			path.unshift(left.property);
+		} else if (left.property.type === 'Identifier') {
+			if (left.computed) {
+				path.unshift(left.property);
+			} else {
+				path.unshift(b.literal(left.property.name));
+			}
+		} else {
+			return expression;
+		}
+
+		left = left.object;
+	}
+
+	path.unshift(b.literal(name.name));
+
+	const loc = locator(/** @type {number} */ (left.start));
+
+	return b.call(
+		'$$ownership_validator.mutation',
+		b.literal(binding.prop_alias),
+		b.array(path),
+		expression,
+		loc && b.literal(loc.line),
+		loc && b.literal(loc.column)
+	);
+}

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/constants.js

@@ -23,6 +23,5 @@ export const EFFECT_HAS_DERIVED = 1 << 20;
 export const EFFECT_IS_UPDATING = 1 << 21;
 
 export const STATE_SYMBOL = Symbol('$state');
-export const STATE_SYMBOL_METADATA = Symbol('$state metadata');
 export const LEGACY_PROPS = Symbol('legacy props');
 export const LOADING_ATTR_SYMBOL = Symbol('');

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

@@ -1,7 +1,6 @@
 /** @import { ComponentContext } from '#client' */
 
 import { DEV } from 'esm-env';
-import { add_owner } from './dev/ownership.js';
 import { lifecycle_outside_component } from '../shared/errors.js';
 import { source } from './reactivity/sources.js';
 import {
@@ -67,15 +66,6 @@ export function getContext(key) {
  */
 export function setContext(key, context) {
 	const context_map = get_or_init_context_map('setContext');
-
-	if (DEV) {
-		// When state is put into context, we treat as if it's global from now on.
-		// We do for performance reasons (it's for example very expensive to call
-		// getContext on a big object many times when part of a list component)
-		// and danger of false positives.
-		untrack(() => add_owner(context, null, true));
-	}
-
 	context_map.set(key, context);
 	return context;
 }