feat: add svelte/no-immutable-reactive-statements rule (#439)

.changeset/curvy-bananas-pretend.md

@@ -0,0 +1,5 @@
+---
+"eslint-plugin-svelte": minor
+---
+
+feat: add `svelte/no-immutable-reactive-statements` rule

README.md

@@ -339,6 +339,7 @@ These rules relate to better ways of doing things to help you avoid problems:
 | [svelte/block-lang](https://sveltejs.github.io/eslint-plugin-svelte/rules/block-lang/) | disallows the use of languages other than those specified in the configuration for the lang attribute of `<script>` and `<style>` blocks. |  |
 | [svelte/button-has-type](https://sveltejs.github.io/eslint-plugin-svelte/rules/button-has-type/) | disallow usage of button without an explicit type attribute |  |
 | [svelte/no-at-debug-tags](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-at-debug-tags/) | disallow the use of `{@debug}` | :star: |
+| [svelte/no-immutable-reactive-statements](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-immutable-reactive-statements/) | disallow reactive statements that don't reference reactive values. |  |
 | [svelte/no-reactive-functions](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-reactive-functions/) | it's not necessary to define functions in reactive statements | :bulb: |
 | [svelte/no-reactive-literals](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-reactive-literals/) | don't assign literal values in reactive statements | :bulb: |
 | [svelte/no-unused-svelte-ignore](https://sveltejs.github.io/eslint-plugin-svelte/rules/no-unused-svelte-ignore/) | disallow unused svelte-ignore comments | :star: |

docs/rules.md

@@ -52,6 +52,7 @@ These rules relate to better ways of doing things to help you avoid problems:
 | [svelte/block-lang](./rules/block-lang.md) | disallows the use of languages other than those specified in the configuration for the lang attribute of `<script>` and `<style>` blocks. |  |
 | [svelte/button-has-type](./rules/button-has-type.md) | disallow usage of button without an explicit type attribute |  |
 | [svelte/no-at-debug-tags](./rules/no-at-debug-tags.md) | disallow the use of `{@debug}` | :star: |
+| [svelte/no-immutable-reactive-statements](./rules/no-immutable-reactive-statements.md) | disallow reactive statements that don't reference reactive values. |  |
 | [svelte/no-reactive-functions](./rules/no-reactive-functions.md) | it's not necessary to define functions in reactive statements | :bulb: |
 | [svelte/no-reactive-literals](./rules/no-reactive-literals.md) | don't assign literal values in reactive statements | :bulb: |
 | [svelte/no-unused-svelte-ignore](./rules/no-unused-svelte-ignore.md) | disallow unused svelte-ignore comments | :star: |

docs/rules/no-immutable-reactive-statements.md

@@ -0,0 +1,67 @@
+---
+pageClass: "rule-details"
+sidebarDepth: 0
+title: "svelte/no-immutable-reactive-statements"
+description: "disallow reactive statements that don't reference reactive values."
+---
+
+# svelte/no-immutable-reactive-statements
+
+> disallow reactive statements that don't reference reactive values.
+
+- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> **_This rule has not been released yet._** </badge>
+
+## :book: Rule Details
+
+This rule reports if all variables referenced in reactive statements are immutable. That reactive statement is immutable and not reactive.
+
+<ESLintCodeBlock>
+
+<!--eslint-skip-->
+
+```svelte
+<script>
+  /* eslint svelte/no-immutable-reactive-statements: "error" */
+  import myStore from "./my-stores"
+  import myVar from "./my-variables"
+  let mutableVar = "hello"
+  export let prop
+  /* ✓ GOOD */
+  $: computed1 = mutableVar + " " + mutableVar
+  $: computed2 = fn1(mutableVar)
+  $: console.log(mutableVar)
+  $: console.log(computed1)
+  $: console.log($myStore)
+  $: console.log(prop)
+
+  const immutableVar = "hello"
+  /* ✗ BAD */
+  $: computed3 = fn1(immutableVar)
+  $: computed4 = fn2()
+  $: console.log(immutableVar)
+  $: console.log(myVar)
+
+  /* ignore */
+  $: console.log(unknown)
+
+  function fn1(v) {
+    return v + " " + v
+  }
+  function fn2() {
+    return mutableVar + " " + mutableVar
+  }
+</script>
+
+<input bind:value={mutableVar} />
+```
+
+</ESLintCodeBlock>
+
+## :wrench: Options
+
+Nothing.
+
+## :mag: Implementation
+
+- [Rule source](https://github.com/sveltejs/eslint-plugin-svelte/blob/main/src/rules/no-immutable-reactive-statements.ts)
+- [Test source](https://github.com/sveltejs/eslint-plugin-svelte/blob/main/tests/src/rules/no-immutable-reactive-statements.ts)

src/rules/no-immutable-reactive-statements.ts

@@ -0,0 +1,162 @@
+import type { AST } from "svelte-eslint-parser"
+import { createRule } from "../utils"
+import type {
+  Scope,
+  Variable,
+  Reference,
+  Definition,
+} from "@typescript-eslint/scope-manager"
+
+export default createRule("no-immutable-reactive-statements", {
+  meta: {
+    docs: {
+      description:
+        "disallow reactive statements that don't reference reactive values.",
+      category: "Best Practices",
+      // TODO Switch to recommended in the major version.
+      recommended: false,
+    },
+    schema: [],
+    messages: {
+      immutable:
+        "This statement is not reactive because all variables referenced in the reactive statement are immutable.",
+    },
+    type: "suggestion",
+  },
+  create(context) {
+    const scopeManager = context.getSourceCode().scopeManager
+    const globalScope = scopeManager.globalScope
+    const toplevelScope =
+      globalScope?.childScopes.find((scope) => scope.type === "module") ||
+      globalScope
+    if (!globalScope || !toplevelScope) {
+      return {}
+    }
+
+    const cacheMutableVariable = new WeakMap<Variable, boolean>()
+
+    /**
+     * Checks whether the given reference is a mutable variable or not.
+     */
+    function isMutableVariableReference(reference: Reference) {
+      if (reference.identifier.name.startsWith("$")) {
+        // It is reactive store reference.
+        return true
+      }
+      if (!reference.resolved) {
+        // Unknown variable
+        return true
+      }
+      return isMutableVariable(reference.resolved)
+    }
+
+    /**
+     * Checks whether the given variable is a mutable variable or not.
+     */
+    function isMutableVariable(variable: Variable) {
+      const cache = cacheMutableVariable.get(variable)
+      if (cache != null) {
+        return cache
+      }
+      if (variable.defs.length === 0) {
+        // Global variables are assumed to be immutable.
+        return true
+      }
+      const isMutable = variable.defs.some((def) => {
+        if (def.type === "Variable") {
+          const parent = def.parent
+          if (parent.kind === "const") {
+            return false
+          }
+          const pp = parent.parent
+          if (
+            pp &&
+            pp.type === "ExportNamedDeclaration" &&
+            pp.declaration === parent
+          ) {
+            // Props
+            return true
+          }
+          return hasWrite(variable)
+        }
+        if (def.type === "ImportBinding") {
+          return false
+        }
+
+        if (def.node.type === "AssignmentExpression") {
+          // Reactive values
+          return true
+        }
+        return false
+      })
+      cacheMutableVariable.set(variable, isMutable)
+      return isMutable
+    }
+
+    /** Checks whether the given variable has a write or reactive store reference or not. */
+    function hasWrite(variable: Variable) {
+      const defIds = variable.defs.map((def: Definition) => def.name)
+      return variable.references.some(
+        (reference) =>
+          reference.isWrite() &&
+          !defIds.some(
+            (defId) =>
+              defId.range[0] <= reference.identifier.range[0] &&
+              reference.identifier.range[1] <= defId.range[1],
+          ),
+      )
+    }
+
+    /**
+     * Iterates through references to top-level variables in the given range.
+     */
+    function* iterateRangeReferences(scope: Scope, range: [number, number]) {
+      for (const variable of scope.variables) {
+        for (const reference of variable.references) {
+          if (
+            range[0] <= reference.identifier.range[0] &&
+            reference.identifier.range[1] <= range[1]
+          ) {
+            yield reference
+          }
+        }
+      }
+    }
+
+    return {
+      SvelteReactiveStatement(node: AST.SvelteReactiveStatement) {
+        for (const reference of iterateRangeReferences(
+          toplevelScope,
+          node.range,
+        )) {
+          if (reference.isWriteOnly()) {
+            continue
+          }
+          if (isMutableVariableReference(reference)) {
+            return
+          }
+        }
+        if (
+          globalScope.through.some(
+            (reference) =>
+              node.range[0] <= reference.identifier.range[0] &&
+              reference.identifier.range[1] <= node.range[1],
+          )
+        ) {
+          // Do not report if there are missing references.
+          return
+        }
+
+        context.report({
+          node:
+            node.body.type === "ExpressionStatement" &&
+            node.body.expression.type === "AssignmentExpression" &&
+            node.body.expression.operator === "="
+              ? node.body.expression.right
+              : node.body,
+          messageId: "immutable",
+        })
+      },
+    }
+  },
+})

src/utils/rules.ts

@@ -24,6 +24,7 @@ import noDupeUseDirectives from "../rules/no-dupe-use-directives"
 import noDynamicSlotName from "../rules/no-dynamic-slot-name"
 import noExportLoadInSvelteModuleInKitPages from "../rules/no-export-load-in-svelte-module-in-kit-pages"
 import noExtraReactiveCurlies from "../rules/no-extra-reactive-curlies"
+import noImmutableReactiveStatements from "../rules/no-immutable-reactive-statements"
 import noInnerDeclarations from "../rules/no-inner-declarations"
 import noNotFunctionHandler from "../rules/no-not-function-handler"
 import noObjectInTextMustaches from "../rules/no-object-in-text-mustaches"
@@ -79,6 +80,7 @@ export const rules = [
   noDynamicSlotName,
   noExportLoadInSvelteModuleInKitPages,
   noExtraReactiveCurlies,
+  noImmutableReactiveStatements,
   noInnerDeclarations,
   noNotFunctionHandler,
   noObjectInTextMustaches,

tests/fixtures/rules/no-immutable-reactive-statements/invalid/immutable-let-errors.yaml

@@ -0,0 +1,18 @@
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 3
+  column: 18
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 4
+  column: 18
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 5
+  column: 6
+  suggestions: null

tests/fixtures/rules/no-immutable-reactive-statements/invalid/immutable-let-input.svelte

@@ -0,0 +1,10 @@
+<script>
+  let immutableVar = "hello"
+  $: computed1 = `${immutableVar} ${immutableVar}`
+  $: computed1 = fn1(immutableVar)
+  $: console.log(immutableVar)
+
+  function fn1(v) {
+    return `${v} ${v}`
+  }
+</script>

tests/fixtures/rules/no-immutable-reactive-statements/invalid/immutable01-errors.yaml

@@ -0,0 +1,24 @@
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 7
+  column: 18
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 8
+  column: 18
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 9
+  column: 6
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 10
+  column: 6
+  suggestions: null

tests/fixtures/rules/no-immutable-reactive-statements/invalid/immutable01-input.svelte

@@ -0,0 +1,20 @@
+<script>
+  import myVar from "./my-variables"
+  let mutableVar = "hello"
+
+  const immutableVar = "hello"
+  /* ✗ BAD */
+  $: computed3 = fn1(immutableVar)
+  $: computed4 = fn2()
+  $: console.log(immutableVar)
+  $: console.log(myVar)
+
+  function fn1(v) {
+    return `${v} ${v}`
+  }
+  function fn2() {
+    return `${mutableVar} ${mutableVar}`
+  }
+</script>
+
+<input bind:value={mutableVar} />

tests/fixtures/rules/no-immutable-reactive-statements/invalid/readonly-export01-errors.yaml

@@ -0,0 +1,18 @@
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 12
+  column: 17
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 13
+  column: 17
+  suggestions: null
+- message:
+    This statement is not reactive because all variables referenced in the
+    reactive statement are immutable.
+  line: 14
+  column: 17
+  suggestions: null

tests/fixtures/rules/no-immutable-reactive-statements/invalid/readonly-export01-input.svelte

@@ -0,0 +1,15 @@
+<script>
+  export const thisIs = "readonly"
+
+  export function greet(name) {
+    console.log(`hello ${name}!`)
+  }
+
+  export class Foo {}
+
+  const immutableVar = "hello"
+
+  $: message1 = greet(immutableVar)
+  $: message2 = `this is${thisIs}`
+  $: instance = new Foo()
+</script>

tests/fixtures/rules/no-immutable-reactive-statements/valid/mutable01-input.svelte

@@ -0,0 +1,19 @@
+<script>
+  import myStore from "./my-stores"
+  import myVar from "./my-variables"
+  let mutableVar = "hello"
+  export let prop
+  /* ✓ GOOD */
+  $: computed1 = `${mutableVar} ${mutableVar}`
+  $: computed2 = fn1(mutableVar)
+  $: console.log(mutableVar)
+  $: console.log(computed1)
+  $: console.log($myStore)
+  $: console.log(prop)
+
+  function fn1(v) {
+    return `${v} ${v}`
+  }
+</script>
+
+<input bind:value={mutableVar} />