better error reporting

pull/16607/head
Jack Goodall 1 month ago
parent a9ffe159ec
commit 02ea592b52

@ -1,4 +1,4 @@
/** @import { Expression } from 'estree' */ /** @import { Expression, SpreadElement } from 'estree' */
/** @import { AST } from '#compiler' */ /** @import { AST } from '#compiler' */
/** @import { Parser } from '../index.js' */ /** @import { Parser } from '../index.js' */
import { is_void } from '../../../../utils.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; const first_value = value === true ? undefined : Array.isArray(value) ? value[0] : value;
/** @type {Expression | null} */ /** @type {Expression | SpreadElement | null} */
let expression = null; let expression = null;
if (first_value) { 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`, // 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? // which means stringified value, which isn't allowed for some directives?
expression = first_value.expression; expression = first_value.expression;
// Handle spread syntax in bind directives
if (type === 'BindDirective' && first_value.metadata.expression.has_spread) { if (type === 'BindDirective' && first_value.metadata.expression.has_spread) {
// Create a SpreadElement to represent ...array syntax
expression = { expression = {
type: 'SpreadElement', type: 'SpreadElement',
start: first_value.start, start: first_value.start,

@ -167,8 +167,8 @@ export function BindDirective(node, context) {
// Validate that the spread is applied to a valid expression that returns an array // Validate that the spread is applied to a valid expression that returns an array
const argument = node.expression.argument; const argument = node.expression.argument;
if ( if (
argument.type !== 'Identifier' && argument.type !== 'Identifier' &&
argument.type !== 'MemberExpression' && argument.type !== 'MemberExpression' &&
argument.type !== 'CallExpression' argument.type !== 'CallExpression'
) { ) {
e.bind_invalid_expression(node); e.bind_invalid_expression(node);

@ -7,7 +7,7 @@ import * as b from '#compiler/builders';
import { binding_properties } from '../../../bindings.js'; import { binding_properties } from '../../../bindings.js';
import { build_attribute_value } from './shared/element.js'; import { build_attribute_value } from './shared/element.js';
import { build_bind_this, validate_binding } from './shared/utils.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 * @param {AST.BindDirective} node
@ -18,11 +18,7 @@ export function BindDirective(node, context) {
// Handle SpreadElement by creating a variable declaration before visiting // Handle SpreadElement by creating a variable declaration before visiting
if (node.expression.type === 'SpreadElement') { if (node.expression.type === 'SpreadElement') {
const { get: getter, set: setter } = handle_spread_binding( const { get: getter, set: setter } = init_spread_bindings(node.expression, context);
node.expression,
context.state,
context.visit
);
get = getter; get = getter;
set = setter; set = setter;
} else { } else {

@ -9,7 +9,7 @@ import { regex_is_valid_identifier } from '../../../../patterns.js';
import is_reference from 'is-reference'; import is_reference from 'is-reference';
import { dev, is_ignored, locator, component_name } from '../../../../../state.js'; import { dev, is_ignored, locator, component_name } from '../../../../../state.js';
import { build_getter } from '../../utils.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) * A utility for extracting complex expressions (such as call expressions)
@ -209,9 +209,10 @@ export function parse_directive_name(name) {
* @param {Expression} value * @param {Expression} value
* @param {import('zimmerframe').Context<AST.SvelteNode, ComponentClientTransformState>} context * @param {import('zimmerframe').Context<AST.SvelteNode, ComponentClientTransformState>} 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') { 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); return b.call('$.bind_this', value, set, get);
} }

@ -21,7 +21,7 @@ import {
is_load_error_element is_load_error_element
} from '../../../../../../utils.js'; } from '../../../../../../utils.js';
import { escape_html } from '../../../../../../escaping.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']; const WHITESPACE_INSENSITIVE_ATTRIBUTES = ['class', 'style'];
@ -121,7 +121,7 @@ export function build_element_attributes(node, context) {
// Handle SpreadElement for bind directives // Handle SpreadElement for bind directives
if (attribute.expression.type === 'SpreadElement') { 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); expression = b.call(get);
} else if (expression.type === 'SequenceExpression') { } else if (expression.type === 'SequenceExpression') {
expression = b.call(expression.expressions[0]); expression = b.call(expression.expressions[0]);

@ -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 { ComponentClientTransformState } from '../client/types.js' */
/** @import { ComponentServerTransformState } from '../server/types.js' */ /** @import { ComponentServerTransformState } from '../server/types.js' */
/** @import { AST } from '#compiler' */
import * as b from '#compiler/builders'; 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 {SpreadElement} spread_expression
* @param {ComponentClientTransformState | ComponentServerTransformState} state * @param {Context<AST.SvelteNode, ComponentClientTransformState> | Context<AST.SvelteNode, ComponentServerTransformState>} context
* @param {function} visit * @returns {{ get: Expression, set: Expression }}
* @returns {{get: Expression, set: Expression}}
*/ */
export function handle_spread_binding(spread_expression, state, visit) { export function init_spread_bindings(spread_expression, { state, visit }) {
// Generate a unique variable name for this spread binding
const id = b.id(state.scope.generate('$$bindings'));
const visited_expression = /** @type {Expression} */ (visit(spread_expression.argument)); const visited_expression = /** @type {Expression} */ (visit(spread_expression.argument));
state.init.push(b.const(id, visited_expression)); const expression_text = dev
? b.literal(source.slice(spread_expression.argument.start, spread_expression.argument.end))
const noop = b.arrow([], b.block([])); : undefined;
// 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 getter = b.logical( const id = state.scope.generate('$$spread_binding');
'??', const get = b.id(id + '_get');
b.conditional( const set = b.id(id + '_set');
b.call('Array.isArray', id), state.init.push(
b.member(id, b.literal(0), true), b.const(
b.member(id, b.id('get')) b.array_pattern([get, set]),
), b.call('$.validate_spread_bindings', visited_expression, expression_text)
noop )
); );
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 }; return { get, set };
} }

@ -649,13 +649,13 @@ function return_builder(argument = null) {
} }
/** /**
* @param {string} str * @param {string | ESTree.TemplateLiteral} str
* @returns {ESTree.ThrowStatement} * @returns {ESTree.ThrowStatement}
*/ */
export function throw_error(str) { export function throw_error(str) {
return { return {
type: 'ThrowStatement', type: 'ThrowStatement',
argument: new_builder('Error', literal(str)) argument: new_builder('Error', typeof str === 'string' ? literal(str) : str)
}; };
} }

@ -172,7 +172,8 @@ export {
validate_dynamic_element_tag, validate_dynamic_element_tag,
validate_store, validate_store,
validate_void_dynamic_element, validate_void_dynamic_element,
prevent_snippet_stringification prevent_snippet_stringification,
validate_spread_bindings
} from '../shared/validate.js'; } from '../shared/validate.js';
export { strict_equals, equals } from './dev/equality.js'; export { strict_equals, equals } from './dev/equality.js';
export { log_if_contains_state } from './dev/console-log.js'; export { log_if_contains_state } from './dev/console-log.js';

@ -511,13 +511,14 @@ export { assign_payload, copy_payload } from './payload.js';
export { snapshot } from '../shared/clone.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 { export {
invalid_default_snippet, invalid_default_snippet,
validate_dynamic_element_tag, validate_dynamic_element_tag,
validate_void_dynamic_element, validate_void_dynamic_element,
prevent_snippet_stringification prevent_snippet_stringification,
validate_spread_bindings
} from '../shared/validate.js'; } from '../shared/validate.js';
export { escape_html as escape }; export { escape_html as escape };

@ -114,4 +114,21 @@ export function svelte_element_invalid_this_value() {
} else { } else {
throw new Error(`https://svelte.dev/e/svelte_element_invalid_this_value`); throw new Error(`https://svelte.dev/e/svelte_element_invalid_this_value`);
} }
} }
/**
* `%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`);
}
}

@ -1,6 +1,7 @@
import { is_void } from '../../utils.js'; import { is_void } from '../../utils.js';
import * as w from './warnings.js'; import * as w from './warnings.js';
import * as e from './errors.js'; import * as e from './errors.js';
import { noop } from './utils.js';
export { invalid_default_snippet } from './errors.js'; export { invalid_default_snippet } from './errors.js';
@ -45,3 +46,23 @@ export function prevent_snippet_stringification(fn) {
}; };
return 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];
}

@ -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, `<input type="checkbox" >`.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();
}

@ -0,0 +1,37 @@
<script>
const empty_bindings_array = []
const empty_bindings_object = {}
const incompatible_bindings_object = {
read() {
console.log('read');
return true;
},
write(v) {
console.log('write', v);
}
}
const undefined_bindings_array = [undefined, undefined];
const undefined_bindings_object = { get: undefined, set: undefined };
const null_bindings_array = [null, null];
const null_bindings_object = { get: null, set: null };
function onchange(event) {
console.log('change', event.currentTarget.checked);
}
</script>
<input type="checkbox" {onchange} />
<input type="checkbox" bind:checked={...empty_bindings_array} {onchange} />
<input type="checkbox" bind:checked={...empty_bindings_object} {onchange} />
<input type="checkbox" bind:checked={...incompatible_bindings_object} {onchange} />
<input type="checkbox" bind:checked={...undefined_bindings_array} {onchange} />
<input type="checkbox" bind:checked={...undefined_bindings_object} {onchange} />
<input type="checkbox" bind:checked={...null_bindings_array} {onchange} />
<input type="checkbox" bind:checked={...null_bindings_object} {onchange} />

@ -0,0 +1,9 @@
import { test } from '../../test';
export default test({
expect_unhandled_rejections: true,
compileOptions: {
dev: true
},
error: 'invalid_spread_bindings'
});

@ -0,0 +1,7 @@
<script>
function getInvalidBindings() {
return { get: 'not a function', set: 'not a function' };
}
</script>
<input type="checkbox" bind:checked={...getInvalidBindings()} />
Loading…
Cancel
Save