From 134d43539e7c722669bd33a911cd95639c3af6ef Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Wed, 23 Apr 2025 00:19:08 -0600 Subject: [PATCH 01/10] feat: State declarations in class constructors --- .../2-analyze/visitors/CallExpression.js | 100 +++++++++++++++++- 1 file changed, 97 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js index 904817b014e4..683e343aa56a 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js @@ -116,9 +116,11 @@ export function CallExpression(node, context) { case '$derived': case '$derived.by': if ( - (parent.type !== 'VariableDeclarator' || - get_parent(context.path, -3).type === 'ConstTag') && - !(parent.type === 'PropertyDefinition' && !parent.static && !parent.computed) + !( + call_expression_is_variable_declaration(parent, context) || + call_expression_is_class_property_definition(parent) || + call_expression_is_valid_class_property_assignment_in_constructor(parent, context) + ) ) { e.state_invalid_placement(node, rune); } @@ -270,3 +272,95 @@ function get_function_label(nodes) { return parent.id.name; } } + +/** + * + * @param {AST.SvelteNode} parent + * @param {Context} context + */ +function call_expression_is_variable_declaration(parent, context) { + return parent.type === 'VariableDeclarator' && get_parent(context.path, -3).type !== 'ConstTag'; +} + +/** + * + * @param {AST.SvelteNode} parent + */ +function call_expression_is_class_property_definition(parent) { + return parent.type === 'PropertyDefinition' && !parent.static && !parent.computed; +} + +/** + * + * @param {AST.SvelteNode} parent + * @param {Context} context + * @returns + */ +function call_expression_is_valid_class_property_assignment_in_constructor(parent, context) { + return ( + expression_is_assignment_to_top_level_property_of_this(parent) && + current_node_is_in_constructor_root_or_control_flow_blocks(context) + ); +} + +/** + * yes: + * - `this.foo = bar` + * + * no: + * - `this = bar` + * - `this.foo.baz = bar` + * - `anything_other_than_this = bar` + * + * @param {AST.SvelteNode} node + */ +function expression_is_assignment_to_top_level_property_of_this(node) { + return ( + node.type === 'AssignmentExpression' && + node.operator === '=' && + node.left.type === 'MemberExpression' && + node.left.object.type === 'ThisExpression' && + node.left.property.type === 'Identifier' + ); +} + +/** + * @param {AST.SvelteNode} node + */ +function node_is_constructor(node) { + return ( + node.type === 'MethodDefinition' && + node.key.type === 'Identifier' && + node.key.name === 'constructor' + ); +} + +// if blocks are just IfStatements with BlockStatements or other IfStatements as consequents +const allowed_parent_types = new Set([ + 'IfStatement', + 'BlockStatement', + 'SwitchCase', + 'SwitchStatement' +]); + +/** + * Succeeds if the node's only direct parents are `if` / `else if` / `else` blocks _and_ + * those blocks are the direct children of the constructor. + * + * @param {Context} context + */ +function current_node_is_in_constructor_root_or_control_flow_blocks(context) { + let parent_index = -3; // this gets us from CallExpression -> AssignmentExpression -> ExpressionStatement -> Whatever is here + while (true) { + const grandparent = get_parent(context.path, parent_index - 1); + const parent = get_parent(context.path, parent_index); + if (grandparent && node_is_constructor(grandparent)) { + // if this is the case then `parent` is the FunctionExpression + return true; + } + if (!allowed_parent_types.has(parent.type)) { + return false; + } + parent_index--; + } +} From fb8d6d7975ff53e833779cb0ac8d3e117f768fb8 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 24 Apr 2025 15:02:43 -0600 Subject: [PATCH 02/10] feat: Analysis phase --- .../98-reference/.generated/compile-errors.md | 33 +++- .../svelte/messages/compile-errors/script.md | 31 +++- packages/svelte/src/compiler/errors.js | 15 +- .../src/compiler/phases/2-analyze/index.js | 10 +- .../src/compiler/phases/2-analyze/types.d.ts | 5 +- .../visitors/AssignmentExpression.js | 1 + .../2-analyze/visitors/CallExpression.js | 80 +--------- .../phases/2-analyze/visitors/ClassBody.js | 30 ---- .../2-analyze/visitors/ClassDeclaration.js | 3 +- .../2-analyze/visitors/PropertyDefinition.js | 12 ++ .../visitors/shared/class-analysis.js | 147 ++++++++++++++++++ .../class-state-field-static/_config.js | 2 +- .../samples/runes-no-rune-each/_config.js | 3 +- .../runes-wrong-derived-placement/_config.js | 2 +- .../runes-wrong-state-placement/_config.js | 3 +- .../const-tag-invalid-rune-usage/errors.json | 2 +- 16 files changed, 257 insertions(+), 122 deletions(-) delete mode 100644 packages/svelte/src/compiler/phases/2-analyze/visitors/ClassBody.js create mode 100644 packages/svelte/src/compiler/phases/2-analyze/visitors/PropertyDefinition.js create mode 100644 packages/svelte/src/compiler/phases/2-analyze/visitors/shared/class-analysis.js diff --git a/documentation/docs/98-reference/.generated/compile-errors.md b/documentation/docs/98-reference/.generated/compile-errors.md index e8669ead533d..fb0591b884a5 100644 --- a/documentation/docs/98-reference/.generated/compile-errors.md +++ b/documentation/docs/98-reference/.generated/compile-errors.md @@ -208,6 +208,37 @@ Cannot assign to %thing% Cannot bind to %thing% ``` +### constructor_state_reassignment + +``` +Cannot redeclare stateful field `%name%` in the constructor. The field was originally declared here: `%original_location%` +``` + +To create stateful class fields in the constructor, the rune assignment must be the _first_ assignment to the class field. +Assignments thereafter must not use the rune. + +```ts +constructor() { + this.count = $state(0); + this.count = $state(1); // invalid, assigning to the same property with `$state` again +} + +constructor() { + this.count = $state(0); + this.count = $state.raw(1); // invalid, assigning to the same property with a different rune +} + +constructor() { + this.count = 0; + this.count = $state(1); // invalid, this property was created as a regular property, not state +} + +constructor() { + this.count = $state(0); + this.count = 1; // valid, this is setting the state that has already been declared +} +``` + ### css_empty_declaration ``` @@ -855,7 +886,7 @@ Cannot export state from a module if it is reassigned. Either export a function ### state_invalid_placement ``` -`%rune%(...)` can only be used as a variable declaration initializer or a class field +`%rune%(...)` can only be used as a variable declaration initializer, a class field declaration, or the first assignment to a class field at the top level of the constructor. ``` ### store_invalid_scoped_subscription diff --git a/packages/svelte/messages/compile-errors/script.md b/packages/svelte/messages/compile-errors/script.md index aabcbeae4812..780c9f4d2863 100644 --- a/packages/svelte/messages/compile-errors/script.md +++ b/packages/svelte/messages/compile-errors/script.md @@ -10,6 +10,35 @@ > Cannot bind to %thing% +## constructor_state_reassignment + +> Cannot redeclare stateful field `%name%` in the constructor. The field was originally declared here: `%original_location%` + +To create stateful class fields in the constructor, the rune assignment must be the _first_ assignment to the class field. +Assignments thereafter must not use the rune. + +```ts +constructor() { + this.count = $state(0); + this.count = $state(1); // invalid, assigning to the same property with `$state` again +} + +constructor() { + this.count = $state(0); + this.count = $state.raw(1); // invalid, assigning to the same property with a different rune +} + +constructor() { + this.count = 0; + this.count = $state(1); // invalid, this property was created as a regular property, not state +} + +constructor() { + this.count = $state(0); + this.count = 1; // valid, this is setting the state that has already been declared +} +``` + ## declaration_duplicate > `%name%` has already been declared @@ -218,7 +247,7 @@ It's possible to export a snippet from a ` + + diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/_config.js new file mode 100644 index 000000000000..4cf1aea213dc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/_config.js @@ -0,0 +1,45 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + // The component context class instance gets shared between tests, strangely, causing hydration to fail? + mode: ['client', 'server'], + + async test({ assert, target, logs }) { + const btn = target.querySelector('button'); + + flushSync(() => { + btn?.click(); + }); + + assert.deepEqual(logs, [0, 'class trigger false', 'local trigger false', 1]); + + flushSync(() => { + btn?.click(); + }); + + assert.deepEqual(logs, [0, 'class trigger false', 'local trigger false', 1, 2]); + + flushSync(() => { + btn?.click(); + }); + + assert.deepEqual(logs, [0, 'class trigger false', 'local trigger false', 1, 2, 3]); + + flushSync(() => { + btn?.click(); + }); + + assert.deepEqual(logs, [ + 0, + 'class trigger false', + 'local trigger false', + 1, + 2, + 3, + 4, + 'class trigger true', + 'local trigger true' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/main.svelte new file mode 100644 index 000000000000..03687d01bb3d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-derived-unowned/main.svelte @@ -0,0 +1,37 @@ + + + + + diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/_config.js new file mode 100644 index 000000000000..32cca6c69375 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target }) { + const btn = target.querySelector('button'); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/main.svelte new file mode 100644 index 000000000000..d8feb554cd18 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-subclass/main.svelte @@ -0,0 +1,22 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-constructor/_config.js new file mode 100644 index 000000000000..f35dc57228a1 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target }) { + const btn = target.querySelector('button'); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-constructor/main.svelte new file mode 100644 index 000000000000..aa8ba1658b03 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor/main.svelte @@ -0,0 +1,18 @@ + + + diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-1/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-1/errors.json new file mode 100644 index 000000000000..eaff12b89645 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-1/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": 5, + "column": 2 + }, + "end": { + "line": 5, + "column": 24 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-1/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-1/input.svelte.js new file mode 100644 index 000000000000..05cd4d9d9d64 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-1/input.svelte.js @@ -0,0 +1,7 @@ +export class Counter { + count = $state(0); + + constructor() { + this.count = $state(0); + } +} diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-2/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-2/errors.json new file mode 100644 index 000000000000..a27d7411d1f6 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-2/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):3:2`", + "start": { + "line": 5, + "column": 2 + }, + "end": { + "line": 5, + "column": 24 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-2/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-2/input.svelte.js new file mode 100644 index 000000000000..e37be4b3e691 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-2/input.svelte.js @@ -0,0 +1,7 @@ +export class Counter { + constructor() { + this.count = $state(0); + this.count = 1; + this.count = $state(0); + } +} diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-3/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-3/errors.json new file mode 100644 index 000000000000..8017794a679d --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-3/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):3:2`", + "start": { + "line": 5, + "column": 2 + }, + "end": { + "line": 5, + "column": 28 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-3/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-3/input.svelte.js new file mode 100644 index 000000000000..f9196ff3cd51 --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-3/input.svelte.js @@ -0,0 +1,7 @@ +export class Counter { + constructor() { + this.count = $state(0); + this.count = 1; + this.count = $state.raw(0); + } +} diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-4/errors.json b/packages/svelte/tests/validator/samples/class-state-constructor-4/errors.json new file mode 100644 index 000000000000..9f959874c80e --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-4/errors.json @@ -0,0 +1,14 @@ +[ + { + "code": "state_invalid_placement", + "message": "`$state(...)` can only be used as a variable declaration initializer, a class field declaration, or the first assignment to a class field at the top level of the constructor.", + "start": { + "line": 4, + "column": 16 + }, + "end": { + "line": 4, + "column": 25 + } + } +] diff --git a/packages/svelte/tests/validator/samples/class-state-constructor-4/input.svelte.js b/packages/svelte/tests/validator/samples/class-state-constructor-4/input.svelte.js new file mode 100644 index 000000000000..bf1aada1b5df --- /dev/null +++ b/packages/svelte/tests/validator/samples/class-state-constructor-4/input.svelte.js @@ -0,0 +1,7 @@ +export class Counter { + constructor() { + if (true) { + this.count = $state(0); + } + } +} From ac42ad5580125ab3eb99eeee7618d521a8100671 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Tue, 29 Apr 2025 16:22:57 -0400 Subject: [PATCH 08/10] 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 6bf5eb73d5db..aed7ff7fe8e8 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 4b3d758200b6..02b585fd4dba 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 ff8d2b5ce12c..34d80d388ca1 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 70dd3646765b..778fea32e5c2 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 9977c979db3f..ae84f0878228 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 44a641508766..73390d0e38ad 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 e482f10d49dc..20d9e75eca18 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 96fe27ca5dcc..2d02e705c317 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 8999f096142e..c610110cef9d 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 003c3a2c4945..29fb8c5c83c2 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 000000000000..f47bee71df87 --- /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 000000000000..e2c4f302b397 --- /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 000000000000..359274d15661 --- /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 000000000000..bc3d19a14fae --- /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 000000000000..359274d15661 --- /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 000000000000..2ebe52e685ed --- /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); + } +} From b44eed9a091c360a31d3b0529c7fbd555dc1fffe Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Tue, 29 Apr 2025 16:49:00 -0400 Subject: [PATCH 09/10] tests --- .../validator/samples/class-state-constructor-5/errors.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index 359274d15661..7942b016cb2b 100644 --- a/packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json +++ b/packages/svelte/tests/validator/samples/class-state-constructor-5/errors.json @@ -1,13 +1,13 @@ [ { "code": "constructor_state_reassignment", - "message": "Cannot redeclare stateful field `count` in the constructor. The field was originally declared here: `(unknown):2:1`", + "message": "Cannot redeclare stateful field `count` in the constructor. The field was originally declared here: `(unknown):3:1`", "start": { - "line": 4, + "line": 5, "column": 2 }, "end": { - "line": 4, + "line": 5, "column": 27 } } From 12a02b7eed5c3b01bc4be6ea2f060ecd3a8d5e4d Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Tue, 29 Apr 2025 17:00:13 -0400 Subject: [PATCH 10/10] test for better types --- .../_config.js | 20 +++++++++++++++++++ .../main.svelte | 12 +++++++++++ 2 files changed, 32 insertions(+) create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/_config.js new file mode 100644 index 000000000000..02cf36d900cc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target }) { + const btn = target.querySelector('button'); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + + flushSync(() => { + btn?.click(); + }); + assert.htmlEqual(target.innerHTML, ``); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/main.svelte new file mode 100644 index 000000000000..5dbbb10afd35 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-constructor-predeclared-field/main.svelte @@ -0,0 +1,12 @@ + + +