fix: proxify values when assigning using ||=, &&= and ??= operators (#14273)

* add failing test for #14268

* simplify

* proxify values when using ||=, &&= and ??= assignment operators

* proxify values assigned to private state fields

* changeset

* fix

* fix

* add warning

* update test

Author: Rich-Harris

.changeset/shaggy-spies-happen.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: proxify values when assigning using `||=`, `&&=` and `??=` operators

documentation/docs/98-reference/.generated/client-warnings.md

@@ -1,5 +1,37 @@
 <!-- This file is generated by scripts/process-messages/index.js. Do not edit! -->
 
+### 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
+<script>
+	let object = $state({ array: null });
+
+	function add() {
+		(object.array ??= []).push(object.array.length);
+	}
+</script>
+
+<button onclick={add}>add</button>
+<p>items: {JSON.stringify(object.items)}</p>
+```
+
+...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
 
 ```

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
+<script>
+	let object = $state({ array: null });
+
+	function add() {
+		(object.array ??= []).push(object.array.length);
+	}
+</script>
+
+<button onclick={add}>add</button>
+<p>items: {JSON.stringify(object.items)}</p>
+```
+
+...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

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<string, string>} */
+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;
 }

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);
+}

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';

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

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: `<button>items: null</button>`,
+
+	test({ assert, target, warnings }) {
+		const btn = target.querySelector('button');
+
+		flushSync(() => btn?.click());
+		assert.htmlEqual(target.innerHTML, `<button>items: []</button>`);
+
+		flushSync(() => btn?.click());
+		assert.htmlEqual(target.innerHTML, `<button>items: [0]</button>`);
+
+		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.'
+		]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/proxy-nullish-coalescing-assignment-warning/main.svelte

@@ -0,0 +1,7 @@
+<script>
+	let object = $state({ items: null });
+</script>
+
+<button onclick={() => (object.items ??= []).push(object.items.length)}>
+	items: {JSON.stringify(object.items)}
+</button>

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: `<button>items: null</button>`,
+
+	test({ assert, target }) {
+		const [btn1, btn2] = target.querySelectorAll('button');
+
+		flushSync(() => btn1.click());
+		assert.htmlEqual(target.innerHTML, `<button>items: [0]</button>`);
+
+		flushSync(() => btn1.click());
+		assert.htmlEqual(target.innerHTML, `<button>items: [0,1]</button>`);
+	}
+});