From e98c242d2dc54ddcf7da01ac37d6f8b5dc3389f9 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 14 Mar 2024 18:23:09 +0100 Subject: [PATCH] compile time mutation validation --- packages/svelte/src/compiler/errors.js | 2 ++ .../src/compiler/phases/2-analyze/index.js | 15 +++++++---- .../compiler/phases/2-analyze/validation.js | 27 +++++++++++++------ .../3-transform/client/transform-client.js | 8 +++--- .../phases/3-transform/client/utils.js | 9 ++++--- .../3-transform/client/visitors/global.js | 3 ++- .../client/visitors/javascript-legacy.js | 6 ++--- .../3-transform/client/visitors/template.js | 1 + .../3-transform/server/transform-server.js | 7 ++--- packages/svelte/src/compiler/types/index.d.ts | 4 ++- .../_config.js | 9 +++++++ .../main.svelte | 4 +++ .../samples/each-bind-this-member/main.svelte | 2 +- .../Counter.svelte | 2 +- .../sub.svelte | 3 ++- .../Counter.svelte | 2 +- .../props-bound-fallback/Counter.svelte | 2 +- .../props-default-reactivity/Counter.svelte | 2 +- .../samples/proxy-prop-bound/Counter.svelte | 2 +- 19 files changed, 75 insertions(+), 35 deletions(-) create mode 100644 packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/_config.js create mode 100644 packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/main.svelte diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index 72397e1f6e..b1bc2c5c8d 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -182,6 +182,8 @@ const runes = { `$props() assignment must not contain nested properties or computed keys`, 'invalid-props-location': () => `$props() can only be used at the top level of components as a variable declaration initializer`, + 'invalid-props-mutation': () => + 'Properties defined by $props() cannot be mutated. Use $props.bindable() instead, or make a copy of the value and reassign it.', /** @param {string} rune */ 'invalid-state-location': (rune) => `${rune}(...) can only be used as a variable declaration initializer or a class field`, diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 5f278dc792..05eda25ac5 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -436,7 +436,7 @@ export function analyze_component(root, options) { ); } } else { - instance.scope.declare(b.id('$$props'), 'prop', 'synthetic'); + instance.scope.declare(b.id('$$props'), 'bindable_prop', 'synthetic'); instance.scope.declare(b.id('$$restProps'), 'rest_prop', 'synthetic'); for (const { ast, scope, scopes } of [module, instance, template]) { @@ -466,7 +466,10 @@ export function analyze_component(root, options) { } for (const [name, binding] of instance.scope.declarations) { - if (binding.kind === 'prop' && binding.node.name !== '$$props') { + if ( + (binding.kind === 'prop' || binding.kind === 'bindable_prop') && + binding.node.name !== '$$props' + ) { const references = binding.references.filter( (r) => r.node !== binding.node && r.path.at(-1)?.type !== 'ExportSpecifier' ); @@ -758,7 +761,7 @@ const legacy_scope_tweaker = { (binding.kind === 'normal' && (binding.declaration_kind === 'let' || binding.declaration_kind === 'var')) ) { - binding.kind = 'prop'; + binding.kind = 'bindable_prop'; if (specifier.exported.name !== specifier.local.name) { binding.prop_alias = specifier.exported.name; } @@ -796,7 +799,7 @@ const legacy_scope_tweaker = { for (const declarator of node.declaration.declarations) { for (const id of extract_identifiers(declarator.id)) { const binding = /** @type {import('#compiler').Binding} */ (state.scope.get(id.name)); - binding.kind = 'prop'; + binding.kind = 'bindable_prop'; } } } @@ -871,7 +874,9 @@ const runes_scope_tweaker = { ? 'derived' : path.is_rest ? 'rest_prop' - : 'prop'; + : rune === '$props.bindable' + ? 'bindable_prop' + : 'prop'; } if (rune === '$props' || rune === '$props.bindable') { diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 52e5786824..dc48a10846 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -299,17 +299,19 @@ const validation = { error(node, 'invalid-binding-expression'); } + const binding = context.state.scope.get(left.name); + if ( assignee.type === 'Identifier' && node.name !== 'this' // bind:this also works for regular variables ) { - const binding = context.state.scope.get(left.name); // reassignment if ( !binding || (binding.kind !== 'state' && binding.kind !== 'frozen_state' && binding.kind !== 'prop' && + binding.kind !== 'bindable_prop' && binding.kind !== 'each' && binding.kind !== 'store_sub' && !binding.mutated) @@ -328,7 +330,9 @@ const validation = { // TODO handle mutations of non-state/props in runes mode } - const binding = context.state.scope.get(left.name); + if (assignee.type === 'MemberExpression' && binding?.kind === 'prop') { + error(node, 'invalid-props-mutation'); + } if (node.name === 'group') { if (!binding) { @@ -969,7 +973,9 @@ function validate_no_const_assignment(node, argument, scope, is_binding) { function validate_assignment(node, argument, state) { validate_no_const_assignment(node, argument, state.scope, false); - if (state.analysis.runes && argument.type === 'Identifier') { + if (!state.analysis.runes) return; + + if (argument.type === 'Identifier') { const binding = state.scope.get(argument.name); if (binding?.kind === 'derived') { error(node, 'invalid-derived-assignment'); @@ -978,19 +984,24 @@ function validate_assignment(node, argument, state) { if (binding?.kind === 'each') { error(node, 'invalid-each-assignment'); } + } else if (argument.type === 'MemberExpression') { + const id = object(argument); + if (id && state.scope.get(id.name)?.kind === 'prop') { + error(node, 'invalid-props-mutation'); + } } - let object = /** @type {import('estree').Expression | import('estree').Super} */ (argument); + let obj = /** @type {import('estree').Expression | import('estree').Super} */ (argument); /** @type {import('estree').Expression | import('estree').PrivateIdentifier | null} */ let property = null; - while (object.type === 'MemberExpression') { - property = object.property; - object = object.object; + while (obj.type === 'MemberExpression') { + property = obj.property; + obj = obj.object; } - if (object.type === 'ThisExpression' && property?.type === 'PrivateIdentifier') { + if (obj.type === 'ThisExpression' && property?.type === 'PrivateIdentifier') { if (state.private_derived_state.includes(property.name)) { error(node, 'invalid-derived-assignment'); } 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 3d378df563..4b09a2b302 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 @@ -251,7 +251,8 @@ export function client_component(source, analysis, options) { if (analysis.accessors) { for (const [name, binding] of analysis.instance.scope.declarations) { - if (binding.kind !== 'prop' || name.startsWith('$$')) continue; + if ((binding.kind !== 'prop' && binding.kind !== 'bindable_prop') || name.startsWith('$$')) + continue; const key = binding.prop_alias ?? name; @@ -356,7 +357,7 @@ export function client_component(source, analysis, options) { /** @type {string[]} */ const named_props = analysis.exports.map(({ name, alias }) => alias ?? name); for (const [name, binding] of analysis.instance.scope.declarations) { - if (binding.kind === 'prop') named_props.push(binding.prop_alias ?? name); + if (binding.kind === 'bindable_prop') named_props.push(binding.prop_alias ?? name); } component_block.body.unshift( @@ -464,7 +465,8 @@ export function client_component(source, analysis, options) { const props_str = []; for (const [name, binding] of analysis.instance.scope.declarations) { - if (binding.kind !== 'prop' || name.startsWith('$$')) continue; + if ((binding.kind !== 'prop' && binding.kind !== 'bindable_prop') || name.startsWith('$$')) + continue; const key = binding.prop_alias ?? name; const prop_def = typeof ce === 'boolean' ? {} : ce.props?.[key] || {}; 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 d93f8f5b92..cd274eef23 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -78,7 +78,7 @@ export function serialize_get_binding(node, state) { return typeof binding.expression === 'function' ? binding.expression(node) : binding.expression; } - if (binding.kind === 'prop') { + if (binding.kind === 'prop' || binding.kind === 'bindable_prop') { if (binding.node.name === '$$props') { // Special case for $$props which only exists in the old world // TODO this probably shouldn't have a 'prop' binding kind @@ -377,6 +377,7 @@ export function serialize_set_binding(node, context, fallback, options) { binding.kind !== 'state' && binding.kind !== 'frozen_state' && binding.kind !== 'prop' && + binding.kind !== 'bindable_prop' && binding.kind !== 'each' && binding.kind !== 'legacy_reactive' && !is_store @@ -389,7 +390,7 @@ export function serialize_set_binding(node, context, fallback, options) { const serialize = () => { if (left === node.left) { - if (binding.kind === 'prop') { + if (binding.kind === 'prop' || binding.kind === 'bindable_prop') { return b.call(left, value); } else if (is_store) { return b.call('$.store_set', serialize_get_binding(b.id(left_name), state), value); @@ -467,7 +468,7 @@ export function serialize_set_binding(node, context, fallback, options) { b.call('$.untrack', b.id('$' + left_name)) ); } else if (!state.analysis.runes) { - if (binding.kind === 'prop') { + if (binding.kind === 'bindable_prop') { return b.call( left, b.sequence([ @@ -571,7 +572,7 @@ function get_hoistable_params(node, context) { params.push(b.id(binding.expression.object.arguments[0].name)); } else if ( // If we are referencing a simple $$props value, then we need to reference the object property instead - binding.kind === 'prop' && + (binding.kind === 'prop' || binding.kind === 'bindable_prop') && !binding.reassigned && binding.initial === null && !context.state.analysis.accessors diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js index 5d1689cadc..c299dd99ef 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js @@ -52,6 +52,7 @@ export const global_visitors = { binding?.kind === 'each' || binding?.kind === 'legacy_reactive' || binding?.kind === 'prop' || + binding?.kind === 'bindable_prop' || is_store ) { /** @type {import('estree').Expression[]} */ @@ -64,7 +65,7 @@ export const global_visitors = { fn += '_store'; args.push(serialize_get_binding(b.id(name), state), b.call('$' + name)); } else { - if (binding.kind === 'prop') fn += '_prop'; + if (binding.kind === 'prop' || binding.kind === 'bindable_prop') fn += '_prop'; args.push(b.id(name)); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js index ed4c6e8474..ffb089bf83 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js @@ -40,7 +40,7 @@ export const javascript_visitors_legacy = { state.scope.get_bindings(declarator) ); const has_state = bindings.some((binding) => binding.kind === 'state'); - const has_props = bindings.some((binding) => binding.kind === 'prop'); + const has_props = bindings.some((binding) => binding.kind === 'bindable_prop'); if (!has_state && !has_props) { const init = declarator.init; @@ -80,7 +80,7 @@ export const javascript_visitors_legacy = { declarations.push( b.declarator( path.node, - binding.kind === 'prop' + binding.kind === 'bindable_prop' ? get_prop_source(binding, state, binding.prop_alias ?? name, value) : value ) @@ -168,7 +168,7 @@ export const javascript_visitors_legacy = { // If the binding is a prop, we need to deep read it because it could be fine-grained $state // from a runes-component, where mutations don't trigger an update on the prop as a whole. - if (name === '$$props' || name === '$$restProps' || binding.kind === 'prop') { + if (name === '$$props' || name === '$$restProps' || binding.kind === 'bindable_prop') { serialized = b.call('$.deep_read_state', serialized); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index e5302a3a7c..06ba29c33d 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1367,6 +1367,7 @@ function serialize_event_handler(node, { state, visit }) { binding.kind === 'legacy_reactive' || binding.kind === 'derived' || binding.kind === 'prop' || + binding.kind === 'bindable_prop' || binding.kind === 'store_sub') ) { handler = dynamic_handler(); diff --git a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js index 2173df9fc4..bac1f0929c 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js @@ -446,6 +446,7 @@ function serialize_set_binding(node, context, fallback) { binding.kind !== 'state' && binding.kind !== 'frozen_state' && binding.kind !== 'prop' && + binding.kind !== 'bindable_prop' && binding.kind !== 'each' && binding.kind !== 'legacy_reactive' && !is_store @@ -1131,7 +1132,7 @@ const javascript_visitors_legacy = { state.scope.get_bindings(declarator) ); const has_state = bindings.some((binding) => binding.kind === 'state'); - const has_props = bindings.some((binding) => binding.kind === 'prop'); + const has_props = bindings.some((binding) => binding.kind === 'bindable_prop'); if (!has_state && !has_props) { declarations.push(/** @type {import('estree').VariableDeclarator} */ (visit(declarator))); @@ -2258,7 +2259,7 @@ export function server_component(analysis, options) { /** @type {import('estree').Property[]} */ const props = []; for (const [name, binding] of analysis.instance.scope.declarations) { - if (binding.kind === 'prop' && !name.startsWith('$$')) { + if (binding.kind === 'bindable_prop' && !name.startsWith('$$')) { props.push(b.init(binding.prop_alias ?? name, b.id(name))); } } @@ -2280,7 +2281,7 @@ export function server_component(analysis, options) { /** @type {string[]} */ const named_props = analysis.exports.map(({ name, alias }) => alias ?? name); for (const [name, binding] of analysis.instance.scope.declarations) { - if (binding.kind === 'prop') named_props.push(binding.prop_alias ?? name); + if (binding.kind === 'bindable_prop') named_props.push(binding.prop_alias ?? name); } component_block.body.unshift( diff --git a/packages/svelte/src/compiler/types/index.d.ts b/packages/svelte/src/compiler/types/index.d.ts index 6d09effe93..9f82c014c8 100644 --- a/packages/svelte/src/compiler/types/index.d.ts +++ b/packages/svelte/src/compiler/types/index.d.ts @@ -241,7 +241,8 @@ export interface Binding { node: Identifier; /** * - `normal`: A variable that is not in any way special - * - `prop`: A normal prop (possibly mutated) + * - `prop`: A normal prop (possibly reassigned) + * - `bindable_prop`: A prop one can `bind:` to (possibly reassigned or mutated) * - `rest_prop`: A rest prop * - `state`: A state variable * - `derived`: A derived variable @@ -253,6 +254,7 @@ export interface Binding { kind: | 'normal' | 'prop' + | 'bindable_prop' | 'rest_prop' | 'state' | 'frozen_state' diff --git a/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/_config.js b/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/_config.js new file mode 100644 index 0000000000..5afbe53acd --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/_config.js @@ -0,0 +1,9 @@ +import { test } from '../../test'; + +export default test({ + error: { + code: 'invalid-props-mutation', + message: + 'Properties defined by $props() cannot be mutated. Use $props.bindable() instead, or make a copy of the value and reassign it.' + } +}); diff --git a/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/main.svelte b/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/main.svelte new file mode 100644 index 0000000000..10390e812d --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/runes-non-bindable-prop-mutated/main.svelte @@ -0,0 +1,4 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/each-bind-this-member/main.svelte b/packages/svelte/tests/runtime-runes/samples/each-bind-this-member/main.svelte index a685cc9c84..c66525b3d2 100644 --- a/packages/svelte/tests/runtime-runes/samples/each-bind-this-member/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/each-bind-this-member/main.svelte @@ -1,5 +1,5 @@ {#each items as item, i} diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte index d1be326830..8b132280c6 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte @@ -1,6 +1,6 @@ diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding/Counter.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding/Counter.svelte index d1be326830..8b132280c6 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding/Counter.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding/Counter.svelte @@ -1,6 +1,6 @@