From 45532ce085c7d18adce594a25d2940876d6e27cd Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 8 Feb 2024 10:46:57 +0000 Subject: [PATCH 1/7] fix: improve code generation for mutation to each block reference --- .changeset/violet-pigs-jam.md | 5 ++++ .../src/compiler/phases/2-analyze/index.js | 24 +++++++++++++------ .../samples/each-mutation-3/_config.js | 17 +++++++++++++ .../samples/each-mutation-3/main.svelte | 7 ++++++ 4 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 .changeset/violet-pigs-jam.md create mode 100644 packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte diff --git a/.changeset/violet-pigs-jam.md b/.changeset/violet-pigs-jam.md new file mode 100644 index 000000000000..b10457440a8c --- /dev/null +++ b/.changeset/violet-pigs-jam.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve code generation for mutation to each block reference diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index a7d4a9180ab0..e60abec0eb87 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -193,13 +193,23 @@ function get_delegated_event(event_name, handler, context) { binding !== null && // Bail-out if the the binding is a rest param (binding.declaration_kind === 'rest_param' || - // Bail-out if we reference anything from the EachBlock (for now) that mutates in non-runes mode, - (((!context.state.analysis.runes && binding.kind === 'each') || - // or any normal not reactive bindings that are mutated. - binding.kind === 'normal' || - // or any reactive imports (those are rewritten) (can only happen in legacy mode) - binding.kind === 'legacy_reactive_import') && - binding.mutated)) + // Bail-out if we reference anything from the EachBlock (for now) that mutates. + (binding.kind === 'each' && + // We also need to ensure that if the each binding is mutated, that the mutation occurs on + // the identifier itself, not some member expression that the binding is part of. + binding.references.some(({ path }) => { + const last = path.at(-1); + + return ( + last != null && + (last.type === 'AssignmentExpression' || last.type === 'UpdateExpression') + ); + })) || + // or any normal not reactive bindings that are mutated. + binding.kind === 'normal' || + // or any reactive imports (those are rewritten) (can only happen in legacy mode) + binding.kind === 'legacy_reactive_import') && + binding.mutated ) { return non_hoistable; } diff --git a/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js b/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js new file mode 100644 index 000000000000..610f9b00dbb0 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js @@ -0,0 +1,17 @@ +import { test } from '../../test'; + +export default test({ + html: ``, + + async test({ assert, target }) { + const [btn1] = target.querySelectorAll('button'); + + await btn1.click(); + await Promise.resolve(); + + assert.htmlEqual( + target.innerHTML, + `` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte new file mode 100644 index 000000000000..f9744c08de8a --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte @@ -0,0 +1,7 @@ + + +{#each arr as value} + +{/each} From cbef25c507bb5b0cd85e075606a25d15b95f4685 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 8 Feb 2024 12:48:05 +0000 Subject: [PATCH 2/7] fix: add compiler error for each block mutations in runes mode --- .changeset/violet-pigs-jam.md | 2 +- packages/svelte/src/compiler/errors.js | 3 +- .../src/compiler/phases/2-analyze/index.js | 30 ++++++++++++------- .../runes-invalid-each-mutation/_config.js | 8 +++++ .../runes-invalid-each-mutation}/main.svelte | 0 .../samples/each-mutation-3/_config.js | 17 ----------- 6 files changed, 30 insertions(+), 30 deletions(-) create mode 100644 packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js rename packages/svelte/tests/{runtime-runes/samples/each-mutation-3 => compiler-errors/samples/runes-invalid-each-mutation}/main.svelte (100%) delete mode 100644 packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js diff --git a/.changeset/violet-pigs-jam.md b/.changeset/violet-pigs-jam.md index b10457440a8c..260cbbfae183 100644 --- a/.changeset/violet-pigs-jam.md +++ b/.changeset/violet-pigs-jam.md @@ -2,4 +2,4 @@ "svelte": patch --- -fix: improve code generation for mutation to each block reference +fix: add compiler error for each block mutations in runes mode diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index 4d6b91c007ed..0632cfa31efe 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -202,7 +202,8 @@ const runes = { }`, /** @param {string} name */ 'invalid-runes-mode-import': (name) => `${name} cannot be used in runes mode`, - 'duplicate-props-rune': () => `Cannot use $props() more than once` + 'duplicate-props-rune': () => `Cannot use $props() more than once`, + 'invalid-each-mutation': () => `Cannot mutate each block reference directly in runes mode` }; /** @satisfies {Errors} */ diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index e60abec0eb87..f1580469dc59 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -189,22 +189,30 @@ function get_delegated_event(event_name, handler, context) { return non_hoistable; } + const is_runes = context.state.analysis.runes; + + // Validate that we don't have any each bindings that are mutated. + if (is_runes && binding !== null && binding.kind === 'each') { + for (const { path } of binding.references) { + const last = path.at(-1); + + // If the last reference node is an update or assigment expression to the binding + // then we know the binding was mutated directly rather than part of another node path. + if ( + last != null && + (last.type === 'AssignmentExpression' || last.type === 'UpdateExpression') + ) { + error(last, 'invalid-each-mutation'); + } + } + } + if ( binding !== null && // Bail-out if the the binding is a rest param (binding.declaration_kind === 'rest_param' || // Bail-out if we reference anything from the EachBlock (for now) that mutates. - (binding.kind === 'each' && - // We also need to ensure that if the each binding is mutated, that the mutation occurs on - // the identifier itself, not some member expression that the binding is part of. - binding.references.some(({ path }) => { - const last = path.at(-1); - - return ( - last != null && - (last.type === 'AssignmentExpression' || last.type === 'UpdateExpression') - ); - })) || + (!is_runes && binding.kind === 'each') || // or any normal not reactive bindings that are mutated. binding.kind === 'normal' || // or any reactive imports (those are rewritten) (can only happen in legacy mode) diff --git a/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js new file mode 100644 index 000000000000..baca047e1cb0 --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js @@ -0,0 +1,8 @@ +import { test } from '../../test'; + +export default test({ + error: { + code: 'invalid-each-mutation', + message: 'Cannot mutate each block reference directly in runes mode' + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/main.svelte similarity index 100% rename from packages/svelte/tests/runtime-runes/samples/each-mutation-3/main.svelte rename to packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js b/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js deleted file mode 100644 index 610f9b00dbb0..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/each-mutation-3/_config.js +++ /dev/null @@ -1,17 +0,0 @@ -import { test } from '../../test'; - -export default test({ - html: ``, - - async test({ assert, target }) { - const [btn1] = target.querySelectorAll('button'); - - await btn1.click(); - await Promise.resolve(); - - assert.htmlEqual( - target.innerHTML, - `` - ); - } -}); From 09f6519ea61c23a1b2c930fcf19de648fd90975d Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 8 Feb 2024 12:49:18 +0000 Subject: [PATCH 3/7] lint --- packages/svelte/src/compiler/phases/2-analyze/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index f1580469dc59..4c6ca0fd42fd 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -211,7 +211,7 @@ function get_delegated_event(event_name, handler, context) { binding !== null && // Bail-out if the the binding is a rest param (binding.declaration_kind === 'rest_param' || - // Bail-out if we reference anything from the EachBlock (for now) that mutates. + // Bail-out if we reference anything from the EachBlock (for now) that mutates in non-runes mode, (!is_runes && binding.kind === 'each') || // or any normal not reactive bindings that are mutated. binding.kind === 'normal' || From 9237c666659a7a2bbba132e708e40a9106a8b6e8 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 8 Feb 2024 12:50:02 +0000 Subject: [PATCH 4/7] lint --- .../svelte/src/compiler/phases/2-analyze/index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 4c6ca0fd42fd..6e036febf340 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -212,12 +212,12 @@ function get_delegated_event(event_name, handler, context) { // Bail-out if the the binding is a rest param (binding.declaration_kind === 'rest_param' || // Bail-out if we reference anything from the EachBlock (for now) that mutates in non-runes mode, - (!is_runes && binding.kind === 'each') || - // or any normal not reactive bindings that are mutated. - binding.kind === 'normal' || - // or any reactive imports (those are rewritten) (can only happen in legacy mode) - binding.kind === 'legacy_reactive_import') && - binding.mutated + (((!is_runes && binding.kind === 'each') || + // or any normal not reactive bindings that are mutated. + binding.kind === 'normal' || + // or any reactive imports (those are rewritten) (can only happen in legacy mode) + binding.kind === 'legacy_reactive_import') && + binding.mutated)) ) { return non_hoistable; } From 39d7c3e5f6c50b685e01bb90883ed36f27937489 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 8 Feb 2024 13:00:19 +0000 Subject: [PATCH 5/7] lint --- packages/svelte/src/compiler/errors.js | 2 +- .../samples/runes-invalid-each-mutation/_config.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index 0632cfa31efe..52f0aa491daf 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -203,7 +203,7 @@ const runes = { /** @param {string} name */ 'invalid-runes-mode-import': (name) => `${name} cannot be used in runes mode`, 'duplicate-props-rune': () => `Cannot use $props() more than once`, - 'invalid-each-mutation': () => `Cannot mutate each block reference directly in runes mode` + 'invalid-each-mutation': () => `Cannot mutate each block references directly in runes mode` }; /** @satisfies {Errors} */ diff --git a/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js index baca047e1cb0..abd77ebe653c 100644 --- a/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js +++ b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js @@ -3,6 +3,6 @@ import { test } from '../../test'; export default test({ error: { code: 'invalid-each-mutation', - message: 'Cannot mutate each block reference directly in runes mode' + message: 'Cannot mutate each block references directly in runes mode' } }); From 5bfceb9f95605611946f48ae520611df7cfb3817 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 8 Feb 2024 12:12:33 -0500 Subject: [PATCH 6/7] use existing validate_assignment logic --- .../src/compiler/phases/2-analyze/index.js | 20 +---------- .../compiler/phases/2-analyze/validation.js | 34 ++++++++++--------- 2 files changed, 19 insertions(+), 35 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 6e036febf340..a7d4a9180ab0 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -189,30 +189,12 @@ function get_delegated_event(event_name, handler, context) { return non_hoistable; } - const is_runes = context.state.analysis.runes; - - // Validate that we don't have any each bindings that are mutated. - if (is_runes && binding !== null && binding.kind === 'each') { - for (const { path } of binding.references) { - const last = path.at(-1); - - // If the last reference node is an update or assigment expression to the binding - // then we know the binding was mutated directly rather than part of another node path. - if ( - last != null && - (last.type === 'AssignmentExpression' || last.type === 'UpdateExpression') - ) { - error(last, 'invalid-each-mutation'); - } - } - } - if ( binding !== null && // Bail-out if the the binding is a rest param (binding.declaration_kind === 'rest_param' || // Bail-out if we reference anything from the EachBlock (for now) that mutates in non-runes mode, - (((!is_runes && binding.kind === 'each') || + (((!context.state.analysis.runes && binding.kind === 'each') || // or any normal not reactive bindings that are mutated. binding.kind === 'normal' || // or any reactive imports (those are rewritten) (can only happen in legacy mode) diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 6a1aaf0fecce..cbd8a354bb25 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -334,6 +334,9 @@ function is_tag_valid_with_parent(tag, parent_tag) { * @type {import('zimmerframe').Visitors} */ const validation = { + AssignmentExpression(node, context) { + validate_assignment(node, node.left, context.state); + }, BindDirective(node, context) { validate_no_const_assignment(node, node.expression, context.state.scope, true); @@ -655,6 +658,9 @@ const validation = { error(child, 'invalid-title-content'); } }, + UpdateExpression(node, context) { + validate_assignment(node, node.argument, context.state); + }, ExpressionTag(node, context) { if (!node.parent) return; if (context.state.parent_element) { @@ -904,24 +910,28 @@ 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); - let left = /** @type {import('estree').Expression | import('estree').Super} */ (argument); - - if (left.type === 'Identifier') { - const binding = state.scope.get(left.name); + if (state.analysis.runes && argument.type === 'Identifier') { + const binding = state.scope.get(argument.name); if (binding?.kind === 'derived') { error(node, 'invalid-derived-assignment'); } + + if (binding?.kind === 'each') { + error(node, 'invalid-each-mutation'); + } } + let object = /** @type {import('estree').Expression | import('estree').Super} */ (argument); + /** @type {import('estree').Expression | import('estree').PrivateIdentifier | null} */ let property = null; - while (left.type === 'MemberExpression') { - property = left.property; - left = left.object; + while (object.type === 'MemberExpression') { + property = object.property; + object = object.object; } - if (left.type === 'ThisExpression' && property?.type === 'PrivateIdentifier') { + if (object.type === 'ThisExpression' && property?.type === 'PrivateIdentifier') { if (state.private_derived_state.includes(property.name)) { error(node, 'invalid-derived-assignment'); } @@ -929,14 +939,6 @@ function validate_assignment(node, argument, state) { } export const validation_runes = merge(validation, a11y_validators, { - AssignmentExpression(node, { state, path }) { - const parent = path.at(-1); - if (parent && parent.type === 'ConstTag') return; - validate_assignment(node, node.left, state); - }, - UpdateExpression(node, { state }) { - validate_assignment(node, node.argument, state); - }, LabeledStatement(node, { path }) { if (node.label.name !== '$' || path.at(-1)?.type !== 'Program') return; error(node, 'invalid-legacy-reactive-statement'); From 9f6bf0dd8cab2f018cbb5fbd6061f9d432ece70f Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 8 Feb 2024 12:14:24 -0500 Subject: [PATCH 7/7] assignment, not mutation --- packages/svelte/src/compiler/errors.js | 2 +- packages/svelte/src/compiler/phases/2-analyze/validation.js | 2 +- .../samples/runes-invalid-each-mutation/_config.js | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index 52f0aa491daf..8b5c8a4aeef3 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -203,7 +203,7 @@ const runes = { /** @param {string} name */ 'invalid-runes-mode-import': (name) => `${name} cannot be used in runes mode`, 'duplicate-props-rune': () => `Cannot use $props() more than once`, - 'invalid-each-mutation': () => `Cannot mutate each block references directly in runes mode` + 'invalid-each-assignment': () => `Cannot reassign each block argument in runes mode` }; /** @satisfies {Errors} */ diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index cbd8a354bb25..11fa80761a85 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -917,7 +917,7 @@ function validate_assignment(node, argument, state) { } if (binding?.kind === 'each') { - error(node, 'invalid-each-mutation'); + error(node, 'invalid-each-assignment'); } } diff --git a/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js index abd77ebe653c..eee6ca5c8d4f 100644 --- a/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js +++ b/packages/svelte/tests/compiler-errors/samples/runes-invalid-each-mutation/_config.js @@ -2,7 +2,7 @@ import { test } from '../../test'; export default test({ error: { - code: 'invalid-each-mutation', - message: 'Cannot mutate each block references directly in runes mode' + code: 'invalid-each-assignment', + message: 'Cannot reassign each block argument in runes mode' } });