warn on nonstate references

Author: Rich-Harris

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -38,7 +38,7 @@ function js(script, root, allow_reactive_declarations, parent) {
 		body: []
 	};
 
-	const { scope, scopes } = create_scopes(ast, root, allow_reactive_declarations, parent);
+	const { scope, scopes } = create_scopes(ast, root, allow_reactive_declarations, false, parent);
 
 	return { ast, scope, scopes };
 }
@@ -191,7 +191,7 @@ function get_delegated_event(node, context) {
  * @returns {import('../types.js').Analysis}
  */
 export function analyze_module(ast, options) {
-	const { scope, scopes } = create_scopes(ast, new ScopeRoot(), false, null);
+	const { scope, scopes } = create_scopes(ast, new ScopeRoot(), false, false, null);
 
 	for (const [name, references] of scope.references) {
 		if (name[0] !== '$' || ReservedKeywords.includes(name)) continue;
@@ -242,7 +242,7 @@ export function analyze_component(root, options) {
 	const module = js(root.module, scope_root, false, null);
 	const instance = js(root.instance, scope_root, true, module.scope);
 
-	const { scope, scopes } = create_scopes(root.fragment, scope_root, false, instance.scope);
+	const { scope, scopes } = create_scopes(root.fragment, scope_root, false, true, instance.scope);
 
 	/** @type {import('../types.js').Template} */
 	const template = { ast: root.fragment, scope, scopes };
@@ -414,6 +414,15 @@ export function analyze_component(root, options) {
 		analysis.reactive_statements = order_reactive_statements(analysis.reactive_statements);
 	}
 
+	// warn on any nonstate declarations that are a) mutated and b) referenced in the template
+	for (const scope of [module.scope, instance.scope]) {
+		for (const [name, binding] of scope.declarations) {
+			if (binding.kind === 'normal' && binding.mutated && binding.referenced_in_template) {
+				warn(warnings, binding.node, [], 'non-state-reference', name);
+			}
+		}
+	}
+
 	analysis.stylesheet.validate(analysis);
 
 	for (const element of analysis.elements) {

packages/svelte/src/compiler/phases/scope.js

@@ -91,6 +91,7 @@ export class Scope {
 		const binding = {
 			node,
 			references: [],
+			referenced_in_template: false,
 			legacy_dependencies: [],
 			initial,
 			mutated: false,
@@ -168,20 +169,22 @@ export class Scope {
 	/**
 	 * @param {import('estree').Identifier} node
 	 * @param {import('#compiler').SvelteNode[]} path
+	 * @param {boolean} is_template
 	 */
-	reference(node, path) {
+	reference(node, path, is_template) {
 		let references = this.references.get(node.name);
 		if (!references) this.references.set(node.name, (references = []));
 
 		references.push({ node, path });
 
-		const declaration = this.declarations.get(node.name);
-		if (declaration) {
-			declaration.references.push({ node, path });
+		const binding = this.declarations.get(node.name);
+		if (binding) {
+			if (is_template) binding.referenced_in_template = true;
+			binding.references.push({ node, path });
 		} else if (this.#parent) {
-			this.#parent.reference(node, path);
+			this.#parent.reference(node, path, is_template);
 		} else {
-			// no declaration was found, and this is the top level scope,
+			// no binding was found, and this is the top level scope,
 			// which means this is a global
 			this.root.conflicts.add(node.name);
 		}
@@ -214,10 +217,11 @@ export class ScopeRoot {
  * @param {import('#compiler').SvelteNode} ast
  * @param {ScopeRoot} root
  * @param {boolean} allow_reactive_declarations
+ * @param {boolean} is_template
  * @param {Scope | null} parent
  */
-export function create_scopes(ast, root, allow_reactive_declarations, parent) {
-	/** @typedef {{ scope: Scope }} State */
+export function create_scopes(ast, root, allow_reactive_declarations, is_template, parent) {
+	/** @typedef {{ scope: Scope, is_template: boolean }} State */
 
 	/**
 	 * A map of node->associated scope. A node appearing in this map does not necessarily mean that it created a scope
@@ -228,9 +232,9 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 	scopes.set(ast, scope);
 
 	/** @type {State} */
-	const state = { scope };
+	const state = { scope, is_template };
 
-	/** @type {[Scope, { node: import('estree').Identifier; path: import('#compiler').SvelteNode[] }][]} */
+	/** @type {[Scope, { node: import('estree').Identifier; path: import('#compiler').SvelteNode[]; is_template: boolean }][]} */
 	const references = [];
 
 	/** @type {[Scope, import('estree').Pattern | import('estree').MemberExpression][]} */
@@ -261,7 +265,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 		const scope = state.scope.child(true);
 		scopes.set(node, scope);
 
-		next({ scope });
+		next({ ...state, scope });
 	};
 
 	/**
@@ -270,7 +274,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 	const SvelteFragment = (node, { state, next }) => {
 		const scope = analyze_let_directives(node, state.scope);
 		scopes.set(node, scope);
-		next({ scope });
+		next({ ...state, scope });
 	};
 
 	/**
@@ -316,7 +320,10 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 		Identifier(node, { path, state }) {
 			const parent = path.at(-1);
 			if (parent && is_reference(node, /** @type {import('estree').Node} */ (parent))) {
-				references.push([state.scope, { node, path: path.slice() }]);
+				references.push([
+					state.scope,
+					{ node, path: path.slice(), is_template: state.is_template }
+				]);
 			}
 		},
 
@@ -339,15 +346,15 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 				}
 			}
 
-			next({ scope });
+			next({ ...state, scope });
 		},
 
 		SvelteFragment,
 		SvelteElement: SvelteFragment,
 		RegularElement: SvelteFragment,
 
 		Component(node, { state, visit, path }) {
-			state.scope.reference(b.id(node.name), path);
+			state.scope.reference(b.id(node.name), path, false);
 
 			// let:x from the default slot is a weird one:
 			// Its scope only applies to children that are not slots themselves.
@@ -371,7 +378,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 				} else if (child.type === 'SnippetBlock') {
 					visit(child);
 				} else {
-					visit(child, { scope });
+					visit(child, { ...state, scope });
 				}
 			}
 		},
@@ -405,7 +412,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 			if (node.id) scope.declare(node.id, 'normal', 'function');
 
 			add_params(scope, node.params);
-			next({ scope });
+			next({ scope, is_template: false });
 		},
 
 		FunctionDeclaration(node, { state, next }) {
@@ -415,15 +422,15 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 			scopes.set(node, scope);
 
 			add_params(scope, node.params);
-			next({ scope });
+			next({ scope, is_template: false });
 		},
 
 		ArrowFunctionExpression(node, { state, next }) {
 			const scope = state.scope.child();
 			scopes.set(node, scope);
 
 			add_params(scope, node.params);
-			next({ scope });
+			next({ scope, is_template: false });
 		},
 
 		ForStatement: create_block_scope,
@@ -468,7 +475,7 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 					state.scope.declare(id, 'normal', 'let');
 				}
 
-				next({ scope });
+				next({ ...state, scope });
 			} else {
 				next();
 			}
@@ -506,13 +513,13 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 					(node.key.type !== 'Identifier' || !node.index || node.key.name !== node.index);
 				scope.declare(b.id(node.index), is_keyed ? 'derived' : 'normal', 'const');
 			}
-			if (node.key) visit(node.key, { scope });
+			if (node.key) visit(node.key, { ...state, scope });
 
 			// children
 			for (const child of node.body.nodes) {
-				visit(child, { scope });
+				visit(child, { ...state, scope });
 			}
-			if (node.fallback) visit(node.fallback, { scope });
+			if (node.fallback) visit(node.fallback, { ...state, scope });
 
 			// Check if inner scope shadows something from outer scope.
 			// This is necessary because we need access to the array expression of the each block
@@ -579,13 +586,13 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 				}
 			}
 
-			context.next({ scope: child_scope });
+			context.next({ ...state, scope: child_scope });
 		},
 
 		Fragment: (node, context) => {
 			const scope = context.state.scope.child(node.transparent);
 			scopes.set(node, scope);
-			context.next({ scope });
+			context.next({ ...state, scope });
 		},
 
 		BindDirective(node, context) {
@@ -622,8 +629,8 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 
 	// we do this after the fact, so that we don't need to worry
 	// about encountering references before their declarations
-	for (const [scope, { node, path }] of references) {
-		scope.reference(node, path);
+	for (const [scope, { node, path, is_template }] of references) {
+		scope.reference(node, path, is_template);
 	}
 
 	for (const [scope, node] of updates) {

packages/svelte/src/compiler/types/index.d.ts

@@ -268,6 +268,7 @@ export interface Binding {
 	initial: null | Expression | FunctionDeclaration | ClassDeclaration | ImportDeclaration;
 	is_called: boolean;
 	references: { node: Identifier; path: SvelteNode[] }[];
+	referenced_in_template: boolean;
 	mutated: boolean;
 	scope: Scope;
 	/** For `legacy_reactive`: its reactive dependencies */

packages/svelte/src/compiler/warnings.js

@@ -23,7 +23,10 @@ const runes = {
 		`Referencing a local variable with a $ prefix will create a store subscription. Please rename ${name} to avoid the ambiguity.`,
 	/** @param {string} name */
 	'state-rune-not-mutated': (name) =>
-		`${name} is declared with $state(...) but is never updated. Did you mean to create a function that changes its value?`
+		`${name} is declared with $state(...) but is never updated. Did you mean to create a function that changes its value?`,
+	/** @param {string} name */
+	'non-state-reference': (name) =>
+		`${name} is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates.`
 };
 
 /** @satisfies {Warnings} */

packages/svelte/tests/validator/samples/runes-referenced-nonstate/_config.js

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

packages/svelte/tests/validator/samples/runes-referenced-nonstate/input.svelte

@@ -0,0 +1,8 @@
+<script>
+	let a = $state(1);
+	let b = 2;
+</script>
+
+<button onclick={() => a += 1}>a += 1</button>
+<button onclick={() => b += 1}>b += 1</button>
+<p>{a} + {b} = {a + b}</p>

packages/svelte/tests/validator/samples/runes-referenced-nonstate/warnings.json

@@ -0,0 +1,14 @@
+[
+	{
+		"code": "non-state-reference",
+		"message": "b is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates.",
+		"start": {
+			"column": 5,
+			"line": 3
+		},
+		"end": {
+			"column": 6,
+			"line": 3
+		}
+	}
+]