From 02ea592b52ccc453ff3a19213a871fcccb2c0303 Mon Sep 17 00:00:00 2001 From: Jack Goodall Date: Tue, 29 Jul 2025 16:42:39 +0200 Subject: [PATCH] better error reporting --- .../compiler/phases/1-parse/state/element.js | 8 +-- .../2-analyze/visitors/BindDirective.js | 4 +- .../client/visitors/BindDirective.js | 8 +-- .../client/visitors/shared/utils.js | 7 ++- .../server/visitors/shared/element.js | 4 +- .../3-transform/shared/spread_bindings.js | 55 +++++++------------ .../svelte/src/compiler/utils/builders.js | 4 +- packages/svelte/src/internal/client/index.js | 3 +- packages/svelte/src/internal/server/index.js | 5 +- packages/svelte/src/internal/shared/errors.js | 19 ++++++- .../svelte/src/internal/shared/validate.js | 21 +++++++ .../samples/bind-spread-empty/_config.js | 21 +++++++ .../samples/bind-spread-empty/main.svelte | 37 +++++++++++++ .../samples/bind-spread-error/_config.js | 9 +++ .../samples/bind-spread-error/main.svelte | 7 +++ 15 files changed, 152 insertions(+), 60 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/bind-spread-empty/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/bind-spread-empty/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/bind-spread-error/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/bind-spread-error/main.svelte diff --git a/packages/svelte/src/compiler/phases/1-parse/state/element.js b/packages/svelte/src/compiler/phases/1-parse/state/element.js index f281c61f75..731f4b5997 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/element.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/element.js @@ -1,4 +1,4 @@ -/** @import { Expression } from 'estree' */ +/** @import { Expression, SpreadElement } from 'estree' */ /** @import { AST } from '#compiler' */ /** @import { Parser } from '../index.js' */ import { is_void } from '../../../../utils.js'; @@ -643,7 +643,7 @@ function read_attribute(parser) { const first_value = value === true ? undefined : Array.isArray(value) ? value[0] : value; - /** @type {Expression | null} */ + /** @type {Expression | SpreadElement | null} */ let expression = null; if (first_value) { @@ -655,10 +655,8 @@ function read_attribute(parser) { // TODO throw a parser error in a future version here if this `[ExpressionTag]` instead of `ExpressionTag`, // which means stringified value, which isn't allowed for some directives? expression = first_value.expression; - - // Handle spread syntax in bind directives + if (type === 'BindDirective' && first_value.metadata.expression.has_spread) { - // Create a SpreadElement to represent ...array syntax expression = { type: 'SpreadElement', start: first_value.start, 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 4ff27a68d8..dbb05bc5ef 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/BindDirective.js @@ -167,8 +167,8 @@ export function BindDirective(node, context) { // Validate that the spread is applied to a valid expression that returns an array const argument = node.expression.argument; if ( - argument.type !== 'Identifier' && - argument.type !== 'MemberExpression' && + argument.type !== 'Identifier' && + argument.type !== 'MemberExpression' && argument.type !== 'CallExpression' ) { e.bind_invalid_expression(node); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js index 74b3978f5d..19b7dda348 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js @@ -7,7 +7,7 @@ import * as b from '#compiler/builders'; import { binding_properties } from '../../../bindings.js'; import { build_attribute_value } from './shared/element.js'; import { build_bind_this, validate_binding } from './shared/utils.js'; -import { handle_spread_binding } from '../../shared/spread_bindings.js'; +import { init_spread_bindings } from '../../shared/spread_bindings.js'; /** * @param {AST.BindDirective} node @@ -18,11 +18,7 @@ export function BindDirective(node, context) { // Handle SpreadElement by creating a variable declaration before visiting if (node.expression.type === 'SpreadElement') { - const { get: getter, set: setter } = handle_spread_binding( - node.expression, - context.state, - context.visit - ); + const { get: getter, set: setter } = init_spread_bindings(node.expression, context); get = getter; set = setter; } else { diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 83d0a49959..c29f70ef98 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -9,7 +9,7 @@ import { regex_is_valid_identifier } from '../../../../patterns.js'; import is_reference from 'is-reference'; import { dev, is_ignored, locator, component_name } from '../../../../../state.js'; import { build_getter } from '../../utils.js'; -import { handle_spread_binding } from '../../../shared/spread_bindings.js'; +import { init_spread_bindings } from '../../../shared/spread_bindings.js'; /** * A utility for extracting complex expressions (such as call expressions) @@ -209,9 +209,10 @@ export function parse_directive_name(name) { * @param {Expression} value * @param {import('zimmerframe').Context} context */ -export function build_bind_this(expression, value, { state, visit }) { +export function build_bind_this(expression, value, context) { + const { state, visit } = context; if (expression.type === 'SpreadElement') { - const { get, set } = handle_spread_binding(expression, state, visit); + const { get, set } = init_spread_bindings(expression, context); return b.call('$.bind_this', value, set, get); } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js index 0ce26571e5..2f9b8ab81e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js @@ -21,7 +21,7 @@ import { is_load_error_element } from '../../../../../../utils.js'; import { escape_html } from '../../../../../../escaping.js'; -import { handle_spread_binding } from '../../../shared/spread_bindings.js'; +import { init_spread_bindings } from '../../../shared/spread_bindings.js'; const WHITESPACE_INSENSITIVE_ATTRIBUTES = ['class', 'style']; @@ -121,7 +121,7 @@ export function build_element_attributes(node, context) { // Handle SpreadElement for bind directives if (attribute.expression.type === 'SpreadElement') { - const { get } = handle_spread_binding(attribute.expression, context.state, context.visit); + const { get } = init_spread_bindings(attribute.expression, context); expression = b.call(get); } else if (expression.type === 'SequenceExpression') { expression = b.call(expression.expressions[0]); diff --git a/packages/svelte/src/compiler/phases/3-transform/shared/spread_bindings.js b/packages/svelte/src/compiler/phases/3-transform/shared/spread_bindings.js index c5727aead5..03a463b593 100644 --- a/packages/svelte/src/compiler/phases/3-transform/shared/spread_bindings.js +++ b/packages/svelte/src/compiler/phases/3-transform/shared/spread_bindings.js @@ -1,49 +1,32 @@ -/** @import { CallExpression, Expression, SpreadElement, Super } from 'estree' */ +/** @import { Expression, SpreadElement } from 'estree' */ +/** @import { Context } from 'zimmerframe' */ /** @import { ComponentClientTransformState } from '../client/types.js' */ /** @import { ComponentServerTransformState } from '../server/types.js' */ +/** @import { AST } from '#compiler' */ import * as b from '#compiler/builders'; +import { dev, source } from '../../../state.js'; /** - * Handles SpreadElement by creating a variable declaration and returning getter/setter expressions + * Initializes spread bindings for a SpreadElement in a bind directive. * @param {SpreadElement} spread_expression - * @param {ComponentClientTransformState | ComponentServerTransformState} state - * @param {function} visit - * @returns {{get: Expression, set: Expression}} + * @param {Context | Context} context + * @returns {{ get: Expression, set: Expression }} */ -export function handle_spread_binding(spread_expression, state, visit) { - // Generate a unique variable name for this spread binding - const id = b.id(state.scope.generate('$$bindings')); - +export function init_spread_bindings(spread_expression, { state, visit }) { const visited_expression = /** @type {Expression} */ (visit(spread_expression.argument)); - state.init.push(b.const(id, visited_expression)); - - const noop = b.arrow([], b.block([])); - - // Generate helper variables for clearer error messages - const get = b.id(state.scope.generate(id.name + '_get')); - const set = b.id(state.scope.generate(id.name + '_set')); + const expression_text = dev + ? b.literal(source.slice(spread_expression.argument.start, spread_expression.argument.end)) + : undefined; - const getter = b.logical( - '??', - b.conditional( - b.call('Array.isArray', id), - b.member(id, b.literal(0), true), - b.member(id, b.id('get')) - ), - noop + const id = state.scope.generate('$$spread_binding'); + const get = b.id(id + '_get'); + const set = b.id(id + '_set'); + state.init.push( + b.const( + b.array_pattern([get, set]), + b.call('$.validate_spread_bindings', visited_expression, expression_text) + ) ); - const setter = b.logical( - '??', - b.conditional( - b.call('Array.isArray', id), - b.member(id, b.literal(1), true), - b.member(id, b.id('set')) - ), - noop - ); - - state.init.push(b.const(get, getter)); - state.init.push(b.const(set, setter)); return { get, set }; } diff --git a/packages/svelte/src/compiler/utils/builders.js b/packages/svelte/src/compiler/utils/builders.js index 56a5f31ffe..6701884c9f 100644 --- a/packages/svelte/src/compiler/utils/builders.js +++ b/packages/svelte/src/compiler/utils/builders.js @@ -649,13 +649,13 @@ function return_builder(argument = null) { } /** - * @param {string} str + * @param {string | ESTree.TemplateLiteral} str * @returns {ESTree.ThrowStatement} */ export function throw_error(str) { return { type: 'ThrowStatement', - argument: new_builder('Error', literal(str)) + argument: new_builder('Error', typeof str === 'string' ? literal(str) : str) }; } diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index c094c9e044..34781d1039 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -172,7 +172,8 @@ export { validate_dynamic_element_tag, validate_store, validate_void_dynamic_element, - prevent_snippet_stringification + prevent_snippet_stringification, + validate_spread_bindings } from '../shared/validate.js'; export { strict_equals, equals } from './dev/equality.js'; export { log_if_contains_state } from './dev/console-log.js'; diff --git a/packages/svelte/src/internal/server/index.js b/packages/svelte/src/internal/server/index.js index 62ee22d6fc..7cfe15cae9 100644 --- a/packages/svelte/src/internal/server/index.js +++ b/packages/svelte/src/internal/server/index.js @@ -511,13 +511,14 @@ export { assign_payload, copy_payload } from './payload.js'; export { snapshot } from '../shared/clone.js'; -export { fallback, to_array } from '../shared/utils.js'; +export { fallback, to_array, noop } from '../shared/utils.js'; export { invalid_default_snippet, validate_dynamic_element_tag, validate_void_dynamic_element, - prevent_snippet_stringification + prevent_snippet_stringification, + validate_spread_bindings } from '../shared/validate.js'; export { escape_html as escape }; diff --git a/packages/svelte/src/internal/shared/errors.js b/packages/svelte/src/internal/shared/errors.js index 66685cb00b..ac06e510f0 100644 --- a/packages/svelte/src/internal/shared/errors.js +++ b/packages/svelte/src/internal/shared/errors.js @@ -114,4 +114,21 @@ export function svelte_element_invalid_this_value() { } else { throw new Error(`https://svelte.dev/e/svelte_element_invalid_this_value`); } -} \ No newline at end of file +} + +/** + * `%name%%member%` must be a function or `undefined` + * @param {string} name + * @returns {never} + */ +export function invalid_spread_bindings(name) { + if (DEV) { + const error = new Error(`invalid_spread_bindings\n\`${name}\` must be a function or \`undefined\`\nhttps://svelte.dev/e/invalid_spread_bindings`); + + error.name = 'Svelte error'; + + throw error; + } else { + throw new Error(`https://svelte.dev/e/invalid_spread_bindings`); + } +} diff --git a/packages/svelte/src/internal/shared/validate.js b/packages/svelte/src/internal/shared/validate.js index 48e76f0958..720baa03cb 100644 --- a/packages/svelte/src/internal/shared/validate.js +++ b/packages/svelte/src/internal/shared/validate.js @@ -1,6 +1,7 @@ import { is_void } from '../../utils.js'; import * as w from './warnings.js'; import * as e from './errors.js'; +import { noop } from './utils.js'; export { invalid_default_snippet } from './errors.js'; @@ -45,3 +46,23 @@ export function prevent_snippet_stringification(fn) { }; return fn; } + +/** + * @param {any} spread_object + * @param {string} name + * @return {[() => unknown, (value: unknown) => void]} + */ +export function validate_spread_bindings(spread_object, name) { + const is_array = Array.isArray(spread_object); + const getter = is_array ? spread_object[0] : spread_object.get; + const setter = is_array ? spread_object[1] : spread_object.set; + + if (typeof getter !== 'function' && getter != null) { + e.invalid_spread_bindings(name + (is_array ? '[0]' : '.get')); + } + if (typeof setter !== 'function' && setter != null) { + e.invalid_spread_bindings(name + (is_array ? '[1]' : '.set')); + } + + return [getter ?? noop, setter ?? noop]; +} diff --git a/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/_config.js b/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/_config.js new file mode 100644 index 0000000000..57cce72d30 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/_config.js @@ -0,0 +1,21 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + const checkboxes = target.querySelectorAll('input'); + + flushSync(); + + assert.htmlEqual(target.innerHTML, ``.repeat(checkboxes.length)); + + checkboxes.forEach((checkbox) => checkbox.click()); + + assert.deepEqual(logs, repeatArray(checkboxes.length, ['change', true])); + } +}); + +/** @template T */ +function repeatArray(/** @type {number} */ times, /** @type {T[]} */ array) { + return /** @type {T[]} */ Array.from({ length: times }, () => array).flat(); +} diff --git a/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/main.svelte b/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/main.svelte new file mode 100644 index 0000000000..e1c37443f6 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-spread-empty/main.svelte @@ -0,0 +1,37 @@ + + + + + + + + + + + + + + + + + diff --git a/packages/svelte/tests/runtime-runes/samples/bind-spread-error/_config.js b/packages/svelte/tests/runtime-runes/samples/bind-spread-error/_config.js new file mode 100644 index 0000000000..65c6deeda0 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-spread-error/_config.js @@ -0,0 +1,9 @@ +import { test } from '../../test'; + +export default test({ + expect_unhandled_rejections: true, + compileOptions: { + dev: true + }, + error: 'invalid_spread_bindings' +}); diff --git a/packages/svelte/tests/runtime-runes/samples/bind-spread-error/main.svelte b/packages/svelte/tests/runtime-runes/samples/bind-spread-error/main.svelte new file mode 100644 index 0000000000..8dc8f03644 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-spread-error/main.svelte @@ -0,0 +1,7 @@ + + +