fix: tighten up $ prefix validation (#13261)

Our current validation was both too lax and too strict:
- too strict, because you can define a `$` variable in Svelte 4 if it's not at the top level
- too strict, because you can define `$`-prefixed function parameters, but not give the parameter the name `$`
- too lax, because you can define a `$`-prefixed variable if you're at least one level deep into a function. In runes mode, this should be an error

This PR aligns the behavior, ensures this isn't a breaking change in legacy anymore, and makes the validation in runes mode more strict

Author: dummdidumm

.changeset/twelve-shrimps-run.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: tighten up `$` prefix validation

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

@@ -1,12 +1,17 @@
 /** @import { ClassDeclaration } from 'estree' */
 /** @import { Context } from '../types' */
 import * as w from '../../../warnings.js';
+import { validate_identifier_name } from './shared/utils.js';
 
 /**
  * @param {ClassDeclaration} node
  * @param {Context} context
  */
 export function ClassDeclaration(node, context) {
+	if (context.state.analysis.runes) {
+		validate_identifier_name(context.state.scope.get(node.id.name));
+	}
+
 	// In modules, we allow top-level module scope only, in components, we allow the component scope,
 	// which is function_depth of 1. With the exception of `new class` which is also not allowed at
 	// component scope level either.

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

@@ -1,11 +1,16 @@
 /** @import { FunctionDeclaration } from 'estree' */
 /** @import { Context } from '../types' */
 import { visit_function } from './shared/function.js';
+import { validate_identifier_name } from './shared/utils.js';
 
 /**
  * @param {FunctionDeclaration} node
  * @param {Context} context
  */
 export function FunctionDeclaration(node, context) {
+	if (context.state.analysis.runes) {
+		validate_identifier_name(context.state.scope.get(node.id.name));
+	}
+
 	visit_function(node, context);
 }

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

@@ -2,7 +2,7 @@
 /** @import { Binding } from '#compiler' */
 /** @import { Context } from '../types' */
 import { get_rune } from '../../scope.js';
-import { ensure_no_module_import_conflict } from './shared/utils.js';
+import { ensure_no_module_import_conflict, validate_identifier_name } from './shared/utils.js';
 import * as e from '../../../errors.js';
 import { extract_paths } from '../../../utils/ast.js';
 import { equal } from '../../../utils/assert.js';
@@ -17,6 +17,11 @@ export function VariableDeclarator(node, context) {
 	if (context.state.analysis.runes) {
 		const init = node.init;
 		const rune = get_rune(init, context.state.scope);
+		const paths = extract_paths(node.id);
+
+		for (const path of paths) {
+			validate_identifier_name(context.state.scope.get(/** @type {Identifier} */ (path.node).name));
+		}
 
 		// TODO feels like this should happen during scope creation?
 		if (
@@ -26,7 +31,7 @@ export function VariableDeclarator(node, context) {
 			rune === '$derived.by' ||
 			rune === '$props'
 		) {
-			for (const path of extract_paths(node.id)) {
+			for (const path of paths) {
 				// @ts-ignore this fails in CI for some insane reason
 				const binding = /** @type {Binding} */ (context.state.scope.get(path.node.name));
 				binding.kind =

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

@@ -1,5 +1,5 @@
-/** @import { AssignmentExpression, Expression, Pattern, PrivateIdentifier, Super, UpdateExpression, VariableDeclarator } from 'estree' */
-/** @import { AST } from '#compiler' */
+/** @import { AssignmentExpression, Expression, Identifier, Pattern, PrivateIdentifier, Super, UpdateExpression, VariableDeclarator } from 'estree' */
+/** @import { AST, Binding } from '#compiler' */
 /** @import { AnalysisState, Context } from '../../types' */
 /** @import { Scope } from '../../../scope' */
 /** @import { NodeLike } from '../../../../errors.js' */
@@ -196,3 +196,32 @@ export function is_pure(node, context) {
 	// TODO add more cases (safe Svelte imports, etc)
 	return false;
 }
+
+/**
+ * Checks if the name is valid, which it is when it's not starting with (or is) a dollar sign or if it's a function parameter.
+ * The second argument is the depth of the scope, which is there for backwards compatibility reasons: In Svelte 4, you
+ * were allowed to define `$`-prefixed variables anywhere below the top level of components. Once legacy mode is gone, this
+ * argument can be removed / the call sites adjusted accordingly.
+ * @param {Binding | null} binding
+ * @param {number | undefined} [function_depth]
+ */
+export function validate_identifier_name(binding, function_depth) {
+	if (!binding) return;
+
+	const declaration_kind = binding.declaration_kind;
+
+	if (
+		declaration_kind !== 'synthetic' &&
+		declaration_kind !== 'param' &&
+		declaration_kind !== 'rest_param' &&
+		(!function_depth || function_depth <= 1)
+	) {
+		const node = binding.node;
+
+		if (node.name === '$') {
+			e.dollar_binding_invalid(node);
+		} else if (node.name.startsWith('$')) {
+			e.dollar_prefix_invalid(node);
+		}
+	}
+}

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

@@ -14,6 +14,7 @@ import {
 } from '../utils/ast.js';
 import { is_reserved, is_rune } from '../../utils.js';
 import { determine_slot } from '../utils/slot.js';
+import { validate_identifier_name } from './2-analyze/visitors/shared/utils.js';
 
 export class Scope {
 	/** @type {ScopeRoot} */
@@ -78,20 +79,6 @@ export class Scope {
 	 * @returns {Binding}
 	 */
 	declare(node, kind, declaration_kind, initial = null) {
-		if (node.name === '$') {
-			e.dollar_binding_invalid(node);
-		}
-
-		if (
-			node.name.startsWith('$') &&
-			declaration_kind !== 'synthetic' &&
-			declaration_kind !== 'param' &&
-			declaration_kind !== 'rest_param' &&
-			this.function_depth <= 1
-		) {
-			e.dollar_prefix_invalid(node);
-		}
-
 		if (this.parent) {
 			if (declaration_kind === 'var' && this.#porous) {
 				return this.parent.declare(node, kind, declaration_kind);
@@ -123,6 +110,9 @@ export class Scope {
 			prop_alias: null,
 			metadata: null
 		};
+
+		validate_identifier_name(binding, this.function_depth);
+
 		this.declarations.set(node.name, binding);
 		this.root.conflicts.add(node.name);
 		return binding;

packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/_config.js

@@ -0,0 +1,9 @@
+import { test } from '../../test';
+
+export default test({
+	error: {
+		code: 'dollar_binding_invalid',
+		message: 'The $ name is reserved, and cannot be used for variables and imports',
+		position: [108, 109]
+	}
+});

packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/main.svelte

@@ -0,0 +1,11 @@
+<svelte:options runes={false} />
+
+<script>
+	function ok($) {}
+	function ok2() {
+		let $;
+	}
+
+	// error
+	let $;
+</script>

packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/_config.js renamed to packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/_config.js

@@ -0,0 +1,8 @@
+<svelte:options runes />
+
+<script>
+	function ok($) {}
+	function error() {
+		let $;
+	}
+</script>

packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/main.svelte

@@ -0,0 +1,8 @@
+import { test } from '../../test';
+
+export default test({
+	error: {
+		code: 'dollar_binding_invalid',
+		message: 'The $ name is reserved, and cannot be used for variables and imports'
+	}
+});

packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/_config.js

@@ -1,3 +1,5 @@
+<svelte:options runes />
+
 <script>
 	let $;
 </script>