From 3c19e50ac4f9c889cc8ef6b34bc149e6b0ccf9af Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Thu, 11 Jul 2024 20:36:34 +0200 Subject: [PATCH 1/3] fix: detect mutations within assignments expressions This enhances our "variable was mutated" detection to also recognize that `foo` in `[foo[1]] = [..]` was mutated. This allows us to be more granular about detecting mutations to each expressions in legacy mode, which fixes #12169 --- .changeset/thirty-dogs-whisper.md | 5 +++ .../src/compiler/phases/2-analyze/index.js | 32 +++++++++---------- packages/svelte/src/compiler/phases/scope.js | 2 +- packages/svelte/src/compiler/utils/ast.js | 24 ++++++++++---- .../samples/each-block-const/input.svelte | 5 +++ .../samples/each-block-const/output.svelte | 5 +++ 6 files changed, 49 insertions(+), 24 deletions(-) create mode 100644 .changeset/thirty-dogs-whisper.md create mode 100644 packages/svelte/tests/migrate/samples/each-block-const/input.svelte create mode 100644 packages/svelte/tests/migrate/samples/each-block-const/output.svelte diff --git a/.changeset/thirty-dogs-whisper.md b/.changeset/thirty-dogs-whisper.md new file mode 100644 index 000000000000..de10b3ea8c7e --- /dev/null +++ b/.changeset/thirty-dogs-whisper.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: detect mutations within assignment expressions diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index d48fa345612c..b10f21b9cc19 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -769,8 +769,7 @@ const legacy_scope_tweaker = { } if ( - binding !== null && - binding.kind === 'normal' && + binding?.kind === 'normal' && ((binding.scope === state.instance_scope && binding.declaration_kind !== 'function') || binding.declaration_kind === 'import') ) { @@ -795,23 +794,24 @@ const legacy_scope_tweaker = { parent.left === binding.node ) { binding.kind = 'derived'; - } else { - let idx = -1; - let ancestor = path.at(idx); - while (ancestor) { - if (ancestor.type === 'EachBlock') { - // Ensures that the array is reactive when only its entries are mutated - // TODO: this doesn't seem correct. We should be checking at the points where - // the identifier (the each expression) is used in a way that makes it reactive. - // This just forces the collection identifier to always be reactive even if it's - // not. - if (ancestor.expression === (idx === -1 ? node : path.at(idx + 1))) { - binding.kind = 'state'; - break; + } + } else if (binding?.kind === 'each' && binding.mutated) { + // Ensure that the array is marked as reactive even when only its entries are mutated + let idx = -1; + let ancestor = path.at(idx); + while (ancestor) { + if ( + ancestor.type === 'EachBlock' && + state.analysis.template.scopes.get(ancestor)?.declarations.get(node.name) === binding + ) { + for (const reference of ancestor.metadata.references) { + if (reference.kind === 'normal') { + reference.kind = 'state'; } } - ancestor = path.at(--idx); + break; } + ancestor = path.at(--idx); } } } diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index d9a36f4dc73f..bdc18954c08e 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -713,7 +713,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { const binding = scope.get(/** @type {import('estree').Identifier} */ (object).name); if (binding) binding.mutated = true; } else { - extract_identifiers(node).forEach((identifier) => { + extract_identifiers(node, [], true).forEach((identifier) => { const binding = scope.get(identifier.name); if (binding && identifier !== binding.node) { binding.mutated = true; diff --git a/packages/svelte/src/compiler/utils/ast.js b/packages/svelte/src/compiler/utils/ast.js index a820d0da8e81..cc69393f69cd 100644 --- a/packages/svelte/src/compiler/utils/ast.js +++ b/packages/svelte/src/compiler/utils/ast.js @@ -54,23 +54,33 @@ export function is_event_attribute(attribute) { } /** - * Extracts all identifiers from a pattern. + * Extracts all identifiers from a pattern. By default only those which can be newly declared as part of an assignment expression are included. * @param {ESTree.Pattern} param * @param {ESTree.Identifier[]} [nodes] + * @param {boolean} [include_member_expressions] If `true`, will also include member expressions which can be important in a case like `[a, b] = [c, d]` * @returns {ESTree.Identifier[]} */ -export function extract_identifiers(param, nodes = []) { +export function extract_identifiers(param, nodes = [], include_member_expressions = false) { switch (param.type) { case 'Identifier': nodes.push(param); break; + case 'MemberExpression': + if ( + include_member_expressions && + (param.object.type === 'Identifier' || param.object.type === 'MemberExpression') + ) { + extract_identifiers(param.object, nodes, include_member_expressions); + } + break; + case 'ObjectPattern': for (const prop of param.properties) { if (prop.type === 'RestElement') { - extract_identifiers(prop.argument, nodes); + extract_identifiers(prop.argument, nodes, include_member_expressions); } else { - extract_identifiers(prop.value, nodes); + extract_identifiers(prop.value, nodes, include_member_expressions); } } @@ -78,17 +88,17 @@ export function extract_identifiers(param, nodes = []) { case 'ArrayPattern': for (const element of param.elements) { - if (element) extract_identifiers(element, nodes); + if (element) extract_identifiers(element, nodes, include_member_expressions); } break; case 'RestElement': - extract_identifiers(param.argument, nodes); + extract_identifiers(param.argument, nodes, include_member_expressions); break; case 'AssignmentPattern': - extract_identifiers(param.left, nodes); + extract_identifiers(param.left, nodes, include_member_expressions); break; } diff --git a/packages/svelte/tests/migrate/samples/each-block-const/input.svelte b/packages/svelte/tests/migrate/samples/each-block-const/input.svelte new file mode 100644 index 000000000000..d2057d6f1934 --- /dev/null +++ b/packages/svelte/tests/migrate/samples/each-block-const/input.svelte @@ -0,0 +1,5 @@ + + +{#each foo as f}{/each} diff --git a/packages/svelte/tests/migrate/samples/each-block-const/output.svelte b/packages/svelte/tests/migrate/samples/each-block-const/output.svelte new file mode 100644 index 000000000000..d2057d6f1934 --- /dev/null +++ b/packages/svelte/tests/migrate/samples/each-block-const/output.svelte @@ -0,0 +1,5 @@ + + +{#each foo as f}{/each} From 1afcbc60b04f2a93394e396e9cc19d7f89e69f21 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 12 Jul 2024 12:41:08 +0200 Subject: [PATCH 2/3] simplify --- packages/svelte/src/compiler/utils/ast.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/svelte/src/compiler/utils/ast.js b/packages/svelte/src/compiler/utils/ast.js index cc69393f69cd..9435332fd37d 100644 --- a/packages/svelte/src/compiler/utils/ast.js +++ b/packages/svelte/src/compiler/utils/ast.js @@ -67,11 +67,10 @@ export function extract_identifiers(param, nodes = [], include_member_expression break; case 'MemberExpression': - if ( - include_member_expressions && - (param.object.type === 'Identifier' || param.object.type === 'MemberExpression') - ) { - extract_identifiers(param.object, nodes, include_member_expressions); + if (include_member_expressions) { + // Only the `a` from `[a.b[c].d]` is of interest to us here + const id = object(param); + if (id) nodes.push(id); } break; From 9b9a268cc22e7463a6401401d43ac25f0856dd19 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 12 Jul 2024 14:17:30 -0400 Subject: [PATCH 3/3] more idiomatic while loop --- packages/svelte/src/compiler/phases/2-analyze/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index b10f21b9cc19..8c2a60602bca 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -797,9 +797,9 @@ const legacy_scope_tweaker = { } } else if (binding?.kind === 'each' && binding.mutated) { // Ensure that the array is marked as reactive even when only its entries are mutated - let idx = -1; - let ancestor = path.at(idx); - while (ancestor) { + let i = path.length; + while (i--) { + const ancestor = path[i]; if ( ancestor.type === 'EachBlock' && state.analysis.template.scopes.get(ancestor)?.declarations.get(node.name) === binding @@ -811,7 +811,6 @@ const legacy_scope_tweaker = { } break; } - ancestor = path.at(--idx); } } }