Add composition api's computed function support to vue/no-side-effect…

…s-in-computed-properties close #1393 (#1407) * Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 * check targetBody * fix false negative with arr.reverse() * only report variables declared inside setup
vuejs · Jan 21, 2021 · 3f20a08 · 3f20a08
1 parent d7eacd6
commit 3f20a08
Show file tree

Hide file tree

Showing 3 changed files with 431 additions and 73 deletions.
diff --git a/docs/rules/no-side-effects-in-computed-properties.md b/docs/rules/no-side-effects-in-computed-properties.md
@@ -2,20 +2,20 @@
 pageClass: rule-details
 sidebarDepth: 0
 title: vue/no-side-effects-in-computed-properties
-description: disallow side effects in computed properties
+description: disallow side effects in computed properties and functions
 since: v3.6.0
 ---
 # vue/no-side-effects-in-computed-properties
 
-> disallow side effects in computed properties
+> disallow side effects in computed properties and functions
 
 - :gear: This rule is included in all of `"plugin:vue/vue3-essential"`, `"plugin:vue/essential"`, `"plugin:vue/vue3-strongly-recommended"`, `"plugin:vue/strongly-recommended"`, `"plugin:vue/vue3-recommended"` and `"plugin:vue/recommended"`.
 
 ## :book: Rule Details
 
-This rule is aimed at preventing the code which makes side effects in computed properties.
+This rule is aimed at preventing the code which makes side effects in computed properties and functions.
 
-It is considered a very bad practice to introduce side effects inside computed properties. It makes the code not predictable and hard to understand.
+It is considered a very bad practice to introduce side effects inside computed properties and functions. It makes the code not predictable and hard to understand.
 
 <eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">
 
@@ -58,6 +58,51 @@ export default {
 
 </eslint-code-block>
 
+<eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">
+
+```vue
+<script>
+import {computed} from 'vue'
+/* ✓ GOOD */
+export default {
+  setup() {
+    const foo = useFoo()
+
+    const fullName = computed(() => `${foo.firstName} ${foo.lastName}`)
+    const reversedArray = computed(() => {
+      return foo.array.slice(0).reverse() // .slice makes a copy of the array, instead of mutating the orginal
+    })
+  }
+}
+</script>
+```
+
+</eslint-code-block>
+
+<eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">
+
+```vue
+<script>
+import {computed} from 'vue'
+/* ✗ BAD */
+export default {
+  setup() {
+    const foo = useFoo()
+    
+    const fullName = computed(() => {
+      foo.firstName = 'lorem' // <- side effect
+      return `${foo.firstName} ${foo.lastName}`
+    })
+    const reversedArray = computed(() => {
+      return foo.array.reverse() // <- side effect - orginal array is being mutated
+    })
+  }
+}
+</script>
+```
+
+</eslint-code-block>
+
 ## :wrench: Options
 
 Nothing.

diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js
@@ -3,7 +3,7 @@
  * @author Michał Sajnóg
  */
 'use strict'
-
+const { ReferenceTracker, findVariable } = require('eslint-utils')
 const utils = require('../utils')
 
 /**
@@ -31,6 +31,10 @@ module.exports = {
   create(context) {
     /** @type {Map<ObjectExpression, ComponentComputedProperty[]>} */
     const computedPropertiesMap = new Map()
+    /** @type {Array<FunctionExpression | ArrowFunctionExpression>} */
+    const computedCallNodes = []
+    /** @type {Array<FunctionExpression | ArrowFunctionExpression | FunctionDeclaration>} */
+    const setupFunctions = []
 
     /**
      * @typedef {object} ScopeStack
@@ -54,56 +58,143 @@ module.exports = {
       scopeStack = scopeStack && scopeStack.upper
     }
 
-    return utils.defineVueVisitor(context, {
-      onVueObjectEnter(node) {
-        computedPropertiesMap.set(node, utils.getComputedProperties(node))
-      },
-      ':function': onFunctionEnter,
-      ':function:exit': onFunctionExit,
-
-      /**
-       * @param {(Identifier | ThisExpression) & {parent: MemberExpression}} node
-       * @param {VueObjectData} data
-       */
-      'MemberExpression > :matches(Identifier, ThisExpression)'(
-        node,
-        { node: vueNode }
-      ) {
-        if (!scopeStack) {
-          return
-        }
-        const targetBody = scopeStack.body
-        const computedProperty = /** @type {ComponentComputedProperty[]} */ (computedPropertiesMap.get(
-          vueNode
-        )).find((cp) => {
-          return (
-            cp.value &&
-            node.loc.start.line >= cp.value.loc.start.line &&
-            node.loc.end.line <= cp.value.loc.end.line &&
-            targetBody === cp.value
-          )
-        })
-        if (!computedProperty) {
-          return
-        }
+    return Object.assign(
+      {
+        Program() {
+          const tracker = new ReferenceTracker(context.getScope())
+          const traceMap = utils.createCompositionApiTraceMap({
+            [ReferenceTracker.ESM]: true,
+            computed: {
+              [ReferenceTracker.CALL]: true
+            }
+          })
 
-        if (!utils.isThis(node, context)) {
-          return
-        }
-        const mem = node.parent
-        if (mem.object !== node) {
-          return
+          for (const { node } of tracker.iterateEsmReferences(traceMap)) {
+            if (node.type !== 'CallExpression') {
+              continue
+            }
+
+            const getterBody = utils.getGetterBodyFromComputedFunction(node)
+            if (getterBody) {
+              computedCallNodes.push(getterBody)
+            }
+          }
         }
+      },
+      utils.defineVueVisitor(context, {
+        onVueObjectEnter(node) {
+          computedPropertiesMap.set(node, utils.getComputedProperties(node))
+        },
+        ':function': onFunctionEnter,
+        ':function:exit': onFunctionExit,
+        onSetupFunctionEnter(node) {
+          setupFunctions.push(node)
+        },
 
-        const invalid = utils.findMutating(mem)
-        if (invalid) {
-          context.report({
-            node: invalid.node,
-            message: 'Unexpected side effect in "{{key}}" computed property.',
-            data: { key: computedProperty.key || 'Unknown' }
+        /**
+         * @param {(Identifier | ThisExpression) & {parent: MemberExpression}} node
+         * @param {VueObjectData} data
+         */
+        'MemberExpression > :matches(Identifier, ThisExpression)'(
+          node,
+          { node: vueNode }
+        ) {
+          if (!scopeStack) {
+            return
+          }
+          const targetBody = scopeStack.body
+
+          const computedProperty = /** @type {ComponentComputedProperty[]} */ (computedPropertiesMap.get(
+            vueNode
+          )).find((cp) => {
+            return (
+              cp.value &&
+              node.loc.start.line >= cp.value.loc.start.line &&
+              node.loc.end.line <= cp.value.loc.end.line &&
+              targetBody === cp.value
+            )
           })
+          if (computedProperty) {
+            if (!utils.isThis(node, context)) {
+              return
+            }
+            const mem = node.parent
+            if (mem.object !== node) {
+              return
+            }
+
+            const invalid = utils.findMutating(mem)
+            if (invalid) {
+              context.report({
+                node: invalid.node,
+                message:
+                  'Unexpected side effect in "{{key}}" computed property.',
+                data: { key: computedProperty.key || 'Unknown' }
+              })
+            }
+            return
+          }
+
+          // ignore `this` for computed functions
+          if (node.type === 'ThisExpression') {
+            return
+          }
+
+          const computedFunction = computedCallNodes.find(
+            (c) =>
+              node.loc.start.line >= c.loc.start.line &&
+              node.loc.end.line <= c.loc.end.line &&
+              targetBody === c.body
+          )
+          if (!computedFunction) {
+            return
+          }
+
+          const mem = node.parent
+          if (mem.object !== node) {
+            return
+          }
+
+          const variable = findVariable(context.getScope(), node)
+          if (!variable || variable.defs.length !== 1) {
+            return
+          }
+
+          const def = variable.defs[0]
+          if (
+            def.type === 'ImplicitGlobalVariable' ||
+            def.type === 'TDZ' ||
+            def.type === 'ImportBinding'
+          ) {
+            return
+          }
+
+          const isDeclaredInsideSetup = setupFunctions.some(
+            (setupFn) =>
+              def.node.loc.start.line >= setupFn.loc.start.line &&
+              def.node.loc.end.line <= setupFn.loc.end.line
+          )
+          if (!isDeclaredInsideSetup) {
+            return
+          }
+
+          if (
+            def.node.loc.start.line >= computedFunction.loc.start.line &&
+            def.node.loc.end.line <= computedFunction.loc.end.line
+          ) {
+            // mutating local variables are accepted
+            return
+          }
+
+          const invalid = utils.findMutating(node)
+          if (invalid) {
+            context.report({
+              node: invalid.node,
+              message: 'Unexpected side effect in computed function.'
+            })
+          }
         }
-      }
-    })
+      })
+    )
   }
 }