fix: rework binding ownership validation

.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;
 }