From 57a4b5d19c0d835f9a66c36a0a2cb541f63e2a86 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 15 Aug 2024 05:26:49 -0400 Subject: [PATCH] feat: better compiler warnings for non-reactive dependencies of reactive statements (#12824) * feat: better compiler warnings for non-reactive dependencies of reactive statements * fix * regenerate --- .changeset/strange-pillows-greet.md | 5 ++++ .../messages/compile-warnings/script.md | 8 ++++-- .../phases/2-analyze/visitors/Identifier.js | 8 ++++++ .../2-analyze/visitors/LabeledStatement.js | 10 +------ .../2-analyze/visitors/MemberExpression.js | 28 ++++++++++++++++++- .../phases/3-transform/client/utils.js | 7 +++-- .../client/visitors/VariableDeclaration.js | 2 +- .../client/visitors/shared/declarations.js | 2 +- packages/svelte/src/compiler/warnings.js | 17 ++++++++--- .../reactive-module-variable/input.svelte | 7 ++++- .../reactive-module-variable/warnings.json | 12 ++++---- 11 files changed, 78 insertions(+), 28 deletions(-) create mode 100644 .changeset/strange-pillows-greet.md diff --git a/.changeset/strange-pillows-greet.md b/.changeset/strange-pillows-greet.md new file mode 100644 index 0000000000..bfb08a11d9 --- /dev/null +++ b/.changeset/strange-pillows-greet.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +feat: better compiler warnings for non-reactive dependencies of reactive statements diff --git a/packages/svelte/messages/compile-warnings/script.md b/packages/svelte/messages/compile-warnings/script.md index a31655b972..3d55da289f 100644 --- a/packages/svelte/messages/compile-warnings/script.md +++ b/packages/svelte/messages/compile-warnings/script.md @@ -26,9 +26,13 @@ > Reactive declarations only exist at the top level of the instance script -## reactive_declaration_module_script +## reactive_declaration_module_script_dependency -> All dependencies of the reactive declaration are declared in a module script and will not be reactive +> 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 diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js index dcea431c1a..1f3406e92e 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js @@ -109,5 +109,13 @@ export function Identifier(node, context) { ) { w.state_referenced_locally(node); } + + if ( + context.state.reactive_statement && + binding.scope === context.state.analysis.module.scope && + binding.reassigned + ) { + w.reactive_declaration_module_script_dependency(node); + } } } diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/LabeledStatement.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/LabeledStatement.js index 9be8c161d4..0ff8c37ade 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/LabeledStatement.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/LabeledStatement.js @@ -4,6 +4,7 @@ import * as e from '../../../errors.js'; import { extract_identifiers, object } from '../../../utils/ast.js'; import * as w from '../../../warnings.js'; +import { is_state_source } from '../../3-transform/client/utils.js'; /** * @param {LabeledStatement} node @@ -66,15 +67,6 @@ export function LabeledStatement(node, context) { context.state.reactive_statements.set(node, reactive_statement); - if ( - reactive_statement.dependencies.length && - reactive_statement.dependencies.every( - (d) => d.scope === context.state.analysis.module.scope && d.declaration_kind !== 'const' - ) - ) { - w.reactive_declaration_module_script(node); - } - if ( node.body.type === 'ExpressionStatement' && node.body.expression.type === 'AssignmentExpression' diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js index 245a164c71..ca67bf47ec 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js @@ -1,6 +1,8 @@ -/** @import { MemberExpression } from 'estree' */ +/** @import { MemberExpression, Node } from 'estree' */ /** @import { Context } from '../types' */ import * as e from '../../../errors.js'; +import * as w from '../../../warnings.js'; +import { object } from '../../../utils/ast.js'; import { is_pure, is_safe_identifier } from './shared/utils.js'; /** @@ -23,5 +25,29 @@ 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') + ) { + if (parent.type !== 'MemberExpression' && parent.type !== 'CallExpression') { + w.reactive_declaration_non_reactive_property(node); + } + } + } + } + } + context.next(); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index 965b8d80a5..35283aed72 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -1,6 +1,7 @@ /** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */ /** @import { Binding, SvelteNode } from '#compiler' */ /** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */ +/** @import { Analysis } from '../../types.js' */ /** @import { Scope } from '../../scope.js' */ import * as b from '../../../utils/builders.js'; import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js'; @@ -16,13 +17,13 @@ import { get_value } from './visitors/shared/declarations.js'; /** * @param {Binding} binding - * @param {ClientTransformState} state + * @param {Analysis} analysis * @returns {boolean} */ -export function is_state_source(binding, state) { +export function is_state_source(binding, analysis) { return ( (binding.kind === 'state' || binding.kind === 'raw_state') && - (!state.analysis.immutable || binding.reassigned || state.analysis.accessors) + (!analysis.immutable || binding.reassigned || analysis.accessors) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js index 8d36046be3..d8106f9d7d 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js @@ -126,7 +126,7 @@ export function VariableDeclaration(node, context) { if (rune === '$state' && should_proxy(value, context.state.scope)) { value = b.call('$.proxy', value); } - if (is_state_source(binding, context.state)) { + if (is_state_source(binding, context.state.analysis)) { value = b.call('$.source', value); } return value; diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/declarations.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/declarations.js index 0ea6a7ff6d..33566f1c10 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/declarations.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/declarations.js @@ -18,7 +18,7 @@ export function get_value(node) { export function add_state_transformers(context) { for (const [name, binding] of context.state.scope.declarations) { if ( - is_state_source(binding, context.state) || + is_state_source(binding, context.state.analysis) || binding.kind === 'derived' || binding.kind === 'legacy_reactive' ) { diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index 0aab91b540..90702409b3 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -101,7 +101,8 @@ export const codes = [ "perf_avoid_inline_class", "perf_avoid_nested_class", "reactive_declaration_invalid_placement", - "reactive_declaration_module_script", + "reactive_declaration_module_script_dependency", + "reactive_declaration_non_reactive_property", "state_referenced_locally", "store_rune_conflict", "css_unused_selector", @@ -630,11 +631,19 @@ export function reactive_declaration_invalid_placement(node) { } /** - * All dependencies of the reactive declaration are declared in a module script and will not be reactive + * Reassignments of module-level declarations will not cause reactive statements to update * @param {null | NodeLike} node */ -export function reactive_declaration_module_script(node) { - w(node, "reactive_declaration_module_script", "All dependencies of the reactive declaration are declared in a module script and will not be reactive"); +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"); } /** diff --git a/packages/svelte/tests/validator/samples/reactive-module-variable/input.svelte b/packages/svelte/tests/validator/samples/reactive-module-variable/input.svelte index 5a540de4f0..ebf98c0e9e 100644 --- a/packages/svelte/tests/validator/samples/reactive-module-variable/input.svelte +++ b/packages/svelte/tests/validator/samples/reactive-module-variable/input.svelte @@ -1,6 +1,11 @@ + diff --git a/packages/svelte/tests/validator/samples/reactive-module-variable/warnings.json b/packages/svelte/tests/validator/samples/reactive-module-variable/warnings.json index b401db5008..9d5105db82 100644 --- a/packages/svelte/tests/validator/samples/reactive-module-variable/warnings.json +++ b/packages/svelte/tests/validator/samples/reactive-module-variable/warnings.json @@ -1,14 +1,14 @@ [ { - "code": "reactive_declaration_module_script", - "message": "All dependencies of the reactive declaration are declared in a module script and will not be reactive", + "code": "reactive_declaration_module_script_dependency", + "message": "Reassignments of module-level declarations will not cause reactive statements to update", "start": { - "column": 1, - "line": 5 + "column": 10, + "line": 10 }, "end": { - "column": 14, - "line": 5 + "column": 13, + "line": 10 } } ]