diff --git a/.changeset/shaggy-spies-happen.md b/.changeset/shaggy-spies-happen.md new file mode 100644 index 0000000000..bb80ffd8f8 --- /dev/null +++ b/.changeset/shaggy-spies-happen.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: proxify values when assigning using `||=`, `&&=` and `??=` operators diff --git a/documentation/docs/98-reference/.generated/client-warnings.md b/documentation/docs/98-reference/.generated/client-warnings.md index 6e97211341..60eb3e126c 100644 --- a/documentation/docs/98-reference/.generated/client-warnings.md +++ b/documentation/docs/98-reference/.generated/client-warnings.md @@ -1,5 +1,37 @@ +### assignment_value_stale + +``` +Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour. +``` + +Given a case like this... + +```svelte + + + +

items: {JSON.stringify(object.items)}

+``` + +...the array being pushed to when the button is first clicked is the `[]` on the right-hand side of the assignment, but the resulting value of `object.array` is an empty state proxy. As a result, the pushed value will be discarded. + +You can fix this by separating it into two statements: + +```js +function add() { + object.array ??= []; + object.array.push(object.array.length); +} +``` + ### binding_property_non_reactive ``` diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index d82cdc47a1..482a3d7faa 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -1,3 +1,33 @@ +## assignment_value_stale + +> Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour. + +Given a case like this... + +```svelte + + + +

items: {JSON.stringify(object.items)}

+``` + +...the array being pushed to when the button is first clicked is the `[]` on the right-hand side of the assignment, but the resulting value of `object.array` is an empty state proxy. As a result, the pushed value will be discarded. + +You can fix this by separating it into two statements: + +```js +function add() { + object.array ??= []; + object.array.push(object.array.length); +} +``` + ## binding_property_non_reactive > `%binding%` is binding to a non-reactive property 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 ee909ede91..66ea2c4941 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 @@ -1,8 +1,14 @@ -/** @import { AssignmentExpression, AssignmentOperator, Expression, Pattern } from 'estree' */ +/** @import { Location } from 'locate-character' */ +/** @import { AssignmentExpression, AssignmentOperator, Expression, Identifier, Literal, MemberExpression, Pattern } from 'estree' */ +/** @import { AST } from '#compiler' */ /** @import { Context } from '../types.js' */ import * as b from '../../../../utils/builders.js'; -import { build_assignment_value } from '../../../../utils/ast.js'; -import { is_ignored } from '../../../../state.js'; +import { + build_assignment_value, + get_attribute_expression, + is_event_attribute +} from '../../../../utils/ast.js'; +import { dev, filename, is_ignored, locator } from '../../../../state.js'; import { build_proxy_reassignment, should_proxy } from '../utils.js'; import { visit_assignment_expression } from '../../shared/assignments.js'; @@ -20,6 +26,24 @@ export function AssignmentExpression(node, context) { : expression; } +/** + * Determines whether the value will be coerced on assignment (as with e.g. `+=`). + * If not, we may need to proxify the value, or warn that the value will not be + * proxified in time + * @param {AssignmentOperator} operator + */ +function is_non_coercive_operator(operator) { + return ['=', '||=', '&&=', '??='].includes(operator); +} + +/** @type {Record} */ +const callees = { + '=': '$.assign', + '&&=': '$.assign_and', + '||=': '$.assign_or', + '??=': '$.assign_nullish' +}; + /** * @param {AssignmentOperator} operator * @param {Pattern} left @@ -41,7 +65,11 @@ function build_assignment(operator, left, right, context) { context.visit(build_assignment_value(operator, left, right)) ); - if (private_state.kind !== 'raw_state' && should_proxy(value, context.state.scope)) { + if ( + private_state.kind === 'state' && + is_non_coercive_operator(operator) && + should_proxy(value, context.state.scope) + ) { value = build_proxy_reassignment(value, b.member(b.this, private_state.id)); } @@ -73,24 +101,28 @@ function build_assignment(operator, left, right, context) { ? context.state.transform[object.name] : null; + const path = context.path.map((node) => node.type); + // reassignment if (object === left && transform?.assign) { + // special case — if an element binding, we know it's a primitive + + const is_primitive = path.at(-1) === 'BindDirective' && path.at(-2) === 'RegularElement'; + let value = /** @type {Expression} */ ( context.visit(build_assignment_value(operator, left, right)) ); - // special case — if an element binding, we know it's a primitive - const path = context.path.map((node) => node.type); - const is_primitive = path.at(-1) === 'BindDirective' && path.at(-2) === 'RegularElement'; - if ( !is_primitive && binding.kind !== 'prop' && binding.kind !== 'bindable_prop' && + binding.kind !== 'raw_state' && context.state.analysis.runes && - should_proxy(value, context.state.scope) + should_proxy(right, context.state.scope) && + is_non_coercive_operator(operator) ) { - value = binding.kind === 'raw_state' ? value : build_proxy_reassignment(value, object); + value = build_proxy_reassignment(value, object); } return transform.assign(object, value); @@ -108,5 +140,57 @@ function build_assignment(operator, left, right, context) { ); } + // in cases like `(object.items ??= []).push(value)`, we may need to warn + // if the value gets proxified, since the proxy _isn't_ the thing that + // will be pushed to. we do this by transforming it to something like + // `$.assign_nullish(object, 'items', [])` + let should_transform = + dev && path.at(-1) !== 'ExpressionStatement' && is_non_coercive_operator(operator); + + // special case — ignore `onclick={() => (...)}` + if ( + path.at(-1) === 'ArrowFunctionExpression' && + (path.at(-2) === 'RegularElement' || path.at(-2) === 'SvelteElement') + ) { + const element = /** @type {AST.RegularElement} */ (context.path.at(-2)); + + const attribute = element.attributes.find((attribute) => { + if (attribute.type !== 'Attribute' || !is_event_attribute(attribute)) { + return false; + } + + const expression = get_attribute_expression(attribute); + + return expression === context.path.at(-1); + }); + + if (attribute) { + should_transform = false; + } + } + + if (left.type === 'MemberExpression' && should_transform) { + const callee = callees[operator]; + + const loc = /** @type {Location} */ (locator(/** @type {number} */ (left.start))); + const location = `${filename}:${loc.line}:${loc.column}`; + + return /** @type {Expression} */ ( + context.visit( + b.call( + callee, + /** @type {Expression} */ (left.object), + /** @type {Expression} */ ( + left.computed + ? left.property + : b.literal(/** @type {Identifier} */ (left.property).name) + ), + right, + b.literal(location) + ) + ) + ); + } + return null; } diff --git a/packages/svelte/src/internal/client/dev/assign.js b/packages/svelte/src/internal/client/dev/assign.js new file mode 100644 index 0000000000..cf8c31a941 --- /dev/null +++ b/packages/svelte/src/internal/client/dev/assign.js @@ -0,0 +1,57 @@ +import * as w from '../warnings.js'; +import { sanitize_location } from './location.js'; + +/** + * + * @param {any} a + * @param {any} b + * @param {string} property + * @param {string} location + */ +function compare(a, b, property, location) { + if (a !== b) { + w.assignment_value_stale(property, /** @type {string} */ (sanitize_location(location))); + } + + return a; +} + +/** + * @param {any} object + * @param {string} property + * @param {any} value + * @param {string} location + */ +export function assign(object, property, value, location) { + return compare((object[property] = value), object[property], property, location); +} + +/** + * @param {any} object + * @param {string} property + * @param {any} value + * @param {string} location + */ +export function assign_and(object, property, value, location) { + return compare((object[property] &&= value), object[property], property, location); +} + +/** + * @param {any} object + * @param {string} property + * @param {any} value + * @param {string} location + */ +export function assign_or(object, property, value, location) { + return compare((object[property] ||= value), object[property], property, location); +} + +/** + * @param {any} object + * @param {string} property + * @param {any} value + * @param {string} location + */ +export function assign_nullish(object, property, value, location) { + return compare((object[property] ??= value), object[property], property, location); +} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index e8cbefb090..6fae2893e6 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -1,4 +1,5 @@ export { FILENAME, HMR, NAMESPACE_SVG } from '../../constants.js'; +export { assign, assign_and, assign_or, assign_nullish } from './dev/assign.js'; export { cleanup_styles } from './dev/css.js'; export { add_locations } from './dev/elements.js'; export { hmr } from './dev/hmr.js'; diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index 5e38db52f0..2f28d3e9e3 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -5,6 +5,20 @@ import { DEV } from 'esm-env'; var bold = 'font-weight: bold'; var normal = 'font-weight: normal'; +/** + * Assignment to `%property%` property (%location%) will evaluate to the right-hand side, not the value of `%property%` following the assignment. This may result in unexpected behaviour. + * @param {string} property + * @param {string} location + */ +export function assignment_value_stale(property, location) { + if (DEV) { + console.warn(`%c[svelte] assignment_value_stale\n%cAssignment to \`${property}\` property (${location}) will evaluate to the right-hand side, not the value of \`${property}\` following the assignment. This may result in unexpected behaviour.`, bold, normal); + } else { + // TODO print a link to the documentation + console.warn("assignment_value_stale"); + } +} + /** * `%binding%` (%location%) is binding to a non-reactive property * @param {string} binding diff --git a/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/_config.js b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/_config.js new file mode 100644 index 0000000000..a6d79c05ed --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/_config.js @@ -0,0 +1,24 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + html: ``, + + test({ assert, target, warnings }) { + const btn = target.querySelector('button'); + + flushSync(() => btn?.click()); + assert.htmlEqual(target.innerHTML, ``); + + flushSync(() => btn?.click()); + assert.htmlEqual(target.innerHTML, ``); + + assert.deepEqual(warnings, [ + 'Assignment to `items` property (main.svelte:5:24) will evaluate to the right-hand side, not the value of `items` following the assignment. This may result in unexpected behaviour.' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/main.svelte b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/main.svelte new file mode 100644 index 0000000000..f151336046 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/main.svelte @@ -0,0 +1,7 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/_config.js b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/_config.js new file mode 100644 index 0000000000..99d957e980 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/_config.js @@ -0,0 +1,16 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target }) { + const [btn1, btn2] = target.querySelectorAll('button'); + + flushSync(() => btn1.click()); + assert.htmlEqual(target.innerHTML, ``); + + flushSync(() => btn1.click()); + assert.htmlEqual(target.innerHTML, ``); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/main.svelte b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/main.svelte new file mode 100644 index 0000000000..84c1c32c5c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment/main.svelte @@ -0,0 +1,7 @@ + + +