feat: turn reactive_declaration_non_reactive_property into a runtime warning (#14192)

* turn `reactive_declaration_non_reactive_property` into a runtime warning

* ignore warning

* Update packages/svelte/src/internal/client/reactivity/effects.js

Co-authored-by: Simon H <[email protected]>

* Update packages/svelte/src/internal/client/runtime.js

Co-authored-by: Simon H <[email protected]>

* fix

* test

* changeset

* Update .changeset/witty-turtles-bake.md

Co-authored-by: Simon H <[email protected]>

* add some details

* check

* regenerate

---------

Co-authored-by: Simon H <[email protected]>

Author: Rich-Harris

.changeset/witty-turtles-bake.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+chore: turn reactive_declaration_non_reactive_property into a runtime warning

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

@@ -86,6 +86,40 @@ Mutating a value outside the component that created it is strongly discouraged.
 %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
 ```
 
+### reactive_declaration_non_reactive_property
+
+```
+A `$:` statement (%location%) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode
+```
+
+In legacy mode, a `$:` [reactive statement](https://svelte.dev/docs/svelte/legacy-reactive-assignments) re-runs when the state it _references_ changes. This is determined at compile time, by analysing the code.
+
+In runes mode, effects and deriveds re-run when there are changes to the values that are read during the function's _execution_.
+
+Often, the result is the same — for example these can be considered equivalent:
+
+```js
+$: sum = a + b;
+```
+
+```js
+const sum = $derived(a + b);
+```
+
+In some cases — such as the one that triggered the above warning — they are _not_ the same:
+
+```js
+const add = () => a + b;
+
+// the compiler can't 'see' that `sum` depends on `a` and `b`, but
+// they _would_ be read while executing the `$derived` version
+$: sum = add();
+```
+
+Similarly, reactive properties of [deep state](https://svelte.dev/docs/svelte/$state#Deep-state) are not visible to the compiler. As such, changes to these properties will cause effects and deriveds to re-run but will _not_ cause `$:` statements to re-run.
+
+When you [migrate this component](https://svelte.dev/docs/svelte/v5-migration-guide) to runes mode, the behaviour will change accordingly.
+
 ### state_proxy_equality_mismatch
 
 ```

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

@@ -726,12 +726,6 @@ Reactive declarations only exist at the top level of the instance script
 Reassignments of module-level declarations will not cause reactive statements to update
 ```
 
-### reactive_declaration_non_reactive_property
-
-```
-Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update
-```
-
 ### script_context_deprecated
 
 ```

packages/svelte/messages/client-warnings/warnings.md

@@ -54,6 +54,38 @@ The easiest way to log a value as it changes over time is to use the [`$inspect`
 
 > %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead
 
+## reactive_declaration_non_reactive_property
+
+> A `$:` statement (%location%) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode
+
+In legacy mode, a `$:` [reactive statement](https://svelte.dev/docs/svelte/legacy-reactive-assignments) re-runs when the state it _references_ changes. This is determined at compile time, by analysing the code.
+
+In runes mode, effects and deriveds re-run when there are changes to the values that are read during the function's _execution_.
+
+Often, the result is the same — for example these can be considered equivalent:
+
+```js
+$: sum = a + b;
+```
+
+```js
+const sum = $derived(a + b);
+```
+
+In some cases — such as the one that triggered the above warning — they are _not_ the same:
+
+```js
+const add = () => a + b;
+
+// the compiler can't 'see' that `sum` depends on `a` and `b`, but
+// they _would_ be read while executing the `$derived` version
+$: sum = add();
+```
+
+Similarly, reactive properties of [deep state](https://svelte.dev/docs/svelte/$state#Deep-state) are not visible to the compiler. As such, changes to these properties will cause effects and deriveds to re-run but will _not_ cause `$:` statements to re-run.
+
+When you [migrate this component](https://svelte.dev/docs/svelte/v5-migration-guide) to runes mode, the behaviour will change accordingly.
+
 ## state_proxy_equality_mismatch
 
 > Reactive `$state(...)` proxies and the values they proxy have different identities. Because of this, comparisons with `%operator%` will produce unexpected results

packages/svelte/messages/compile-warnings/script.md

@@ -26,10 +26,6 @@
 
 > Reassignments of module-level declarations will not cause reactive statements to update
 
-## reactive_declaration_non_reactive_property
-
-> Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update
-
 ## state_referenced_locally
 
 > State referenced in its own scope will never update. Did you mean to reference it inside a closure?

packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js

@@ -26,30 +26,5 @@ export function MemberExpression(node, context) {
 		context.state.analysis.needs_context = true;
 	}
 
-	if (context.state.reactive_statement) {
-		const left = object(node);
-
-		if (left !== null) {
-			const binding = context.state.scope.get(left.name);
-
-			if (binding && binding.kind === 'normal') {
-				const parent = /** @type {Node} */ (context.path.at(-1));
-
-				if (
-					binding.scope === context.state.analysis.module.scope ||
-					binding.declaration_kind === 'import' ||
-					(binding.initial &&
-						binding.initial.type !== 'ArrayExpression' &&
-						binding.initial.type !== 'ObjectExpression' &&
-						binding.scope.function_depth <= 1)
-				) {
-					if (parent.type !== 'MemberExpression' && parent.type !== 'CallExpression') {
-						w.reactive_declaration_non_reactive_property(node);
-					}
-				}
-			}
-		}
-	}
-
 	context.next();
 }

packages/svelte/src/compiler/phases/3-transform/client/visitors/LabeledStatement.js

@@ -1,6 +1,8 @@
+/** @import { Location } from 'locate-character' */
 /** @import { Expression, LabeledStatement, Statement } from 'estree' */
 /** @import { ReactiveStatement } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
+import { dev, is_ignored, locator } from '../../../../state.js';
 import * as b from '../../../../utils/builders.js';
 import { build_getter } from '../utils.js';
 
@@ -48,14 +50,21 @@ export function LabeledStatement(node, context) {
 		sequence.push(serialized);
 	}
 
+	const location =
+		dev && !is_ignored(node, 'reactive_declaration_non_reactive_property')
+			? locator(/** @type {number} */ (node.start))
+			: undefined;
+
 	// these statements will be topologically ordered later
 	context.state.legacy_reactive_statements.set(
 		node,
 		b.stmt(
 			b.call(
 				'$.legacy_pre_effect',
 				sequence.length > 0 ? b.thunk(b.sequence(sequence)) : b.thunk(b.block([])),
-				b.thunk(b.block(body))
+				b.thunk(b.block(body)),
+				location && b.literal(location.line),
+				location && b.literal(location.column)
 			)
 		)
 	);

packages/svelte/src/compiler/warnings.js

@@ -102,7 +102,6 @@ export const codes = [
 	"perf_avoid_nested_class",
 	"reactive_declaration_invalid_placement",
 	"reactive_declaration_module_script_dependency",
-	"reactive_declaration_non_reactive_property",
 	"state_referenced_locally",
 	"store_rune_conflict",
 	"css_unused_selector",
@@ -641,14 +640,6 @@ export function reactive_declaration_module_script_dependency(node) {
 	w(node, "reactive_declaration_module_script_dependency", "Reassignments of module-level declarations will not cause reactive statements to update");
 }
 
-/**
- * Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update
- * @param {null | NodeLike} node
- */
-export function reactive_declaration_non_reactive_property(node) {
-	w(node, "reactive_declaration_non_reactive_property", "Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update");
-}
-
 /**
  * State referenced in its own scope will never update. Did you mean to reference it inside a closure?
  * @param {null | NodeLike} node

packages/svelte/src/constants.js

@@ -44,7 +44,8 @@ export const IGNORABLE_RUNTIME_WARNINGS = /** @type {const} */ ([
 	'hydration_attribute_changed',
 	'hydration_html_changed',
 	'ownership_invalid_binding',
-	'ownership_invalid_mutation'
+	'ownership_invalid_mutation',
+	'reactive_declaration_non_reactive_property'
 ]);
 
 /**

packages/svelte/src/internal/client/dev/location.js

@@ -0,0 +1,25 @@
+import { DEV } from 'esm-env';
+import { FILENAME } from '../../../constants.js';
+import { dev_current_component_function } from '../runtime.js';
+
+/**
+ *
+ * @param {number} [line]
+ * @param {number} [column]
+ */
+export function get_location(line, column) {
+	if (!DEV || line === undefined) return undefined;
+
+	var filename = dev_current_component_function?.[FILENAME];
+	var location = filename && `${filename}:${line}:${column}`;
+
+	return sanitize_location(location);
+}
+
+/**
+ * Prevent devtools trying to make `location` a clickable link by inserting a zero-width space
+ * @param {string | undefined} location
+ */
+export function sanitize_location(location) {
+	return location?.replace(/\//g, '/\u200b');
+}