From 19beb7754e82dd40052499d8c253e1fe2bb69cf8 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 14 Aug 2024 15:46:09 -0400 Subject: [PATCH] chore: set `binding.kind` before analysis (#12843) * analyse exports before walking * more * another * this is unused * move stuff/tidy up * this appears to be unnecessary * this is all unnecessary * simplify * simplify * simplify * simplify * move more stuff over * changeset * unused * separate reassignment from mutation * regenerate * lint --- .changeset/pink-countries-repair.md | 5 + packages/svelte/src/compiler/migrate/index.js | 7 +- .../src/compiler/phases/2-analyze/index.js | 115 +++++++++++++++++- .../phases/2-analyze/visitors/Attribute.js | 4 +- .../2-analyze/visitors/BindDirective.js | 6 +- .../visitors/ExportNamedDeclaration.js | 31 ----- .../2-analyze/visitors/ExportSpecifier.js | 24 +--- .../phases/2-analyze/visitors/Identifier.js | 41 +------ .../2-analyze/visitors/StyleDirective.js | 4 - .../phases/3-transform/client/utils.js | 2 +- .../3-transform/client/visitors/EachBlock.js | 6 +- packages/svelte/src/compiler/phases/scope.js | 42 ++----- packages/svelte/src/compiler/types/index.d.ts | 2 + .../svelte/src/compiler/types/template.d.ts | 2 - packages/svelte/types/index.d.ts | 4 +- 15 files changed, 143 insertions(+), 152 deletions(-) create mode 100644 .changeset/pink-countries-repair.md diff --git a/.changeset/pink-countries-repair.md b/.changeset/pink-countries-repair.md new file mode 100644 index 0000000000..faef646ba1 --- /dev/null +++ b/.changeset/pink-countries-repair.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +chore: set `binding.kind` before analysis diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js index cc5344e5c2..31aee08cfa 100644 --- a/packages/svelte/src/compiler/migrate/index.js +++ b/packages/svelte/src/compiler/migrate/index.js @@ -331,10 +331,7 @@ const instance_script = { const binding = /** @type {Compiler.Binding} */ (state.scope.get(declarator.id.name)); - if ( - state.analysis.uses_props && - (declarator.init || binding.mutated || binding.reassigned) - ) { + if (state.analysis.uses_props && (declarator.init || binding.updated)) { throw new Error( '$$props is used together with named props in a way that cannot be automatically migrated.' ); @@ -350,7 +347,7 @@ const instance_script = { ) : '', optional: !!declarator.init, - bindable: binding.mutated || binding.reassigned, + bindable: binding.updated, ...extract_type_and_comment(declarator, state.str, path) }); state.props_insertion_point = /** @type {number} */ (declarator.end); diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 199ca5c1af..cd4efe6760 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -1,13 +1,13 @@ -/** @import { Node, Program } from 'estree' */ -/** @import { Root, Script, SvelteNode, ValidatedCompileOptions, ValidatedModuleCompileOptions } from '#compiler' */ +/** @import { Expression, Node, Program } from 'estree' */ +/** @import { Binding, Root, Script, SvelteNode, ValidatedCompileOptions, ValidatedModuleCompileOptions } from '#compiler' */ /** @import { AnalysisState, Visitors } from './types' */ /** @import { Analysis, ComponentAnalysis, Js, ReactiveStatement, Template } from '../types' */ import { walk } from 'zimmerframe'; import * as e from '../../errors.js'; import * as w from '../../warnings.js'; -import { is_text_attribute } from '../../utils/ast.js'; +import { extract_identifiers, is_text_attribute } from '../../utils/ast.js'; import * as b from '../../utils/builders.js'; -import { Scope, ScopeRoot, create_scopes, get_rune } from '../scope.js'; +import { Scope, ScopeRoot, create_scopes, get_rune, set_scope } from '../scope.js'; import check_graph_for_cycles from './utils/check_graph_for_cycles.js'; import { create_attribute } from '../nodes.js'; import { analyze_css } from './css/css-analyze.js'; @@ -62,6 +62,7 @@ import { Text } from './visitors/Text.js'; import { TitleElement } from './visitors/TitleElement.js'; import { UpdateExpression } from './visitors/UpdateExpression.js'; import { VariableDeclarator } from './visitors/VariableDeclarator.js'; +import is_reference from 'is-reference'; /** * @type {Visitors} @@ -397,6 +398,112 @@ export function analyze_component(root, source, options) { source }; + if (!runes) { + // every exported `let` or `var` declaration becomes a prop, everything else becomes an export + for (const node of instance.ast.body) { + if (node.type !== 'ExportNamedDeclaration') continue; + + analysis.needs_props = true; + + if (node.declaration) { + if ( + node.declaration.type === 'FunctionDeclaration' || + node.declaration.type === 'ClassDeclaration' + ) { + analysis.exports.push({ + name: /** @type {import('estree').Identifier} */ (node.declaration.id).name, + alias: null + }); + } else if (node.declaration.type === 'VariableDeclaration') { + if (node.declaration.kind === 'const') { + for (const declarator of node.declaration.declarations) { + for (const node of extract_identifiers(declarator.id)) { + analysis.exports.push({ name: node.name, alias: null }); + } + } + } else { + for (const declarator of node.declaration.declarations) { + for (const id of extract_identifiers(declarator.id)) { + const binding = /** @type {Binding} */ (instance.scope.get(id.name)); + binding.kind = 'bindable_prop'; + } + } + } + } + } else { + for (const specifier of node.specifiers) { + const binding = instance.scope.get(specifier.local.name); + + if ( + binding && + (binding.declaration_kind === 'var' || binding.declaration_kind === 'let') + ) { + binding.kind = 'bindable_prop'; + + if (specifier.exported.name !== specifier.local.name) { + binding.prop_alias = specifier.exported.name; + } + } else { + analysis.exports.push({ name: specifier.local.name, alias: specifier.exported.name }); + } + } + } + } + + // if reassigned/mutated bindings are referenced in `$:` blocks + // or the template, turn them into state + for (const binding of instance.scope.declarations.values()) { + if (binding.kind !== 'normal') continue; + + for (const { node, path } of binding.references) { + if (node === binding.node) continue; + + if (binding.updated) { + if ( + path[path.length - 1].type === 'StyleDirective' || + path.some((node) => node.type === 'Fragment') || + (path[1].type === 'LabeledStatement' && path[1].label.name === '$') + ) { + binding.kind = 'state'; + } + } + } + } + + // more legacy nonsense: if an `each` binding is reassigned/mutated, + // treat the expression as being mutated as well + walk(/** @type {SvelteNode} */ (template.ast), null, { + EachBlock(node) { + const scope = /** @type {Scope} */ (template.scopes.get(node)); + + for (const binding of scope.declarations.values()) { + if (binding.updated) { + const state = { scope: /** @type {Scope} */ (scope.parent), scopes: template.scopes }; + + walk(node.expression, state, { + // @ts-expect-error + _: set_scope, + Identifier(node, context) { + const parent = /** @type {Expression} */ (context.path.at(-1)); + + if (is_reference(node, parent)) { + const binding = context.state.scope.get(node.name); + + if (binding && binding.kind === 'normal') { + binding.kind = 'state'; + binding.mutated = binding.updated = true; + } + } + } + }); + + break; + } + } + } + }); + } + if (root.options) { for (const attribute of root.options.attributes) { if (attribute.name === 'accessors') { diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js index 102091c78d..6923bb5aac 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js @@ -134,7 +134,7 @@ function get_delegated_event(event_name, handler, context) { return unhoisted; } - if (binding !== null && binding.initial !== null && !binding.mutated && !binding.is_called) { + if (binding !== null && binding.initial !== null && !binding.updated && !binding.is_called) { const binding_type = binding.initial.type; if ( @@ -188,7 +188,7 @@ function get_delegated_event(event_name, handler, context) { (((!context.state.analysis.runes && binding.kind === 'each') || // or any normal not reactive bindings that are mutated. binding.kind === 'normal') && - binding.mutated)) + binding.updated)) ) { return unhoisted; } diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js index c1557ff600..573616b815 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js @@ -39,7 +39,7 @@ export function BindDirective(node, context) { binding.kind !== 'bindable_prop' && binding.kind !== 'each' && binding.kind !== 'store_sub' && - !binding.mutated)) + !binding.updated)) // TODO wut? ) { e.bind_invalid_value(node.expression); } @@ -80,10 +80,6 @@ export function BindDirective(node, context) { if (references.length > 0) { parent.metadata.contains_group_binding = true; - for (const binding of parent.metadata.references) { - binding.mutated = true; - } - each_blocks.push(parent); ids = ids.filter((id) => !references.includes(id)); ids.push(...extract_all_identifiers_from_expression(parent.expression)[1]); diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportNamedDeclaration.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportNamedDeclaration.js index e50de112e0..547f6ab9c7 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportNamedDeclaration.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportNamedDeclaration.js @@ -39,37 +39,6 @@ export function ExportNamedDeclaration(node, context) { } } - if (context.state.ast_type === 'instance' && !context.state.analysis.runes) { - context.state.analysis.needs_props = true; - - if (node.declaration) { - if ( - node.declaration.type === 'FunctionDeclaration' || - node.declaration.type === 'ClassDeclaration' - ) { - context.state.analysis.exports.push({ - name: /** @type {Identifier} */ (node.declaration.id).name, - alias: null - }); - } else if (node.declaration.type === 'VariableDeclaration') { - if (node.declaration.kind === 'const') { - for (const declarator of node.declaration.declarations) { - for (const node of extract_identifiers(declarator.id)) { - context.state.analysis.exports.push({ name: node.name, alias: null }); - } - } - } else { - for (const declarator of node.declaration.declarations) { - for (const id of extract_identifiers(declarator.id)) { - const binding = /** @type {Binding} */ (context.state.scope.get(id.name)); - binding.kind = 'bindable_prop'; - } - } - } - } - } - } - if (context.state.analysis.runes) { if (node.declaration && context.state.ast_type === 'instance') { if ( diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportSpecifier.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportSpecifier.js index 320b81d086..27eb39acbf 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportSpecifier.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExportSpecifier.js @@ -17,29 +17,7 @@ export function ExportSpecifier(node, context) { }); const binding = context.state.scope.get(node.local.name); - if (binding) binding.reassigned = true; - } else { - context.state.analysis.needs_props = true; - - const binding = /** @type {Binding} */ (context.state.scope.get(node.local.name)); - - if ( - binding !== null && - (binding.kind === 'state' || - binding.kind === 'raw_state' || - (binding.kind === 'normal' && - (binding.declaration_kind === 'let' || binding.declaration_kind === 'var'))) - ) { - binding.kind = 'bindable_prop'; - if (node.exported.name !== node.local.name) { - binding.prop_alias = node.exported.name; - } - } else { - context.state.analysis.exports.push({ - name: node.local.name, - alias: node.exported.name - }); - } + if (binding) binding.reassigned = binding.updated = true; } } else { validate_export(node, context.state.scope, node.local.name); diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js index 7fb819bc17..557206a22b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js @@ -1,4 +1,5 @@ /** @import { Expression, Identifier } from 'estree' */ +/** @import { EachBlock } from '#compiler' */ /** @import { Context } from '../types' */ import is_reference from 'is-reference'; import { should_proxy } from '../../3-transform/client/utils.js'; @@ -77,46 +78,6 @@ export function Identifier(node, context) { if (node.name === '$$restProps') { context.state.analysis.uses_rest_props = true; } - - if ( - binding?.kind === 'normal' && - ((binding.scope === context.state.instance_scope && - binding.declaration_kind !== 'function') || - binding.declaration_kind === 'import') - ) { - if ( - binding.declaration_kind !== 'import' && - binding.mutated && - // TODO could be more fine-grained - not every mention in the template implies a state binding - (context.state.reactive_statement || context.state.ast_type === 'template') - ) { - binding.kind = 'state'; - } else if ( - context.state.reactive_statement && - parent.type === 'AssignmentExpression' && - parent.left === binding.node - ) { - binding.kind = 'derived'; - } - } else if (binding?.kind === 'each' && binding.mutated) { - // Ensure that the array is marked as reactive even when only its entries are mutated - let i = context.path.length; - while (i--) { - const ancestor = context.path[i]; - if ( - ancestor.type === 'EachBlock' && - context.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; - } - } - } } if (binding) { diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/StyleDirective.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/StyleDirective.js index 288a9a61f4..ca9c766e32 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/StyleDirective.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/StyleDirective.js @@ -17,10 +17,6 @@ export function StyleDirective(node, context) { let binding = context.state.scope.get(node.name); if (binding) { - if (!context.state.analysis.runes && binding.mutated) { - binding.kind = 'state'; - } - if (binding.kind !== 'normal') { node.metadata.expression.has_state = true; } 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 333f5ebc1c..965b8d80a5 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -235,7 +235,7 @@ export function is_prop_source(binding, state) { binding.initial || // Until legacy mode is gone, we also need to use the prop source when only mutated is true, // because the parent could be a legacy component which needs coarse-grained reactivity - binding.mutated) + binding.updated) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js index c0d06bf928..9ed168f458 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js @@ -114,7 +114,7 @@ export function EachBlock(node, context) { const indirect_dependencies = collect_parent_each_blocks(context).flatMap((block) => { const array = /** @type {Expression} */ (context.visit(block.expression)); const transitive_dependencies = build_transitive_dependencies( - block.metadata.references, + block.metadata.expression.dependencies, context ); return [array, ...transitive_dependencies]; @@ -126,7 +126,7 @@ export function EachBlock(node, context) { indirect_dependencies.push(collection); const transitive_dependencies = build_transitive_dependencies( - each_node_meta.references, + each_node_meta.expression.dependencies, context ); indirect_dependencies.push(...transitive_dependencies); @@ -279,7 +279,7 @@ function collect_parent_each_blocks(context) { } /** - * @param {Binding[]} references + * @param {Set} references * @param {ComponentContext} context */ function build_transitive_dependencies(references, context) { diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index f9382a21b6..dba086b51f 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -113,13 +113,14 @@ export class Scope { references: [], legacy_dependencies: [], initial, + reassigned: false, mutated: false, + updated: false, scope: this, kind, declaration_kind, is_called: false, prop_alias: null, - reassigned: false, metadata: null }; this.declarations.set(node.name, binding); @@ -506,17 +507,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { }, EachBlock(node, { state, visit }) { - // Array part is still from the scope above - /** @type {Set} */ - const references_within = new Set(); - const idx = references.length; visit(node.expression); - for (let i = idx; i < references.length; i++) { - const [scope, { node: id }] = references[i]; - if (scope === state.scope) { - references_within.add(id); - } - } // context and children are a new scope const scope = state.scope.child(); @@ -582,9 +573,6 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { index: scope.root.unique('$$index'), item: node.context.type === 'Identifier' ? node.context : b.id('$$item'), declarations: scope.declarations, - references: [...references_within] - .map((id) => /** @type {Binding} */ (state.scope.get(id.name))) - .filter(Boolean), is_controlled: false }; }, @@ -671,7 +659,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { // the css property StyleDirective(node, { path, state, next }) { if (node.value === true) { - state.scope.reference(b.id(node.name), path); + state.scope.reference(b.id(node.name), path.concat(node)); } next(); } @@ -693,25 +681,19 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { } for (const [scope, node] of updates) { - if (node.type === 'MemberExpression') { - let object = node.object; - while (object.type === 'MemberExpression') { - object = object.object; - } + for (const expression of unwrap_pattern(node)) { + const left = object(expression); + const binding = left && scope.get(left.name); - const binding = scope.get(/** @type {Identifier} */ (object).name); - if (binding) binding.mutated = true; - } else { - unwrap_pattern(node).forEach((node) => { - let id = node.type === 'Identifier' ? node : object(node); - if (id === null) return; + if (binding !== null && left !== binding.node) { + binding.updated = true; - const binding = scope.get(id.name); - if (binding && id !== binding.node) { + if (left === expression) { + binding.reassigned = true; + } else { binding.mutated = true; - binding.reassigned = node.type === 'Identifier'; } - }); + } } } diff --git a/packages/svelte/src/compiler/types/index.d.ts b/packages/svelte/src/compiler/types/index.d.ts index 24da8c9b84..d893f62b33 100644 --- a/packages/svelte/src/compiler/types/index.d.ts +++ b/packages/svelte/src/compiler/types/index.d.ts @@ -300,6 +300,8 @@ export interface Binding { references: { node: Identifier; path: SvelteNode[] }[]; mutated: boolean; reassigned: boolean; + /** `true` if mutated _or_ reassigned */ + updated: boolean; scope: Scope; /** For `legacy_reactive`: its reactive dependencies */ legacy_dependencies: Binding[]; diff --git a/packages/svelte/src/compiler/types/template.d.ts b/packages/svelte/src/compiler/types/template.d.ts index 5b4d31da04..318de6953c 100644 --- a/packages/svelte/src/compiler/types/template.d.ts +++ b/packages/svelte/src/compiler/types/template.d.ts @@ -404,8 +404,6 @@ export interface EachBlock extends BaseNode { index: Identifier; item: Identifier; declarations: Map; - /** List of bindings that are referenced within the expression */ - references: Binding[]; /** * Optimization path for each blocks: If the parent isn't a fragment and * it only has a single child, then we can classify the block as being "controlled". diff --git a/packages/svelte/types/index.d.ts b/packages/svelte/types/index.d.ts index 9b6198c547..3c0dede5b0 100644 --- a/packages/svelte/types/index.d.ts +++ b/packages/svelte/types/index.d.ts @@ -949,6 +949,8 @@ declare module 'svelte/compiler' { references: { node: Identifier; path: SvelteNode[] }[]; mutated: boolean; reassigned: boolean; + /** `true` if mutated _or_ reassigned */ + updated: boolean; scope: Scope; /** For `legacy_reactive`: its reactive dependencies */ legacy_dependencies: Binding[]; @@ -1859,8 +1861,6 @@ declare module 'svelte/compiler' { index: Identifier; item: Identifier; declarations: Map; - /** List of bindings that are referenced within the expression */ - references: Binding[]; /** * Optimization path for each blocks: If the parent isn't a fragment and * it only has a single child, then we can classify the block as being "controlled".