From ac42ad5580125ab3eb99eeee7618d521a8100671 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Tue, 29 Apr 2025 16:22:57 -0400 Subject: [PATCH] final cleanup?? --- .../2-analyze/visitors/PropertyDefinition.js | 2 +- .../visitors/shared/class-analysis.js | 58 ++++++++++++++----- .../client/visitors/AssignmentExpression.js | 2 +- .../visitors/shared/client-class-analysis.js | 12 ++-- .../server/visitors/AssignmentExpression.js | 2 +- .../visitors/shared/server-class-analysis.js | 11 ++-- .../3-transform/shared/class_analysis.js | 52 +++++++++++++---- .../phases/3-transform/shared/types.d.ts | 33 ++++++++--- .../compiler/phases/3-transform/types.d.ts | 2 +- packages/svelte/src/compiler/phases/nodes.js | 1 + .../_config.js | 3 + .../main.svelte | 9 +++ .../class-state-constructor-5/errors.json | 14 +++++ .../class-state-constructor-5/input.svelte.js | 7 +++ .../class-state-constructor-6/errors.json | 14 +++++ .../class-state-constructor-6/input.svelte.js | 6 ++ 16 files changed, 182 insertions(+), 46 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/main.svelte create mode 100644 packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json create mode 100644 packages/svelte/tests/validator/samples/class-state-constructor-5/input.svelte.js create mode 100644 packages/svelte/tests/validator/samples/class-state-constructor-6/errors.json create mode 100644 packages/svelte/tests/validator/samples/class-state-constructor-6/input.svelte.js diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/PropertyDefinition.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/PropertyDefinition.js index 6bf5eb73d5..aed7ff7fe8 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/PropertyDefinition.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/PropertyDefinition.js @@ -7,6 +7,6 @@ * @param {Context} context */ export function PropertyDefinition(node, context) { - context.state.class_state?.register?.(node, context); + context.state.class_state?.register(node, context); context.next(); } diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/class-analysis.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/class-analysis.js index 4b3d758200..02b585fd4d 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/class-analysis.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/class-analysis.js @@ -1,4 +1,4 @@ -/** @import { AssignmentExpression, PropertyDefinition, Expression } from 'estree' */ +/** @import { AssignmentExpression, PropertyDefinition, Expression, Identifier, PrivateIdentifier, Literal, MethodDefinition } from 'estree' */ /** @import { AST } from '#compiler' */ /** @import { Context } from '../../types' */ /** @import { StateCreationRuneName } from '../../../../../utils.js' */ @@ -15,9 +15,11 @@ import { is_state_creation_rune } from '../../../../../utils.js'; const reassignable_assignments = new Set(['$state', '$state.raw', 'regular']); export class ClassAnalysis { - // TODO: Probably need to include property definitions here too /** @type {Map} */ - property_assignments = new Map(); + #public_assignments = new Map(); + + /** @type {Map} */ + #private_assignments = new Map(); /** * Determines if the node is a valid assignment to a class property, and if so, @@ -30,30 +32,46 @@ export class ClassAnalysis { let name; /** @type {PropertyAssignmentType} */ let type; + /** @type {boolean} */ + let is_private; if (node.type === 'AssignmentExpression') { if (!this.is_class_property_assignment_at_constructor_root(node, context.path)) { return; } - name = node.left.property.name; + + let maybe_name = get_name_for_identifier(node.left.property); + if (!maybe_name) { + return; + } + + name = maybe_name; type = this.#get_assignment_type(node, context); + is_private = node.left.property.type === 'PrivateIdentifier'; - this.#check_for_conflicts(node, name, type); + this.#check_for_conflicts(node, name, type, is_private); } else { if (!this.#is_assigned_property(node)) { return; } - name = node.key.name; + let maybe_name = get_name_for_identifier(node.key); + if (!maybe_name) { + return; + } + + name = maybe_name; type = this.#get_assignment_type(node, context); + is_private = node.key.type === 'PrivateIdentifier'; // we don't need to check for conflicts here because they're not possible yet } // we don't have to validate anything other than conflicts here, because the rune placement rules // catch all of the other weirdness. - if (!this.property_assignments.has(name)) { - this.property_assignments.set(name, { type, node }); + const map = is_private ? this.#private_assignments : this.#public_assignments; + if (!map.has(name)) { + map.set(name, { type, node }); } } @@ -61,7 +79,7 @@ export class ClassAnalysis { * @template {AST.SvelteNode} T * @param {AST.SvelteNode} node * @param {T[]} path - * @returns {node is AssignmentExpression & { left: { type: 'MemberExpression' } & { object: { type: 'ThisExpression' }; property: { type: 'Identifier' | 'PrivateIdentifier' } } }} + * @returns {node is AssignmentExpression & { left: { type: 'MemberExpression' } & { object: { type: 'ThisExpression' }; property: { type: 'Identifier' | 'PrivateIdentifier' | 'Literal' } } }} */ is_class_property_assignment_at_constructor_root(node, path) { if ( @@ -71,7 +89,8 @@ export class ClassAnalysis { node.left.type === 'MemberExpression' && node.left.object.type === 'ThisExpression' && (node.left.property.type === 'Identifier' || - node.left.property.type === 'PrivateIdentifier') + node.left.property.type === 'PrivateIdentifier' || + node.left.property.type === 'Literal') ) ) { return false; @@ -89,11 +108,13 @@ export class ClassAnalysis { * We only care about properties that have values assigned to them -- if they don't, * they can't be a conflict for state declared in the constructor. * @param {PropertyDefinition} node - * @returns {node is PropertyDefinition & { key: { type: 'PrivateIdentifier' | 'Identifier' }; value: Expression; static: false; computed: false }} + * @returns {node is PropertyDefinition & { key: { type: 'PrivateIdentifier' | 'Identifier' | 'Literal' }; value: Expression; static: false; computed: false }} */ #is_assigned_property(node) { return ( - (node.key.type === 'PrivateIdentifier' || node.key.type === 'Identifier') && + (node.key.type === 'PrivateIdentifier' || + node.key.type === 'Identifier' || + node.key.type === 'Literal') && Boolean(node.value) && !node.static && !node.computed @@ -107,9 +128,10 @@ export class ClassAnalysis { * @param {AssignmentExpression} node * @param {string} name * @param {PropertyAssignmentType} type + * @param {boolean} is_private */ - #check_for_conflicts(node, name, type) { - const existing = this.property_assignments.get(name); + #check_for_conflicts(node, name, type, is_private) { + const existing = (is_private ? this.#private_assignments : this.#public_assignments).get(name); if (!existing) { return; } @@ -140,3 +162,11 @@ export class ClassAnalysis { return 'regular'; } } + +/** + * + * @param {PrivateIdentifier | Identifier | Literal} node + */ +function get_name_for_identifier(node) { + return node.type === 'Literal' ? node.value?.toString() : node.name; +} diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js index ff8d2b5ce1..34d80d388c 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js @@ -17,7 +17,7 @@ import { validate_mutation } from './shared/utils.js'; * @param {Context} context */ export function AssignmentExpression(node, context) { - const stripped_node = context.state.class_analysis?.register_assignment(node, context); + const stripped_node = context.state.class_analysis?.generate_assignment(node, context); if (stripped_node) { return stripped_node; } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/client-class-analysis.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/client-class-analysis.js index 70dd364676..778fea32e5 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/client-class-analysis.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/client-class-analysis.js @@ -56,7 +56,10 @@ export function create_client_class_analysis(body) { left: { ...node.left, // ...swap out the assignment to go directly against the private field - property: field.id + property: field.id, + // this could be a transformation from `this.[1]` to `this.#_` (the private field we generated) + // -- private fields are never computed + computed: false }, // ...and swap out the assignment's value for the state field init right: build_init_value(field.kind, node.right.arguments[0], context) @@ -67,15 +70,14 @@ export function create_client_class_analysis(body) { } /** - * * @param {StateCreationRuneName} kind * @param {Expression | SpreadElement} arg * @param {Context} context */ function build_init_value(kind, arg, context) { - const init = /** @type {Expression} **/ ( - context.visit(arg, { ...context.state, in_constructor: false }) - ); + const init = arg + ? /** @type {Expression} **/ (context.visit(arg, { ...context.state, in_constructor: false })) + : b.void0; switch (kind) { case '$state': diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js index 9977c979db..ae84f08782 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js @@ -10,7 +10,7 @@ import { visit_assignment_expression } from '../../shared/assignments.js'; * @param {Context} context */ export function AssignmentExpression(node, context) { - const stripped_node = context.state.class_analysis?.register_assignment(node, context); + const stripped_node = context.state.class_analysis?.generate_assignment(node, context); if (stripped_node) { return stripped_node; } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/server-class-analysis.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/server-class-analysis.js index 44a6415087..73390d0e38 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/server-class-analysis.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/server-class-analysis.js @@ -23,11 +23,9 @@ export function create_server_class_analysis(body) { // value can be assigned in the constructor. value = null; } else if (field.kind !== '$derived' && field.kind !== '$derived.by') { - return [/** @type {PropertyDefinition} */ (context.visit(node, context.state))]; + return [/** @type {PropertyDefinition} */ (context.visit(node))]; } else { - const init = /** @type {Expression} **/ ( - context.visit(node.value.arguments[0], context.state) - ); + const init = /** @type {Expression} **/ (context.visit(node.value.arguments[0])); value = field.kind === '$derived.by' ? b.call('$.once', init) : b.call('$.once', b.thunk(init)); } @@ -73,7 +71,8 @@ export function create_server_class_analysis(body) { left: { ...node.left, // ...swap out the assignment to go directly against the private field - property: field.id + property: field.id, + computed: false }, // ...and swap out the assignment's value for the state field init right: build_init_value(field.kind, node.right.arguments[0], context) @@ -90,7 +89,7 @@ export function create_server_class_analysis(body) { * @param {Context} context */ function build_init_value(kind, arg, context) { - const init = /** @type {Expression} **/ (context.visit(arg, context.state)); + const init = arg ? /** @type {Expression} **/ (context.visit(arg)) : b.void0; switch (kind) { case '$state': diff --git a/packages/svelte/src/compiler/phases/3-transform/shared/class_analysis.js b/packages/svelte/src/compiler/phases/3-transform/shared/class_analysis.js index e482f10d49..20d9e75eca 100644 --- a/packages/svelte/src/compiler/phases/3-transform/shared/class_analysis.js +++ b/packages/svelte/src/compiler/phases/3-transform/shared/class_analysis.js @@ -19,13 +19,25 @@ import { get_rune } from '../../scope.js'; * @returns {ClassAnalysis} */ export function create_class_analysis(body, build_state_field, build_assignment) { - /** @type {Map} */ + /** + * Public, stateful fields. + * @type {Map} + */ const public_fields = new Map(); - /** @type {Map} */ + /** + * Private, stateful fields. These are namespaced separately because + * public and private fields can have the same name in the AST -- ex. + * `count` and `#count` are both named `count` -- and because it's useful + * in a couple of cases to be able to check for only one or the other. + * @type {Map} + */ const private_fields = new Map(); - /** @type {Array} */ + /** + * Accumulates nodes for the new class body. + * @type {Array} + */ const new_body = []; /** @@ -57,6 +69,8 @@ export function create_class_analysis(body, build_state_field, build_assignment) const replacers = []; /** + * Get a state field by name. + * * @param {string} name * @param {boolean} is_private * @param {ReadonlyArray} [kinds] @@ -69,20 +83,30 @@ export function create_class_analysis(body, build_state_field, build_assignment) } /** + * Create a child context that makes sense for passing to the child analyzers. * @param {TContext} context * @returns {TContext} */ function create_child_context(context) { + const state = { + ...context.state, + class_analysis + }; + // @ts-expect-error - I can't find a way to make TypeScript happy with these + const visit = (node, state_override) => context.visit(node, { ...state, ...state_override }); + // @ts-expect-error - I can't find a way to make TypeScript happy with these + const next = (state_override) => context.next({ ...state, ...state_override }); return { ...context, - state: { - ...context.state, - class_analysis - } + state, + visit, + next }; } /** + * Generate a new body for the class. Ensure there is a visitor for AssignmentExpression that + * calls `generate_assignment` to capture any stateful fields declared in the constructor. * @param {TContext} context */ function generate_body(context) { @@ -107,11 +131,16 @@ export function create_class_analysis(body, build_state_field, build_assignment) } /** + * Given an assignment expression, check to see if that assignment expression declares + * a stateful field. If it does, register that field and then return the processed + * assignment expression. If an assignment expression is returned from this function, + * it should be considered _fully processed_ and should replace the existing assignment + * expression node. * @param {AssignmentExpression} node * @param {TContext} context * @returns {AssignmentExpression | null} The node, if `register_assignment` handled its transformation. */ - function register_assignment(node, context) { + function generate_assignment(node, context) { const child_context = create_child_context(context); if ( !( @@ -119,7 +148,8 @@ export function create_class_analysis(body, build_state_field, build_assignment) node.left.type === 'MemberExpression' && node.left.object.type === 'ThisExpression' && (node.left.property.type === 'Identifier' || - node.left.property.type === 'PrivateIdentifier') + node.left.property.type === 'PrivateIdentifier' || + node.left.property.type === 'Literal') ) ) { return null; @@ -177,6 +207,8 @@ export function create_class_analysis(body, build_state_field, build_assignment) } /** + * Register a class body definition. + * * @param {PropertyDefinition | MethodDefinition | StaticBlock} node * @param {TContext} child_context * @returns {boolean} if this node is stateful and was registered @@ -325,7 +357,7 @@ export function create_class_analysis(body, build_state_field, build_assignment) const class_analysis = { get_field, generate_body, - register_assignment + generate_assignment }; return class_analysis; diff --git a/packages/svelte/src/compiler/phases/3-transform/shared/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/shared/types.d.ts index 96fe27ca5d..2d02e705c3 100644 --- a/packages/svelte/src/compiler/phases/3-transform/shared/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/shared/types.d.ts @@ -1,4 +1,14 @@ -import type { AssignmentExpression, CallExpression, Identifier, MemberExpression, PropertyDefinition, MethodDefinition, PrivateIdentifier, ThisExpression } from 'estree'; +import type { + AssignmentExpression, + CallExpression, + Identifier, + MemberExpression, + PropertyDefinition, + MethodDefinition, + PrivateIdentifier, + ThisExpression, + Literal +} from 'estree'; import type { StateField } from '../types'; import type { Context as ServerContext } from '../server/types'; import type { Context as ClientContext } from '../client/types'; @@ -7,13 +17,13 @@ import type { StateCreationRuneName } from '../../../../utils'; export type StatefulAssignment = AssignmentExpression & { left: MemberExpression & { object: ThisExpression; - property: Identifier | PrivateIdentifier + property: Identifier | PrivateIdentifier | Literal; }; right: CallExpression; }; export type StatefulPropertyDefinition = PropertyDefinition & { - key: Identifier | PrivateIdentifier; + key: Identifier | PrivateIdentifier | Literal; value: CallExpression; }; @@ -34,7 +44,9 @@ export type AssignmentBuilderParams = (params: AssignmentBuilderParams) => AssignmentExpression; +export type AssignmentBuilder = ( + params: AssignmentBuilderParams +) => AssignmentExpression; export type ClassAnalysis = { /** @@ -43,7 +55,11 @@ export type ClassAnalysis = { * @param kinds - What kinds of state creation runes you're looking for, eg. only '$derived.by'. * @returns The field if it exists and matches the given criteria, or null. */ - get_field: (name: string, is_private: boolean, kinds?: Array) => StateField | undefined; + get_field: ( + name: string, + is_private: boolean, + kinds?: Array + ) => StateField | undefined; /** * Given the body of a class, generate a new body with stateful fields. @@ -59,5 +75,8 @@ export type ClassAnalysis = { * a state field on the class. If it is, it registers that state field and modifies the * assignment expression. */ - register_assignment: (node: AssignmentExpression, context: TContext) => AssignmentExpression | null; -} + generate_assignment: ( + node: AssignmentExpression, + context: TContext + ) => AssignmentExpression | null; +}; diff --git a/packages/svelte/src/compiler/phases/3-transform/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/types.d.ts index 8999f09614..c610110cef 100644 --- a/packages/svelte/src/compiler/phases/3-transform/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/types.d.ts @@ -14,4 +14,4 @@ export interface TransformState { export interface StateField { kind: StateCreationRuneName; id: PrivateIdentifier; -} \ No newline at end of file +} diff --git a/packages/svelte/src/compiler/phases/nodes.js b/packages/svelte/src/compiler/phases/nodes.js index 003c3a2c49..29fb8c5c83 100644 --- a/packages/svelte/src/compiler/phases/nodes.js +++ b/packages/svelte/src/compiler/phases/nodes.js @@ -1,4 +1,5 @@ /** @import { AST, ExpressionMetadata } from '#compiler' */ + /** * All nodes that can appear elsewhere than the top level, have attributes and can contain children */ diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/_config.js new file mode 100644 index 0000000000..f47bee71df --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/_config.js @@ -0,0 +1,3 @@ +import { test } from '../../test'; + +export default test({}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/main.svelte new file mode 100644 index 0000000000..e2c4f302b3 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-conflicting-get-name/main.svelte @@ -0,0 +1,9 @@ + \ No newline at end of file diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json new file mode 100644 index 0000000000..359274d156 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json @@ -0,0 +1,14 @@ +[ + { + "code": "constructor_state_reassignment", + "message": "Cannot redeclare stateful field `count` in the constructor. The field was originally declared here: `(unknown):2:1`", + "start": { + "line": 4, + "column": 2 + }, + "end": { + "line": 4, + "column": 27 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-5/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-5/input.svelte.js new file mode 100644 index 0000000000..bc3d19a14f --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-5/input.svelte.js @@ -0,0 +1,7 @@ +export class Counter { + // prettier-ignore + 'count' = $state(0); + constructor() { + this['count'] = $state(0); + } +} diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-6/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-6/errors.json new file mode 100644 index 0000000000..359274d156 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-6/errors.json @@ -0,0 +1,14 @@ +[ + { + "code": "constructor_state_reassignment", + "message": "Cannot redeclare stateful field `count` in the constructor. The field was originally declared here: `(unknown):2:1`", + "start": { + "line": 4, + "column": 2 + }, + "end": { + "line": 4, + "column": 27 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-6/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-6/input.svelte.js new file mode 100644 index 0000000000..2ebe52e685 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-6/input.svelte.js @@ -0,0 +1,6 @@ +export class Counter { + count = $state(0); + constructor() { + this['count'] = $state(0); + } +}