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 <5968653+dummdidumm@users.noreply.github.com>

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

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

* fix

* test

* changeset

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

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

* add some details

* check

* regenerate

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

.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,6 +50,11 @@ 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,
@@ -55,7 +62,9 @@ export function LabeledStatement(node, context) {
 			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');
+}

packages/svelte/src/internal/client/dom/blocks/html.js

@@ -9,6 +9,7 @@ import { hash } from '../../../../utils.js';
 import { DEV } from 'esm-env';
 import { dev_current_component_function } from '../../runtime.js';
 import { get_first_child, get_next_sibling } from '../operations.js';
+import { sanitize_location } from '../../dev/location.js';
 
 /**
  * @param {Element} element
@@ -28,9 +29,7 @@ function check_hash(element, server_hash, value) {
 		location = `in ${dev_current_component_function[FILENAME]}`;
 	}
 
-	w.hydration_html_changed(
-		location?.replace(/\//g, '/\u200b') // prevent devtools trying to make it a clickable link by inserting a zero-width space
-	);
+	w.hydration_html_changed(sanitize_location(location));
 }
 
 /**

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

@@ -16,7 +16,8 @@ import {
 	set_is_flushing_effect,
 	set_signal_status,
 	untrack,
-	skip_reaction
+	skip_reaction,
+	capture_signals
 } from '../runtime.js';
 import {
 	DIRTY,
@@ -39,10 +40,13 @@ import {
 } from '../constants.js';
 import { set } from './sources.js';
 import * as e from '../errors.js';
+import * as w from '../warnings.js';
 import { DEV } from 'esm-env';
 import { define_property } from '../../shared/utils.js';
 import { get_next_sibling } from '../dom/operations.js';
 import { destroy_derived } from './deriveds.js';
+import { FILENAME } from '../../../constants.js';
+import { get_location } from '../dev/location.js';
 
 /**
  * @param {'$effect' | '$effect.pre' | '$inspect'} rune
@@ -261,14 +265,21 @@ export function effect(fn) {
  * Internal representation of `$: ..`
  * @param {() => any} deps
  * @param {() => void | (() => void)} fn
+ * @param {number} [line]
+ * @param {number} [column]
  */
-export function legacy_pre_effect(deps, fn) {
+export function legacy_pre_effect(deps, fn, line, column) {
 	var context = /** @type {ComponentContextLegacy} */ (component_context);
 
 	/** @type {{ effect: null | Effect, ran: boolean }} */
 	var token = { effect: null, ran: false };
 	context.l.r1.push(token);
 
+	if (DEV && line !== undefined) {
+		var location = get_location(line, column);
+		var explicit_deps = capture_signals(deps);
+	}
+
 	token.effect = render_effect(() => {
 		deps();
 
@@ -278,7 +289,18 @@ export function legacy_pre_effect(deps, fn) {
 
 		token.ran = true;
 		set(context.l.r2, true);
-		untrack(fn);
+
+		if (DEV && location) {
+			var implicit_deps = capture_signals(() => untrack(fn));
+
+			for (var signal of implicit_deps) {
+				if (!explicit_deps.has(signal)) {
+					w.reactive_declaration_non_reactive_property(/** @type {string} */ (location));
+				}
+			}
+		} else {
+			untrack(fn);
+		}
 	});
 }
 

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

@@ -894,15 +894,17 @@ export function safe_get(signal) {
 }
 
 /**
- * Invokes a function and captures all signals that are read during the invocation,
- * then invalidates them.
- * @param {() => any} fn
+ * Capture an array of all the signals that are read when `fn` is called
+ * @template T
+ * @param {() => T} fn
  */
-export function invalidate_inner_signals(fn) {
+export function capture_signals(fn) {
 	var previous_captured_signals = captured_signals;
 	captured_signals = new Set();
+
 	var captured = captured_signals;
 	var signal;
+
 	try {
 		untrack(fn);
 		if (previous_captured_signals !== null) {
@@ -913,7 +915,19 @@ export function invalidate_inner_signals(fn) {
 	} finally {
 		captured_signals = previous_captured_signals;
 	}
-	for (signal of captured) {
+
+	return captured;
+}
+
+/**
+ * Invokes a function and captures all signals that are read during the invocation,
+ * then invalidates them.
+ * @param {() => any} fn
+ */
+export function invalidate_inner_signals(fn) {
+	var captured = capture_signals(() => untrack(fn));
+
+	for (var signal of captured) {
 		// Go one level up because derived signals created as part of props in legacy mode
 		if ((signal.f & LEGACY_DERIVED_PROP) !== 0) {
 			for (const dep of /** @type {Derived} */ (signal).deps || []) {

packages/svelte/src/internal/client/warnings.js

@@ -153,6 +153,19 @@ export function ownership_invalid_mutation(component, owner) {
 	}
 }
 
+/**
+ * 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
+ * @param {string} location
+ */
+export function reactive_declaration_non_reactive_property(location) {
+	if (DEV) {
+		console.warn(`%c[svelte] reactive_declaration_non_reactive_property\n%cA \`$:\` 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`, bold, normal);
+	} else {
+		// TODO print a link to the documentation
+		console.warn("reactive_declaration_non_reactive_property");
+	}
+}
+
 /**
  * Reactive `$state(...)` proxies and the values they proxy have different identities. Because of this, comparisons with `%operator%` will produce unexpected results
  * @param {string} operator

packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/_config.js

@@ -0,0 +1,26 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	compileOptions: {
+		dev: true
+	},
+
+	html: '<p>42</p><p>42</p><button>update</button><button>reset</button>',
+
+	test({ assert, target }) {
+		const [update, reset] = target.querySelectorAll('button');
+		flushSync(() => update.click());
+
+		assert.htmlEqual(
+			target.innerHTML,
+			'<p>42</p><p>42</p><button>update</button><button>reset</button>'
+		);
+
+		flushSync(() => reset.click());
+	},
+
+	warnings: [
+		'A `$:` statement (main.svelte:4:1) 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'
+	]
+});

packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/data.svelte.js

@@ -0,0 +1,11 @@
+export const obj = $state({
+	prop: 42
+});
+
+export function update() {
+	obj.prop += 1;
+}
+
+export function reset() {
+	obj.prop = 42;
+}

packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/main.svelte

@@ -0,0 +1,13 @@
+<script>
+	import { obj, update, reset } from './data.svelte.js';
+
+	$: a = obj.prop;
+
+	// svelte-ignore reactive_declaration_non_reactive_property
+	$: b = obj.prop;
+</script>
+
+<p>{a}</p>
+<p>{b}</p>
+<button on:click={update}>update</button>
+<button on:click={reset}>reset</button>

packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/_config.js

@@ -1,3 +0,0 @@
-import { test } from '../../test';
-
-export default test({});

packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/input.svelte

@@ -1,8 +0,0 @@
-<script>
-	import { obj } from './data.js';
-
-	$: prop = obj.prop;
-	obj.foo = 'a different prop';
-</script>
-
-<p>{prop}</p>

packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/warnings.json

@@ -1,14 +0,0 @@
-[
-	{
-		"code": "reactive_declaration_non_reactive_property",
-		"message": "Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update",
-		"start": {
-			"line": 4,
-			"column": 11
-		},
-		"end": {
-			"line": 4,
-			"column": 19
-		}
-	}
-]