From d9369d8e309640f71d7a8559dab8c5fb0ee982e4 Mon Sep 17 00:00:00 2001
From: Simon H <5968653+dummdidumm@users.noreply.github.com>
Date: Mon, 16 Sep 2024 15:07:48 +0200
Subject: [PATCH] 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
---
.changeset/twelve-shrimps-run.md | 5 +++
.../2-analyze/visitors/ClassDeclaration.js | 5 +++
.../2-analyze/visitors/FunctionDeclaration.js | 5 +++
.../2-analyze/visitors/VariableDeclarator.js | 9 +++--
.../phases/2-analyze/visitors/shared/utils.js | 33 +++++++++++++++++--
packages/svelte/src/compiler/phases/scope.js | 18 +++-------
.../_config.js | 9 +++++
.../main.svelte | 11 +++++++
.../_config.js | 0
.../main.svelte | 8 +++++
.../_config.js | 8 +++++
.../main.svelte | 2 ++
12 files changed, 95 insertions(+), 18 deletions(-)
create mode 100644 .changeset/twelve-shrimps-run.md
create mode 100644 packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/_config.js
create mode 100644 packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/main.svelte
rename packages/svelte/tests/compiler-errors/samples/{dollar-binding-declaration => dollar-binding-declaration-runes-2}/_config.js (100%)
create mode 100644 packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/main.svelte
create mode 100644 packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/_config.js
rename packages/svelte/tests/compiler-errors/samples/{dollar-binding-declaration => dollar-binding-declaration-runes}/main.svelte (50%)
diff --git a/.changeset/twelve-shrimps-run.md b/.changeset/twelve-shrimps-run.md
new file mode 100644
index 0000000000..70b6a4a527
--- /dev/null
+++ b/.changeset/twelve-shrimps-run.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: tighten up `$` prefix validation
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ClassDeclaration.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ClassDeclaration.js
index 49be955eba..ec865c8313 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/visitors/ClassDeclaration.js
+++ b/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.
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/FunctionDeclaration.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/FunctionDeclaration.js
index 6d7074628e..2e996431d1 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/visitors/FunctionDeclaration.js
+++ b/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);
}
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/VariableDeclarator.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/VariableDeclarator.js
index 6d9b3e29b9..a7d08d315d 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/visitors/VariableDeclarator.js
+++ b/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 =
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/utils.js
index dad55a1152..266016fcc8 100644
--- a/packages/svelte/src/compiler/phases/2-analyze/visitors/shared/utils.js
+++ b/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);
+ }
+ }
+}
diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js
index 2f25cb251d..c1cf1e4055 100644
--- a/packages/svelte/src/compiler/phases/scope.js
+++ b/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;
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/_config.js b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/_config.js
new file mode 100644
index 0000000000..cd7851d05e
--- /dev/null
+++ b/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]
+ }
+});
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/main.svelte b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/main.svelte
new file mode 100644
index 0000000000..3276e84bec
--- /dev/null
+++ b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-legacy/main.svelte
@@ -0,0 +1,11 @@
+
+
+
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/_config.js b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/_config.js
similarity index 100%
rename from packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/_config.js
rename to packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/_config.js
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/main.svelte b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/main.svelte
new file mode 100644
index 0000000000..e24add5453
--- /dev/null
+++ b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes-2/main.svelte
@@ -0,0 +1,8 @@
+
+
+
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/_config.js b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/_config.js
new file mode 100644
index 0000000000..cd7a4ab291
--- /dev/null
+++ b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/_config.js
@@ -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'
+ }
+});
diff --git a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/main.svelte b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/main.svelte
similarity index 50%
rename from packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/main.svelte
rename to packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/main.svelte
index 019c9cf7b4..42e702447f 100644
--- a/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration/main.svelte
+++ b/packages/svelte/tests/compiler-errors/samples/dollar-binding-declaration-runes/main.svelte
@@ -1,3 +1,5 @@
+
+