From 909301f42f3011f7fd600eb92bf8dba5f27ed150 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 3 Apr 2025 10:20:54 +0200 Subject: [PATCH 01/14] remove old validation --- .../client/visitors/AssignmentExpression.js | 8 +- .../3-transform/client/visitors/ClassBody.js | 27 --- .../client/visitors/UpdateExpression.js | 5 +- .../client/visitors/shared/component.js | 16 -- .../svelte/src/internal/client/constants.js | 1 - .../svelte/src/internal/client/context.js | 10 - .../src/internal/client/dev/ownership.js | 206 ------------------ packages/svelte/src/internal/client/index.js | 10 +- packages/svelte/src/internal/client/proxy.js | 81 +------ .../src/internal/client/reactivity/sources.js | 2 +- .../svelte/src/internal/client/types.d.ts | 8 - 11 files changed, 9 insertions(+), 365 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js index 850cd83b15b6..5e8a24636ec7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js @@ -7,7 +7,7 @@ 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'; @@ -16,13 +16,9 @@ import { visit_assignment_expression } from '../../shared/assignments.js'; * @param {Context} context */ export function AssignmentExpression(node, context) { - const expression = /** @type {Expression} */ ( + return /** @type {Expression} */ ( 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; } /** diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js index 24c20d7f947f..efc3c95c3c34 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js +++ b/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 }; } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js index 13c1b4bc51e1..766a018078a1 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js @@ -1,6 +1,5 @@ /** @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'; @@ -51,7 +50,5 @@ export function UpdateExpression(node, context) { ); } - return is_ignored(node, 'ownership_invalid_mutation') - ? b.call('$.skip_ownership_validation', b.thunk(update)) - : update; + return update; } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index 2bae4486dc58..cc9e39a478be 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -178,22 +178,6 @@ 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 (expression.type === 'SequenceExpression') { if (attribute.name === 'this') { bind_this = attribute.expression; diff --git a/packages/svelte/src/internal/client/constants.js b/packages/svelte/src/internal/client/constants.js index 21377c1cc85f..7e5196c606b4 100644 --- a/packages/svelte/src/internal/client/constants.js +++ b/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(''); diff --git a/packages/svelte/src/internal/client/context.js b/packages/svelte/src/internal/client/context.js index bfca9d5e6a72..7a2fdd0edb6d 100644 --- a/packages/svelte/src/internal/client/context.js +++ b/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; } diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 62119b36dbd6..45c7c52765ba 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -1,13 +1,5 @@ -/** @import { ProxyMetadata } from '#client' */ /** @typedef {{ file: string, line: number, column: number }} Location */ -import { STATE_SYMBOL_METADATA } from '../constants.js'; -import { render_effect, user_pre_effect } from '../reactivity/effects.js'; -import { dev_current_component_function } from '../context.js'; -import { get_prototype_of } from '../../shared/utils.js'; -import * as w from '../warnings.js'; -import { FILENAME, UNINITIALIZED } from '../../../constants.js'; - /** @type {Record>} */ const boundaries = {}; @@ -71,8 +63,6 @@ export function get_component() { return null; } -export const ADD_OWNER = Symbol('ADD_OWNER'); - /** * Together with `mark_module_end`, this function establishes the boundaries of a `.svelte` file, * such that subsequent calls to `get_component` can tell us which component is responsible @@ -106,199 +96,3 @@ export function mark_module_end(component) { boundary.component = component; } } - -/** - * @param {any} object - * @param {any | null} owner - * @param {boolean} [global] - * @param {boolean} [skip_warning] - */ -export function add_owner(object, owner, global = false, skip_warning = false) { - if (object && !global) { - const component = dev_current_component_function; - const metadata = object[STATE_SYMBOL_METADATA]; - if (metadata && !has_owner(metadata, component)) { - let original = get_owner(metadata); - - if (owner && owner[FILENAME] !== component[FILENAME] && !skip_warning) { - w.ownership_invalid_binding(component[FILENAME], owner[FILENAME], original[FILENAME]); - } - } - } - - add_owner_to_object(object, owner, new Set()); -} - -/** - * @param {() => unknown} get_object - * @param {any} Component - * @param {boolean} [skip_warning] - */ -export function add_owner_effect(get_object, Component, skip_warning = false) { - user_pre_effect(() => { - add_owner(get_object(), Component, false, skip_warning); - }); -} - -/** - * @param {any} _this - * @param {Function} owner - * @param {Array<() => any>} getters - * @param {boolean} skip_warning - */ -export function add_owner_to_class(_this, owner, getters, skip_warning) { - _this[ADD_OWNER].current ||= getters.map(() => UNINITIALIZED); - - for (let i = 0; i < getters.length; i += 1) { - const current = getters[i](); - // For performance reasons we only re-add the owner if the state has changed - if (current !== _this[ADD_OWNER][i]) { - _this[ADD_OWNER].current[i] = current; - add_owner(current, owner, false, skip_warning); - } - } -} - -/** - * @param {ProxyMetadata | null} from - * @param {ProxyMetadata} to - */ -export function widen_ownership(from, to) { - if (to.owners === null) { - return; - } - - while (from) { - if (from.owners === null) { - to.owners = null; - break; - } - - for (const owner of from.owners) { - to.owners.add(owner); - } - - from = from.parent; - } -} - -/** - * @param {any} object - * @param {Function | null} owner If `null`, then the object is globally owned and will not be checked - * @param {Set} seen - */ -function add_owner_to_object(object, owner, seen) { - const metadata = /** @type {ProxyMetadata} */ (object?.[STATE_SYMBOL_METADATA]); - - if (metadata) { - // this is a state proxy, add owner directly, if not globally shared - if ('owners' in metadata && metadata.owners != null) { - if (owner) { - metadata.owners.add(owner); - } else { - metadata.owners = null; - } - } - } else if (object && typeof object === 'object') { - if (seen.has(object)) return; - seen.add(object); - if (ADD_OWNER in object && object[ADD_OWNER]) { - // this is a class with state fields. we put this in a render effect - // so that if state is replaced (e.g. `instance.name = { first, last }`) - // the new state is also co-owned by the caller of `getContext` - render_effect(() => { - object[ADD_OWNER](owner); - }); - } else { - var proto = get_prototype_of(object); - - if (proto === Object.prototype) { - // recurse until we find a state proxy - for (const key in object) { - if (Object.getOwnPropertyDescriptor(object, key)?.get) { - // Similar to the class case; the getter could update with a new state - let current = UNINITIALIZED; - render_effect(() => { - const next = object[key]; - if (current !== next) { - current = next; - add_owner_to_object(next, owner, seen); - } - }); - } else { - add_owner_to_object(object[key], owner, seen); - } - } - } else if (proto === Array.prototype) { - // recurse until we find a state proxy - for (let i = 0; i < object.length; i += 1) { - add_owner_to_object(object[i], owner, seen); - } - } - } - } -} - -/** - * @param {ProxyMetadata} metadata - * @param {Function} component - * @returns {boolean} - */ -function has_owner(metadata, component) { - if (metadata.owners === null) { - return true; - } - - return ( - metadata.owners.has(component) || - // This helps avoid false positives when using HMR, where the component function is replaced - (FILENAME in component && - [...metadata.owners].some( - (owner) => /** @type {any} */ (owner)[FILENAME] === component[FILENAME] - )) || - (metadata.parent !== null && has_owner(metadata.parent, component)) - ); -} - -/** - * @param {ProxyMetadata} metadata - * @returns {any} - */ -function get_owner(metadata) { - return ( - metadata?.owners?.values().next().value ?? - get_owner(/** @type {ProxyMetadata} */ (metadata.parent)) - ); -} - -let skip = false; - -/** - * @param {() => any} fn - */ -export function skip_ownership_validation(fn) { - skip = true; - fn(); - skip = false; -} - -/** - * @param {ProxyMetadata} metadata - */ -export function check_ownership(metadata) { - if (skip) return; - - const component = get_component(); - - if (component && !has_owner(metadata, component)) { - let original = get_owner(metadata); - - // @ts-expect-error - if (original[FILENAME] !== component[FILENAME]) { - // @ts-expect-error - w.ownership_invalid_mutation(component[FILENAME], original[FILENAME]); - } else { - w.ownership_invalid_mutation(); - } - } -} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index a5f93e8b171b..5f95647fcc0e 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -4,15 +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 { - ADD_OWNER, - add_owner, - mark_module_start, - mark_module_end, - add_owner_effect, - add_owner_to_class, - skip_ownership_validation -} from './dev/ownership.js'; +export { mark_module_start, mark_module_end } 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'; diff --git a/packages/svelte/src/internal/client/proxy.js b/packages/svelte/src/internal/client/proxy.js index fab271c91652..5e0aa3dbc35f 100644 --- a/packages/svelte/src/internal/client/proxy.js +++ b/packages/svelte/src/internal/client/proxy.js @@ -1,7 +1,6 @@ -/** @import { ProxyMetadata, Source } from '#client' */ +/** @import { Source } from '#client' */ import { DEV } from 'esm-env'; import { get, active_effect, active_reaction, set_active_reaction } from './runtime.js'; -import { component_context } from './context.js'; import { array_prototype, get_descriptor, @@ -9,24 +8,19 @@ import { is_array, object_prototype } from '../shared/utils.js'; -import { check_ownership, widen_ownership } from './dev/ownership.js'; import { state as source, set } from './reactivity/sources.js'; -import { STATE_SYMBOL, STATE_SYMBOL_METADATA } from './constants.js'; +import { STATE_SYMBOL } from './constants.js'; import { UNINITIALIZED } from '../../constants.js'; import * as e from './errors.js'; import { get_stack } from './dev/tracing.js'; import { tracing_mode_flag } from '../flags/index.js'; -/** @type {ProxyMetadata | null} */ -var parent_metadata = null; - /** * @template T * @param {T} value - * @param {Source} [prev] dev mode only * @returns {T} */ -export function proxy(value, prev) { +export function proxy(value) { // if non-proxyable, or is already a proxy, return `value` if (typeof value !== 'object' || value === null || STATE_SYMBOL in value) { return value; @@ -55,16 +49,7 @@ export function proxy(value, prev) { set_active_reaction(reaction); /** @type {T} */ - var result; - - if (DEV) { - var previous_metadata = parent_metadata; - parent_metadata = metadata; - result = fn(); - parent_metadata = previous_metadata; - } else { - result = fn(); - } + var result = fn(); set_active_reaction(previous_reaction); return result; @@ -76,31 +61,6 @@ export function proxy(value, prev) { sources.set('length', source(/** @type {any[]} */ (value).length, stack)); } - /** @type {ProxyMetadata} */ - var metadata; - - if (DEV) { - metadata = { - parent: parent_metadata, - owners: null - }; - - if (prev) { - // Reuse owners from previous state; necessary because reassignment is not guaranteed to have correct component context. - // If no previous proxy exists we play it safe and assume ownerless state - // @ts-expect-error - const prev_owners = prev.v?.[STATE_SYMBOL_METADATA]?.owners; - metadata.owners = prev_owners ? new Set(prev_owners) : null; - } else { - metadata.owners = - parent_metadata === null - ? component_context !== null - ? new Set([component_context.function]) - : null - : new Set(); - } - } - return new Proxy(/** @type {any} */ (value), { defineProperty(_, prop, descriptor) { if ( @@ -160,10 +120,6 @@ export function proxy(value, prev) { }, get(target, prop, receiver) { - if (DEV && prop === STATE_SYMBOL_METADATA) { - return metadata; - } - if (prop === STATE_SYMBOL) { return value; } @@ -179,22 +135,6 @@ export function proxy(value, prev) { if (s !== undefined) { var v = get(s); - - // In case of something like `foo = bar.map(...)`, foo would have ownership - // of the array itself, while the individual items would have ownership - // of the component that created bar. That means if we later do `foo[0].baz = 42`, - // we could get a false-positive ownership violation, since the two proxies - // are not connected to each other via the parent metadata relationship. - // For this reason, we need to widen the ownership of the children - // upon access when we detect they are not connected. - if (DEV) { - /** @type {ProxyMetadata | undefined} */ - var prop_metadata = v?.[STATE_SYMBOL_METADATA]; - if (prop_metadata && prop_metadata?.parent !== metadata) { - widen_ownership(metadata, prop_metadata); - } - } - return v === UNINITIALIZED ? undefined : v; } @@ -225,10 +165,6 @@ export function proxy(value, prev) { }, has(target, prop) { - if (DEV && prop === STATE_SYMBOL_METADATA) { - return true; - } - if (prop === STATE_SYMBOL) { return true; } @@ -295,15 +231,6 @@ export function proxy(value, prev) { ); } - if (DEV) { - /** @type {ProxyMetadata | undefined} */ - var prop_metadata = value?.[STATE_SYMBOL_METADATA]; - if (prop_metadata && prop_metadata?.parent !== metadata) { - widen_ownership(metadata, prop_metadata); - } - check_ownership(metadata); - } - var descriptor = Reflect.getOwnPropertyDescriptor(target, prop); // Set the new value before updating any signals so that any listeners get the new value diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index e4834902fe3f..aa65d5354ac7 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -139,7 +139,7 @@ export function set(source, value, should_proxy = false) { e.state_unsafe_mutation(); } - let new_value = should_proxy ? proxy(value, source) : value; + let new_value = should_proxy ? proxy(value) : value; return internal_set(source, new_value); } diff --git a/packages/svelte/src/internal/client/types.d.ts b/packages/svelte/src/internal/client/types.d.ts index 0c260a0a9ffa..b46bdf201341 100644 --- a/packages/svelte/src/internal/client/types.d.ts +++ b/packages/svelte/src/internal/client/types.d.ts @@ -179,14 +179,6 @@ export type TaskCallback = (now: number) => boolean | void; export type TaskEntry = { c: TaskCallback; f: () => void }; -/** Dev-only */ -export interface ProxyMetadata { - /** The components that 'own' this state, if any. `null` means no owners, i.e. everyone can mutate this state. */ - owners: null | Set; - /** The parent metadata object */ - parent: null | ProxyMetadata; -} - export type ProxyStateObject> = T & { [STATE_SYMBOL]: T; }; From 88a83d67851faa3012ec2ea9d9c537d516f0f93e Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 3 Apr 2025 13:26:18 +0200 Subject: [PATCH 02/14] 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 --- .changeset/sweet-ants-care.md | 5 ++ .../src/compiler/phases/2-analyze/index.js | 1 + .../3-transform/client/transform-client.js | 14 ++++ .../client/visitors/AssignmentExpression.js | 5 +- .../client/visitors/UpdateExpression.js | 3 +- .../client/visitors/shared/component.js | 17 +++++ .../client/visitors/shared/utils.js | 58 +++++++++++++++- .../svelte/src/compiler/phases/types.d.ts | 1 + .../src/internal/client/dev/ownership.js | 67 +++++++++++++++++++ packages/svelte/src/internal/client/index.js | 2 +- 10 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 .changeset/sweet-ants-care.md diff --git a/.changeset/sweet-ants-care.md b/.changeset/sweet-ants-care.md new file mode 100644 index 000000000000..b4805626ab4e --- /dev/null +++ b/.changeset/sweet-ants-care.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: rework binding ownership validation diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 1f636c32df6d..c62fb03e8fef 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/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, diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index 0bdfbae746d0..c6fc4fc849cf 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/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; diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js index 5e8a24636ec7..4baa1c8e6ce5 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js +++ b/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); } /** diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js index 766a018078a1..63c03b0eb6f2 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/UpdateExpression.js +++ b/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); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index cc9e39a478be..d0bae9e6cbe6 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/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; diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index df6308d6316a..2a65ce577dcf 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/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} */ + 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); +} diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index abe2b115de02..f09b88130305 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/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; diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 45c7c52765ba..ddd187296417 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/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>} */ 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} 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] + ); + } + } + }; +} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 5f95647fcc0e..7eed8a744afa 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/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'; From 7c95a0e98e4667fed198264d37a97cbdae8ca1e9 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 3 Apr 2025 13:33:41 +0200 Subject: [PATCH 03/14] remove some now obsolete tests --- .../non-local-mutation-global-2/_config.js | 11 ----- .../non-local-mutation-global-2/child.svelte | 18 -------- .../non-local-mutation-global-2/main.svelte | 5 -- .../_config.js | 6 +-- .../main.svelte | 9 ++++ .../sub.svelte | 0 .../_config.js | 33 ++++--------- .../main.svelte | 11 ++--- .../state.svelte.js | 15 +++++- .../sub.svelte | 0 .../Child.svelte | 0 .../_config.js | 33 ++++--------- .../main.svelte | 14 ++++-- .../_config.js | 37 --------------- .../main.svelte | 11 ----- .../state.svelte.js | 13 ------ .../sub.svelte | 8 ---- .../_config.js | 24 ---------- .../main.svelte | 8 ---- .../state.svelte.js | 14 ------ .../Child.svelte | 9 ---- .../_config.js | 37 --------------- .../main.svelte | 17 ------- .../_config.js | 24 ---------- .../main.svelte | 13 ------ .../Counter.svelte | 7 --- .../main.svelte | 9 ---- .../state.svelte.js | 3 -- .../_config.js | 1 - .../CounterBinding.svelte | 7 --- .../CounterContext.svelte | 13 ------ .../_config.js | 34 -------------- .../main.svelte | 46 ------------------- 33 files changed, 56 insertions(+), 434 deletions(-) delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte rename packages/svelte/tests/runtime-runes/samples/{non-local-mutation-inherited-owner => non-local-mutation-inherited-owner-1}/_config.js (74%) create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/main.svelte rename packages/svelte/tests/runtime-runes/samples/{non-local-mutation-inherited-owner-3 => non-local-mutation-inherited-owner-1}/sub.svelte (100%) rename packages/svelte/tests/runtime-runes/samples/{non-local-mutation-inherited-owner-5 => non-local-mutation-inherited-owner-2}/sub.svelte (100%) rename packages/svelte/tests/runtime-runes/samples/{non-local-mutation-inherited-owner-7 => non-local-mutation-inherited-owner-3}/Child.svelte (100%) delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/state.svelte.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/sub.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/state.svelte.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/Child.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/Counter.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/state.svelte.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js deleted file mode 100644 index b4864154c39a..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js +++ /dev/null @@ -1,11 +0,0 @@ -import { test } from '../../test'; - -export default test({ - compileOptions: { - dev: true - }, - - async test({ assert, warnings }) { - assert.deepEqual(warnings, []); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte deleted file mode 100644 index 13de75364752..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte +++ /dev/null @@ -1,18 +0,0 @@ - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte deleted file mode 100644 index 8a6922e9e250..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte +++ /dev/null @@ -1,5 +0,0 @@ - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/_config.js similarity index 74% rename from packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/_config.js rename to packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/_config.js index c07b9ce129dc..96b18d1854c8 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/_config.js @@ -8,7 +8,7 @@ let warn; let warnings = []; export default test({ - html: ``, + html: ``, compileOptions: { dev: true @@ -34,8 +34,8 @@ export default test({ btn?.click(); }); - assert.htmlEqual(target.innerHTML, ``); + assert.htmlEqual(target.innerHTML, ``); - assert.deepEqual(warnings, []); + assert.deepEqual(warnings, [], 'expected getContext to have widened ownership'); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/main.svelte new file mode 100644 index 000000000000..2dd7cab141d6 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/main.svelte @@ -0,0 +1,9 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/sub.svelte similarity index 100% rename from packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/sub.svelte rename to packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-1/sub.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/_config.js index c07b9ce129dc..66f1726a2aef 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/_config.js @@ -1,41 +1,24 @@ import { flushSync } from 'svelte'; import { test } from '../../test'; -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - export default test({ - html: ``, - compileOptions: { dev: true }, - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, + test({ assert, target, warnings }) { + const [btn1, btn2] = target.querySelectorAll('button'); - after_test: () => { - console.warn = warn; - warnings = []; - }, + flushSync(() => { + btn1.click(); + }); - test({ assert, target }) { - const btn = target.querySelector('button'); + assert.deepEqual(warnings.length, 0); flushSync(() => { - btn?.click(); + btn2.click(); }); - assert.htmlEqual(target.innerHTML, ``); - - assert.deepEqual(warnings, []); + assert.deepEqual(warnings.length, 1); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/main.svelte index ad450a937e40..0be7e434e475 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/main.svelte @@ -1,9 +1,8 @@ - - + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/state.svelte.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/state.svelte.js index 3e7a68cf97d8..2906b9bce52b 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/state.svelte.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/state.svelte.js @@ -1 +1,14 @@ -export let global = $state({}); +export function create_my_state() { + const my_state = $state({ + a: 0 + }); + + function inc() { + my_state.a++; + } + + return { + my_state, + inc + }; +} diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/sub.svelte similarity index 100% rename from packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/sub.svelte rename to packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-2/sub.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/Child.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/Child.svelte similarity index 100% rename from packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/Child.svelte rename to packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/Child.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/_config.js index 96b18d1854c8..ab7327ab8b82 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/_config.js @@ -1,41 +1,24 @@ import { flushSync } from 'svelte'; import { test } from '../../test'; -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - export default test({ - html: ``, - compileOptions: { dev: true }, - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, + async test({ assert, target, warnings }) { + const [btn1, btn2] = target.querySelectorAll('button'); - after_test: () => { - console.warn = warn; - warnings = []; - }, + flushSync(() => { + btn1.click(); + }); - test({ assert, target }) { - const btn = target.querySelector('button'); + assert.deepEqual(warnings.length, 0); flushSync(() => { - btn?.click(); + btn2.click(); }); - assert.htmlEqual(target.innerHTML, ``); - - assert.deepEqual(warnings, [], 'expected getContext to have widened ownership'); + assert.deepEqual(warnings.length, 1); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/main.svelte index 2dd7cab141d6..8e8343790b1b 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-3/main.svelte @@ -1,9 +1,13 @@ - + + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/_config.js deleted file mode 100644 index aeb3740dfe71..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/_config.js +++ /dev/null @@ -1,37 +0,0 @@ -import { flushSync } from 'svelte'; -import { test } from '../../test'; - -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - -export default test({ - compileOptions: { - dev: true - }, - - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, - - after_test: () => { - console.warn = warn; - warnings = []; - }, - - test({ assert, target }) { - const btn = target.querySelector('button'); - - flushSync(() => { - btn?.click(); - }); - - assert.deepEqual(warnings, []); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/main.svelte deleted file mode 100644 index 2d40c13949a6..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/main.svelte +++ /dev/null @@ -1,11 +0,0 @@ - - - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/state.svelte.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/state.svelte.js deleted file mode 100644 index 40790591712c..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/state.svelte.js +++ /dev/null @@ -1,13 +0,0 @@ -class Global { - state = $state({}); - - add_a(a) { - this.state.a = a; - } - - increment_a_b() { - this.state.a.b++; - } -} - -export const global = new Global(); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/sub.svelte deleted file mode 100644 index 044904aa187e..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-4/sub.svelte +++ /dev/null @@ -1,8 +0,0 @@ - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js deleted file mode 100644 index 66f1726a2aef..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/_config.js +++ /dev/null @@ -1,24 +0,0 @@ -import { flushSync } from 'svelte'; -import { test } from '../../test'; - -export default test({ - compileOptions: { - dev: true - }, - - test({ assert, target, warnings }) { - const [btn1, btn2] = target.querySelectorAll('button'); - - flushSync(() => { - btn1.click(); - }); - - assert.deepEqual(warnings.length, 0); - - flushSync(() => { - btn2.click(); - }); - - assert.deepEqual(warnings.length, 1); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/main.svelte deleted file mode 100644 index 0be7e434e475..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/main.svelte +++ /dev/null @@ -1,8 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/state.svelte.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/state.svelte.js deleted file mode 100644 index 2906b9bce52b..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-5/state.svelte.js +++ /dev/null @@ -1,14 +0,0 @@ -export function create_my_state() { - const my_state = $state({ - a: 0 - }); - - function inc() { - my_state.a++; - } - - return { - my_state, - inc - }; -} diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/Child.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/Child.svelte deleted file mode 100644 index aa31fd7606dd..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/Child.svelte +++ /dev/null @@ -1,9 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js deleted file mode 100644 index cc9ea715f0aa..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js +++ /dev/null @@ -1,37 +0,0 @@ -import { flushSync } from 'svelte'; -import { test } from '../../test'; - -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - -export default test({ - compileOptions: { - dev: true - }, - - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, - - after_test: () => { - console.warn = warn; - warnings = []; - }, - - async test({ assert, target }) { - const btn = target.querySelector('button'); - - flushSync(() => { - btn?.click(); - }); - - assert.deepEqual(warnings.length, 0); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte deleted file mode 100644 index 92d7dbd2db6c..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte +++ /dev/null @@ -1,17 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js deleted file mode 100644 index ab7327ab8b82..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/_config.js +++ /dev/null @@ -1,24 +0,0 @@ -import { flushSync } from 'svelte'; -import { test } from '../../test'; - -export default test({ - compileOptions: { - dev: true - }, - - async test({ assert, target, warnings }) { - const [btn1, btn2] = target.querySelectorAll('button'); - - flushSync(() => { - btn1.click(); - }); - - assert.deepEqual(warnings.length, 0); - - flushSync(() => { - btn2.click(); - }); - - assert.deepEqual(warnings.length, 1); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/main.svelte deleted file mode 100644 index 8e8343790b1b..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-7/main.svelte +++ /dev/null @@ -1,13 +0,0 @@ - - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/Counter.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/Counter.svelte deleted file mode 100644 index ffe6ef75c4ed..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/Counter.svelte +++ /dev/null @@ -1,7 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/main.svelte deleted file mode 100644 index 5f1c7461f636..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/main.svelte +++ /dev/null @@ -1,9 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/state.svelte.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/state.svelte.js deleted file mode 100644 index 6881c2faf66b..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner/state.svelte.js +++ /dev/null @@ -1,3 +0,0 @@ -export let global = $state({ - object: { count: -1 } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-7/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-7/_config.js index e766a946d0dc..bd2ecc28b6f7 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-7/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-7/_config.js @@ -1,7 +1,6 @@ import { flushSync } from 'svelte'; import { ok, test } from '../../test'; -// Tests that proxies widen ownership correctly even if not directly connected to each other export default test({ compileOptions: { dev: true diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte deleted file mode 100644 index d6da559fb176..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte +++ /dev/null @@ -1,7 +0,0 @@ - - -

Binding

- - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte deleted file mode 100644 index b935f0a472dc..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte +++ /dev/null @@ -1,13 +0,0 @@ - - -

Context

- - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js deleted file mode 100644 index d6d12d01cd09..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js +++ /dev/null @@ -1,34 +0,0 @@ -import { flushSync } from 'svelte'; -import { test } from '../../test'; - -// Tests that ownership is widened with $derived (on class or on its own) that contains $state -export default test({ - compileOptions: { - dev: true - }, - - test({ assert, target, warnings }) { - const [root, counter_context1, counter_context2, counter_binding1, counter_binding2] = - target.querySelectorAll('button'); - - counter_context1.click(); - counter_context2.click(); - counter_binding1.click(); - counter_binding2.click(); - flushSync(); - - assert.equal(warnings.length, 0); - - root.click(); - flushSync(); - counter_context1.click(); - counter_context2.click(); - counter_binding1.click(); - counter_binding2.click(); - flushSync(); - - assert.equal(warnings.length, 0); - }, - - warnings: [] -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte deleted file mode 100644 index aaade26e162c..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte +++ /dev/null @@ -1,46 +0,0 @@ - - -

Parent

- - - - From 0dbba03c332059d98265287ec5d6bb5e3ad274b4 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 3 Apr 2025 14:02:07 +0200 Subject: [PATCH 04/14] fix --- .../client/visitors/shared/component.js | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index d0bae9e6cbe6..3f12d610edca 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -179,20 +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' && !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 ( + 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( + b.id('$$ownership_validator.binding'), + b.literal(binding.node.name), + b.id(component_name), + b.thunk(expression) + ) ) - ) - ); + ); + } } if (expression.type === 'SequenceExpression') { From ae73afd584d5c4c433bc8be3fba4dec44d1e8cc4 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 3 Apr 2025 14:10:21 +0200 Subject: [PATCH 05/14] better warnings now that we have more info --- .../98-reference/.generated/client-warnings.md | 8 ++------ .../messages/client-warnings/warnings.md | 6 ++---- .../src/internal/client/dev/ownership.js | 5 +++-- .../svelte/src/internal/client/warnings.js | 18 ++++++++++-------- .../non-local-mutation-discouraged/_config.js | 2 +- .../_config.js | 2 +- .../_config.js | 2 +- 7 files changed, 20 insertions(+), 23 deletions(-) diff --git a/documentation/docs/98-reference/.generated/client-warnings.md b/documentation/docs/98-reference/.generated/client-warnings.md index 284e9a7c3e57..e8098b170965 100644 --- a/documentation/docs/98-reference/.generated/client-warnings.md +++ b/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 ``, inside `GrandParent` pass on the variable via `` (note the missing `bind:`) and then do `` 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 +%component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead ``` Consider the following code: diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index 943cf6f01f4f..617df1452b5e 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -132,7 +132,7 @@ 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 ``, inside `GrandParent` pass on the variable via `` (note the missing `bind:`) and then do `` inside `Parent`, this warning is thrown. @@ -140,9 +140,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 +> %component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead Consider the following code: diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index ddd187296417..432b5a050ab5 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -130,7 +130,7 @@ export function create_ownership_validator(props) { */ mutation: (path, result) => { const prop_name = path[0]; - if (is_bound(prop_name)) { + if (is_bound(prop_name) || !parent) { return result; } @@ -143,7 +143,7 @@ export function create_ownership_validator(props) { prop = prop[path[i]]; } - w.ownership_invalid_mutation(component[FILENAME], parent[FILENAME]); + w.ownership_invalid_mutation(component[FILENAME], prop_name, parent[FILENAME]); return result; }, @@ -156,6 +156,7 @@ export function create_ownership_validator(props) { if (!is_bound(key) && parent && value()?.[STATE_SYMBOL]) { w.ownership_invalid_binding( component[FILENAME], + key, child_component[FILENAME], parent[FILENAME] ); diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index 250c6eca2fe9..bcb6deaaa2f7 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -129,27 +129,29 @@ export function lifecycle_double_unmount() { } /** - * %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%={...}`) * @param {string} parent + * @param {string} prop * @param {string} child * @param {string} owner */ -export function ownership_invalid_binding(parent, child, owner) { +export function ownership_invalid_binding(parent, prop, child, owner) { if (DEV) { - console.warn(`%c[svelte] ownership_invalid_binding\n%c${parent} passed a value to ${child} with \`bind:\`, but the value is owned by ${owner}. Consider creating a binding between ${owner} and ${parent}\nhttps://svelte.dev/e/ownership_invalid_binding`, bold, normal); + console.warn(`%c[svelte] ownership_invalid_binding\n%c${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}={...}\`)\nhttps://svelte.dev/e/ownership_invalid_binding`, bold, normal); } else { console.warn(`https://svelte.dev/e/ownership_invalid_binding`); } } /** - * %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead - * @param {string | undefined | null} [component] - * @param {string | undefined | null} [owner] + * %component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead + * @param {string} component + * @param {string} prop + * @param {string} owner */ -export function ownership_invalid_mutation(component, owner) { +export function ownership_invalid_mutation(component, prop, owner) { if (DEV) { - console.warn(`%c[svelte] ownership_invalid_mutation\n%c${component ? `${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 a value outside the component that created it is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead'}\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); + console.warn(`%c[svelte] ownership_invalid_mutation\n%c${component} mutated property \`${prop}\` from parent component ${owner}, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with \`bind:\` (e.g. \`bind:${prop}={...}\` instead of \`${prop}={...}\`), or use a callback instead\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); } else { console.warn(`https://svelte.dev/e/ownership_invalid_mutation`); } diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js index 62c696124285..6f47322cd4fd 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js @@ -10,7 +10,7 @@ export default test({ test({ assert, target, warnings }) { const warning = - 'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead'; + 'Counter.svelte mutated property `object` from parent component main.svelte, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:object={...}` instead of `object={...}`), or use a callback instead'; const [btn1, btn2] = target.querySelectorAll('button'); btn1.click(); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-2/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-2/_config.js index 87474a05cc33..39fa80c55a4e 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-2/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-2/_config.js @@ -8,6 +8,6 @@ export default test({ }, warnings: [ - 'Intermediate.svelte passed a value to Counter.svelte with `bind:`, but the value is owned by main.svelte. Consider creating a binding between main.svelte and Intermediate.svelte' + 'Intermediate.svelte passed property `object` to Counter.svelte with `bind:`, but its parent component main.svelte did not declare `object` as a binding. Consider creating a binding between main.svelte and Intermediate.svelte (e.g. `bind:object={...}` instead of `object={...}`)' ] }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js index 66e51843808e..d70070c58d55 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js @@ -33,7 +33,7 @@ export default test({ assert.htmlEqual(target.innerHTML, ``); assert.deepEqual(warnings, [ - 'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead' + 'Counter.svelte mutated property `notshared` from parent component main.svelte, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:notshared={...}` instead of `notshared={...}`), or use a callback instead' ]); } }); From 4facc705cada95d2517e59cd8706f98832068595 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:04:25 -0400 Subject: [PATCH 06/14] fix --- .../3-transform/client/transform-client.js | 10 +----- .../phases/3-transform/client/utils.js | 33 ++++++++++++++++--- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index c6fc4fc849cf..6cc9d77114bc 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -403,16 +403,8 @@ export function client_component(analysis, options) { // 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')) - ) - ) + b.var('$$ownership_validator', b.call('$.create_ownership_validator', b.id('$$props'))) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index 28e3fabb1990..a37ecd31cc03 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/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; } From 79f9962631b3f071693ed072a4100c6eb8e56c6e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:17:21 -0400 Subject: [PATCH 07/14] hoist --- .../src/internal/client/dev/ownership.js | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 432b5a050ab5..a855950c46ee 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -113,16 +113,6 @@ 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 @@ -130,7 +120,7 @@ export function create_ownership_validator(props) { */ mutation: (path, result) => { const prop_name = path[0]; - if (is_bound(prop_name) || !parent) { + if (is_bound(props, prop_name) || !parent) { return result; } @@ -153,7 +143,7 @@ export function create_ownership_validator(props) { * @param {() => any} value */ binding: (key, child_component, value) => { - if (!is_bound(key) && parent && value()?.[STATE_SYMBOL]) { + if (!is_bound(props, key) && parent && value()?.[STATE_SYMBOL]) { w.ownership_invalid_binding( component[FILENAME], key, @@ -164,3 +154,14 @@ export function create_ownership_validator(props) { } }; } + +/** + * @param {Record} props + * @param {string} prop_name + */ +function is_bound(props, 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; + return !!get_descriptor(props, prop_name)?.set || (is_entry_props && prop_name in props); +} From 6ee9658c44584a0f70da13f57319bae7928f67a9 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:32:07 -0400 Subject: [PATCH 08/14] we only care about mutation, not reassignment --- .../compiler/phases/3-transform/client/visitors/shared/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 2a65ce577dcf..7a20191943ec 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -308,7 +308,7 @@ export function validate_mutation(node, context, expression) { if ( !context.state.options.dev || is_ignored(node, 'ownership_invalid_mutation') || - (left.type !== 'Identifier' && left.type !== 'MemberExpression') + left.type !== 'MemberExpression' ) { return expression; } From 5a291cfd68ea53c66292221a7562af28c56fd390 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:33:15 -0400 Subject: [PATCH 09/14] tidy --- .../phases/3-transform/client/visitors/shared/utils.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 7a20191943ec..07393d0f6c1b 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -7,7 +7,7 @@ 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 { is_ignored, locator } from '../../../../../state.js'; +import { dev, is_ignored, locator } from '../../../../../state.js'; import { create_derived } from '../../utils.js'; /** @@ -305,11 +305,7 @@ export function validate_binding(state, binding, 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 !== 'MemberExpression' - ) { + if (!dev || left.type !== 'MemberExpression' || is_ignored(node, 'ownership_invalid_mutation')) { return expression; } From 9d499e73856d4e93d643703dcad934a847195c7b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:41:01 -0400 Subject: [PATCH 10/14] handle prop aliases --- .../98-reference/.generated/client-warnings.md | 2 +- .../svelte/messages/client-warnings/warnings.md | 2 +- .../3-transform/client/visitors/shared/utils.js | 7 ++++++- .../svelte/src/internal/client/dev/ownership.js | 15 ++++++++------- packages/svelte/src/internal/client/warnings.js | 9 +++++---- 5 files changed, 21 insertions(+), 14 deletions(-) diff --git a/documentation/docs/98-reference/.generated/client-warnings.md b/documentation/docs/98-reference/.generated/client-warnings.md index e8098b170965..4012c551c5c2 100644 --- a/documentation/docs/98-reference/.generated/client-warnings.md +++ b/documentation/docs/98-reference/.generated/client-warnings.md @@ -171,7 +171,7 @@ To fix it, `bind:` to the value instead of just passing a property (i.e. in this ### ownership_invalid_mutation ``` -%component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead +%component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead ``` Consider the following code: diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index 617df1452b5e..539f997c0838 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -140,7 +140,7 @@ To fix it, `bind:` to the value instead of just passing a property (i.e. in this ## ownership_invalid_mutation -> %component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead +> %component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead Consider the following code: diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 07393d0f6c1b..163b8fb3f40c 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -341,5 +341,10 @@ export function validate_mutation(node, context, expression) { path.unshift(b.literal(name.name)); - return b.call('$$ownership_validator.mutation', b.array(path), expression); + return b.call( + '$$ownership_validator.mutation', + b.literal(binding.prop_alias), + b.array(path), + expression + ); } diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index a855950c46ee..9efbd7487fcb 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -115,25 +115,26 @@ export function create_ownership_validator(props) { return { /** + * @param {string} prop * @param {any[]} path * @param {any} result */ - mutation: (path, result) => { - const prop_name = path[0]; - if (is_bound(props, prop_name) || !parent) { + mutation: (prop, path, result) => { + const name = path[0]; + if (is_bound(props, name) || !parent) { return result; } - let prop = props[prop_name]; + let value = props[name]; for (let i = 1; i < path.length - 1; i++) { - if (!prop?.[STATE_SYMBOL]) { + if (!value?.[STATE_SYMBOL]) { return result; } - prop = prop[path[i]]; + value = value[path[i]]; } - w.ownership_invalid_mutation(component[FILENAME], prop_name, parent[FILENAME]); + w.ownership_invalid_mutation(component[FILENAME], name, parent[FILENAME], prop); return result; }, diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index bcb6deaaa2f7..cf90816a4d0a 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -144,14 +144,15 @@ export function ownership_invalid_binding(parent, prop, child, owner) { } /** - * %component% mutated property `%prop%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead + * %component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead * @param {string} component - * @param {string} prop + * @param {string} name * @param {string} owner + * @param {string} prop */ -export function ownership_invalid_mutation(component, prop, owner) { +export function ownership_invalid_mutation(component, name, owner, prop) { if (DEV) { - console.warn(`%c[svelte] ownership_invalid_mutation\n%c${component} mutated property \`${prop}\` from parent component ${owner}, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with \`bind:\` (e.g. \`bind:${prop}={...}\` instead of \`${prop}={...}\`), or use a callback instead\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); + console.warn(`%c[svelte] ownership_invalid_mutation\n%c${component} mutated property \`${name}\` from parent component ${owner}, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with \`bind:\` (e.g. \`bind:${prop}={...}\` instead of \`${prop}={...}\`), or use a callback instead\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); } else { console.warn(`https://svelte.dev/e/ownership_invalid_mutation`); } From 799599638724dd5709dd37395dd241cb172bbbd8 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 12:46:00 -0400 Subject: [PATCH 11/14] mutation validation is only tangentially linked to context requirement --- .../phases/3-transform/client/transform-client.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index 6cc9d77114bc..38fcee8d6fca 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/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 || @@ -402,12 +408,6 @@ 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) { - component_block.body.unshift( - b.var('$$ownership_validator', b.call('$.create_ownership_validator', b.id('$$props'))) - ); - } - component_block.body.unshift(b.stmt(b.call('$.push', ...push_args))); let to_push; From 8b9b6a9793be5fe7e8d5bee69a6089ca19e67cd9 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 13:05:17 -0400 Subject: [PATCH 12/14] no need for two vars, one will do --- .../client/visitors/shared/utils.js | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 163b8fb3f40c..9e4b94fe0771 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -303,7 +303,9 @@ export function validate_binding(state, binding, expression) { * @param {Expression} expression */ export function validate_mutation(node, context, expression) { - const left = node.type === 'AssignmentExpression' ? node.left : node.argument; + 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; @@ -316,27 +318,25 @@ export function validate_mutation(node, context, expression) { if (binding?.kind !== 'prop' && binding?.kind !== 'bindable_prop') return expression; const state = /** @type {ComponentClientTransformState} */ (context.state); - state.analysis.needs_mutation_validation = true; /** @type {Array} */ 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); + + 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(current.property.name)); + path.unshift(b.literal(left.property.name)); } } else { return expression; } - current = current.object; + + left = left.object; } path.unshift(b.literal(name.name)); From d2a5ac4d36b53a87b96758748fb7699587c9ce8e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 14:28:12 -0400 Subject: [PATCH 13/14] update warning, include mutation location --- .../docs/98-reference/.generated/client-warnings.md | 2 +- packages/svelte/messages/client-warnings/warnings.md | 2 +- .../phases/3-transform/client/visitors/shared/utils.js | 6 +++++- packages/svelte/src/internal/client/dev/ownership.js | 9 +++++++-- packages/svelte/src/internal/client/warnings.js | 10 +++++----- packages/svelte/src/utils.js | 6 ++++-- .../samples/non-local-mutation-discouraged/_config.js | 2 +- .../non-local-mutation-with-binding-3/_config.js | 2 +- 8 files changed, 25 insertions(+), 14 deletions(-) diff --git a/documentation/docs/98-reference/.generated/client-warnings.md b/documentation/docs/98-reference/.generated/client-warnings.md index 4012c551c5c2..77d1df4cdde2 100644 --- a/documentation/docs/98-reference/.generated/client-warnings.md +++ b/documentation/docs/98-reference/.generated/client-warnings.md @@ -171,7 +171,7 @@ To fix it, `bind:` to the value instead of just passing a property (i.e. in this ### ownership_invalid_mutation ``` -%component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), 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: diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index 539f997c0838..f8e9ebd8a047 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -140,7 +140,7 @@ To fix it, `bind:` to the value instead of just passing a property (i.e. in this ## ownership_invalid_mutation -> %component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), 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: diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 9e4b94fe0771..af6e56f70c81 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -341,10 +341,14 @@ export function validate_mutation(node, context, expression) { 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 + expression, + loc && b.literal(loc.line), + loc && b.literal(loc.column) ); } diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 9efbd7487fcb..6c40a744dfc5 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -5,6 +5,7 @@ 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'; +import { sanitize_location } from '../../../utils.js'; /** @type {Record>} */ const boundaries = {}; @@ -118,8 +119,10 @@ export function create_ownership_validator(props) { * @param {string} prop * @param {any[]} path * @param {any} result + * @param {number} line + * @param {number} column */ - mutation: (prop, path, result) => { + mutation: (prop, path, result, line, column) => { const name = path[0]; if (is_bound(props, name) || !parent) { return result; @@ -134,7 +137,9 @@ export function create_ownership_validator(props) { value = value[path[i]]; } - w.ownership_invalid_mutation(component[FILENAME], name, parent[FILENAME], prop); + const location = sanitize_location(`${component[FILENAME]}:${line}:${column}`); + + w.ownership_invalid_mutation(name, location, prop, parent[FILENAME]); return result; }, diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index cf90816a4d0a..c84b487e280d 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -144,15 +144,15 @@ export function ownership_invalid_binding(parent, prop, child, owner) { } /** - * %component% mutated property `%name%` from parent component %owner%, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:%prop%={...}` instead of `%prop%={...}`), or use a callback instead - * @param {string} component + * Mutating unbound props (`%name%`, at %location%) is strongly discouraged. Consider using `bind:%prop%={...}` in %parent% (or using a callback) instead * @param {string} name - * @param {string} owner + * @param {string} location * @param {string} prop + * @param {string} parent */ -export function ownership_invalid_mutation(component, name, owner, prop) { +export function ownership_invalid_mutation(name, location, prop, parent) { if (DEV) { - console.warn(`%c[svelte] ownership_invalid_mutation\n%c${component} mutated property \`${name}\` from parent component ${owner}, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with \`bind:\` (e.g. \`bind:${prop}={...}\` instead of \`${prop}={...}\`), or use a callback instead\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); + console.warn(`%c[svelte] ownership_invalid_mutation\n%cMutating unbound props (\`${name}\`, at ${location}) is strongly discouraged. Consider using \`bind:${prop}={...}\` in ${parent} (or using a callback) instead\nhttps://svelte.dev/e/ownership_invalid_mutation`, bold, normal); } else { console.warn(`https://svelte.dev/e/ownership_invalid_mutation`); } diff --git a/packages/svelte/src/utils.js b/packages/svelte/src/utils.js index d4d106d56deb..ada318e85ac7 100644 --- a/packages/svelte/src/utils.js +++ b/packages/svelte/src/utils.js @@ -465,8 +465,10 @@ export function is_raw_text_element(name) { /** * Prevent devtools trying to make `location` a clickable link by inserting a zero-width space - * @param {string | undefined} location + * @template {string | undefined} T + * @param {T} location + * @returns {T}; */ export function sanitize_location(location) { - return location?.replace(/\//g, '/\u200b'); + return /** @type {T} */ (location?.replace(/\//g, '/\u200b')); } diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js index 6f47322cd4fd..84526610260d 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js @@ -10,7 +10,7 @@ export default test({ test({ assert, target, warnings }) { const warning = - 'Counter.svelte mutated property `object` from parent component main.svelte, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:object={...}` instead of `object={...}`), or use a callback instead'; + 'Mutating unbound props (`object`, at Counter.svelte:5:23) is strongly discouraged. Consider using `bind:object={...}` in main.svelte (or using a callback) instead'; const [btn1, btn2] = target.querySelectorAll('button'); btn1.click(); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js index d70070c58d55..7b8cc676d528 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-3/_config.js @@ -33,7 +33,7 @@ export default test({ assert.htmlEqual(target.innerHTML, ``); assert.deepEqual(warnings, [ - 'Counter.svelte mutated property `notshared` from parent component main.svelte, which did not declare it as a binding. This is strongly discouraged. Consider passing props to child components that mutate them with `bind:` (e.g. `bind:notshared={...}` instead of `notshared={...}`), or use a callback instead' + 'Mutating unbound props (`notshared`, at Counter.svelte:10:23) is strongly discouraged. Consider using `bind:notshared={...}` in main.svelte (or using a callback) instead' ]); } }); From 7d4e96f4cc90bcafa7826d8b38c7e5d5db2e5946 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 8 Apr 2025 16:33:06 -0400 Subject: [PATCH 14/14] tweak --- .../phases/3-transform/client/visitors/shared/component.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index 3f12d610edca..2ea68e206e2e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -194,7 +194,7 @@ export function build_component(node, component_name, context, anchor = context. binding_initializers.push( b.stmt( b.call( - b.id('$$ownership_validator.binding'), + '$$ownership_validator.binding', b.literal(binding.node.name), b.id(component_name), b.thunk(expression)