From 6e863e617c993dd6d742447391c60e61a50ee562 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 27 Nov 2023 11:25:08 -0500 Subject: [PATCH] feat: warn on referenced mutated nonstate (#9669) Walk the path and warn if this is a mutated normal variable that's referenced inside a function scope --- .changeset/friendly-lies-camp.md | 5 ++++ .../src/compiler/phases/2-analyze/index.js | 28 +++++++++++++++++-- packages/svelte/src/compiler/phases/scope.js | 8 +++--- packages/svelte/src/compiler/warnings.js | 7 +++-- .../runes-referenced-nonstate/_config.js | 3 ++ .../runes-referenced-nonstate/input.svelte | 10 +++++++ .../runes-referenced-nonstate/warnings.json | 26 +++++++++++++++++ .../warnings.json | 2 +- 8 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 .changeset/friendly-lies-camp.md create mode 100644 packages/svelte/tests/validator/samples/runes-referenced-nonstate/_config.js create mode 100644 packages/svelte/tests/validator/samples/runes-referenced-nonstate/input.svelte create mode 100644 packages/svelte/tests/validator/samples/runes-referenced-nonstate/warnings.json diff --git a/.changeset/friendly-lies-camp.md b/.changeset/friendly-lies-camp.md new file mode 100644 index 0000000000..fe88999868 --- /dev/null +++ b/.changeset/friendly-lies-camp.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +feat: warn on references to mutated non-state in template diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 68f3ac599f..a278c89272 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -218,7 +218,7 @@ export function analyze_module(ast, options) { for (const [, scope] of scopes) { for (const [name, binding] of scope.declarations) { if (binding.kind === 'state' && !binding.mutated) { - warn(warnings, binding.node, [], 'state-rune-not-mutated', name); + warn(warnings, binding.node, [], 'state-not-mutated', name); } } } @@ -377,7 +377,7 @@ export function analyze_component(root, options) { for (const [, scope] of instance.scopes) { for (const [name, binding] of scope.declarations) { if (binding.kind === 'state' && !binding.mutated) { - warn(warnings, binding.node, [], 'state-rune-not-mutated', name); + warn(warnings, binding.node, [], 'state-not-mutated', name); } } } @@ -414,6 +414,30 @@ 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]) { + outer: for (const [name, binding] of scope.declarations) { + if (binding.kind === 'normal' && binding.mutated) { + for (const { path } of binding.references) { + if (path[0].type !== 'Fragment') continue; + for (let i = 1; i < path.length; i += 1) { + const type = path[i].type; + if ( + type === 'FunctionDeclaration' || + type === 'FunctionExpression' || + type === 'ArrowFunctionExpression' + ) { + continue; + } + } + + warn(warnings, binding.node, [], 'non-state-reference', name); + continue outer; + } + } + } + } + analysis.stylesheet.validate(analysis); for (const element of analysis.elements) { diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index 86d0eab67a..e958df604e 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -175,13 +175,13 @@ export class Scope { 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) { + binding.references.push({ node, path }); } else if (this.#parent) { this.#parent.reference(node, path); } 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); } diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index 23e9378c25..6c38b1e052 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -22,8 +22,11 @@ const runes = { `It looks like you're using the $${name} rune, but there is a local binding called ${name}. ` + `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?` + 'state-not-mutated': (name) => + `${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} */ diff --git a/packages/svelte/tests/validator/samples/runes-referenced-nonstate/_config.js b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/_config.js new file mode 100644 index 0000000000..f47bee71df --- /dev/null +++ b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/_config.js @@ -0,0 +1,3 @@ +import { test } from '../../test'; + +export default test({}); diff --git a/packages/svelte/tests/validator/samples/runes-referenced-nonstate/input.svelte b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/input.svelte new file mode 100644 index 0000000000..fd9d6c3173 --- /dev/null +++ b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/input.svelte @@ -0,0 +1,10 @@ + + + + + +

{a} + {b} + {c} = {a + b + c}

diff --git a/packages/svelte/tests/validator/samples/runes-referenced-nonstate/warnings.json b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/warnings.json new file mode 100644 index 0000000000..7e251a4e70 --- /dev/null +++ b/packages/svelte/tests/validator/samples/runes-referenced-nonstate/warnings.json @@ -0,0 +1,26 @@ +[ + { + "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 + } + }, + { + "code": "non-state-reference", + "message": "c is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates.", + "start": { + "column": 5, + "line": 4 + }, + "end": { + "column": 6, + "line": 4 + } + } +] diff --git a/packages/svelte/tests/validator/samples/runes-state-rune-not-mutated/warnings.json b/packages/svelte/tests/validator/samples/runes-state-rune-not-mutated/warnings.json index 628f1f2e9d..5d2b639c8d 100644 --- a/packages/svelte/tests/validator/samples/runes-state-rune-not-mutated/warnings.json +++ b/packages/svelte/tests/validator/samples/runes-state-rune-not-mutated/warnings.json @@ -1,6 +1,6 @@ [ { - "code": "state-rune-not-mutated", + "code": "state-not-mutated", "end": { "column": 11, "line": 3