From a8dc96eb4345c51efd1e5f0cc02781b18aba322a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 15 Jul 2024 00:08:41 -0700 Subject: [PATCH] fix: detect mutations within assignments expressions (alternative approach) (#12429) 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 --------- Co-authored-by: Simon Holthausen --- .changeset/thirty-dogs-whisper.md | 5 +++ .../src/compiler/phases/2-analyze/index.js | 29 ++++++------- packages/svelte/src/compiler/phases/scope.js | 18 +++++--- packages/svelte/src/compiler/utils/ast.js | 43 +++++++++++++------ .../samples/each-block-const/input.svelte | 5 +++ .../samples/each-block-const/output.svelte | 5 +++ 6 files changed, 71 insertions(+), 34 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 0000000000..de10b3ea8c --- /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 565b2fd5cb..4fefd327c6 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -772,8 +772,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') ) { @@ -798,22 +797,22 @@ 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))) { + } + } else if (binding?.kind === 'each' && binding.mutated) { + // Ensure that the array is marked as reactive even when only its entries are mutated + 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 + ) { + for (const binding of ancestor.metadata.references) { + if (binding.kind === 'normal') { binding.kind = 'state'; - break; } } - ancestor = path.at(--idx); + break; } } } diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index d9a36f4dc7..7b23486d9a 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -3,7 +3,12 @@ import { walk } from 'zimmerframe'; import { is_element_node } from './nodes.js'; import * as b from '../utils/builders.js'; import * as e from '../errors.js'; -import { extract_identifiers, extract_identifiers_from_destructuring } from '../utils/ast.js'; +import { + extract_identifiers, + extract_identifiers_from_destructuring, + object, + unwrap_pattern +} from '../utils/ast.js'; import { JsKeywords, Runes } from './constants.js'; export class Scope { @@ -713,11 +718,14 @@ 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) => { - const binding = scope.get(identifier.name); - if (binding && identifier !== binding.node) { + unwrap_pattern(node).forEach((node) => { + let id = node.type === 'Identifier' ? node : object(node); + if (id === null) return; + + const binding = scope.get(id.name); + if (binding && id !== binding.node) { binding.mutated = true; - binding.reassigned = true; + binding.reassigned = node.type === 'Identifier'; } }); } diff --git a/packages/svelte/src/compiler/utils/ast.js b/packages/svelte/src/compiler/utils/ast.js index a820d0da8e..f122ed5021 100644 --- a/packages/svelte/src/compiler/utils/ast.js +++ b/packages/svelte/src/compiler/utils/ast.js @@ -54,47 +54,62 @@ export function is_event_attribute(attribute) { } /** - * Extracts all identifiers from a pattern. - * @param {ESTree.Pattern} param - * @param {ESTree.Identifier[]} [nodes] - * @returns {ESTree.Identifier[]} + * Extracts all identifiers and member expressions from a pattern. + * @param {ESTree.Pattern} pattern + * @param {Array} [nodes] + * @returns {Array} */ -export function extract_identifiers(param, nodes = []) { - switch (param.type) { +export function unwrap_pattern(pattern, nodes = []) { + switch (pattern.type) { case 'Identifier': - nodes.push(param); + nodes.push(pattern); + break; + + case 'MemberExpression': + // member expressions can be part of an assignment pattern, but not a binding pattern + // see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#binding_and_assignment + nodes.push(pattern); break; case 'ObjectPattern': - for (const prop of param.properties) { + for (const prop of pattern.properties) { if (prop.type === 'RestElement') { - extract_identifiers(prop.argument, nodes); + unwrap_pattern(prop.argument, nodes); } else { - extract_identifiers(prop.value, nodes); + unwrap_pattern(prop.value, nodes); } } break; case 'ArrayPattern': - for (const element of param.elements) { - if (element) extract_identifiers(element, nodes); + for (const element of pattern.elements) { + if (element) unwrap_pattern(element, nodes); } break; case 'RestElement': - extract_identifiers(param.argument, nodes); + unwrap_pattern(pattern.argument, nodes); break; case 'AssignmentPattern': - extract_identifiers(param.left, nodes); + unwrap_pattern(pattern.left, nodes); break; } return nodes; } +/** + * Extracts all identifiers from a pattern. + * @param {ESTree.Pattern} pattern + * @returns {ESTree.Identifier[]} + */ +export function extract_identifiers(pattern) { + return unwrap_pattern(pattern, []).filter((node) => node.type === 'Identifier'); +} + /** * Extracts all identifiers and a stringified keypath from an expression. * @param {ESTree.Expression} expr 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 0000000000..d2057d6f19 --- /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 0000000000..d2057d6f19 --- /dev/null +++ b/packages/svelte/tests/migrate/samples/each-block-const/output.svelte @@ -0,0 +1,5 @@ + + +{#each foo as f}{/each}