feat: add decorator support to no-side-effects rule

This is a major rewrite/refactor/simplification of the rule in order to support decorators and other future improvements.

Author: bmish

docs/rules/no-side-effects.md

@@ -2,9 +2,14 @@
 
 :white_check_mark: The `"extends": "plugin:ember/recommended"` property in a configuration file enables this rule.
 
-Don't introduce side-effects in computed properties.
+When using computed properties, do not introduce side effects. Side effects make it much more difficult to reason about the origin of changes.
 
-When using computed properties do not introduce side effects. It will make reasoning about the origin of the change much harder.
+This rule currently disallows the following side-effect-causing statements inside computed properties:
+
+* `this.set('x', 123);`
+* `this.setProperties({ x: 123 });`
+
+Note that other side effects like network requests should be avoided as well.
 
 ## Examples
 
@@ -35,9 +40,3 @@ export default Component.extend({
   })
 });
 ```
-
-## Help Wanted
-
-| Issue | Link |
-| :-- | :-- |
-| :x: Missing native JavaScript class support | [#560](https://github.com/ember-cli/eslint-plugin-ember/issues/560) |

lib/rules/no-side-effects.js

@@ -2,6 +2,9 @@
 
 const types = require('../utils/types');
 const ember = require('../utils/ember');
+const computedPropertyUtils = require('../utils/computed-properties');
+const emberUtils = require('../utils/ember');
+const Traverser = require('../utils/traverser');
 
 //------------------------------------------------------------------------------
 // General rule - Don't introduce side-effects in computed properties
@@ -11,6 +14,40 @@ function isUnallowedMethod(name) {
   return ['set', 'setProperties'].includes(name);
 }
 
+function isEmberSet(node, emberImportAliasName) {
+  const callee = node.callee;
+  return (
+    (types.isIdentifier(callee) && isUnallowedMethod(callee.name)) ||
+    (types.isMemberExpression(callee) &&
+      (types.isThisExpression(callee.object) ||
+        callee.object.name === 'Ember' ||
+        callee.object.name === emberImportAliasName) &&
+      types.isIdentifier(callee.property) &&
+      isUnallowedMethod(callee.property.name))
+  );
+}
+
+/**
+ * Recursively finds calls that could be side effects in a computed property function body.
+ *
+ * @param {ASTNode} computedPropertyBody body of computed property to search
+ * @param {string} emberImportAliasName
+ * @returns {Array<ASTNode>}
+ */
+function findSideEffects(computedPropertyBody, emberImportAliasName) {
+  const results = [];
+
+  new Traverser().traverse(computedPropertyBody, {
+    enter(child) {
+      if (isEmberSet(child, emberImportAliasName)) {
+        results.push(child);
+      }
+    },
+  });
+
+  return results;
+}
+
 const ERROR_MESSAGE = "Don't introduce side-effects in computed properties";
 
 module.exports = {
@@ -44,33 +81,13 @@ module.exports = {
       },
 
       CallExpression(node) {
-        const callee = node.callee;
-        const isEmberSet =
-          (types.isIdentifier(callee) && isUnallowedMethod(callee.name)) ||
-          (types.isMemberExpression(callee) &&
-            (types.isThisExpression(callee.object) ||
-              callee.object.name === 'Ember' ||
-              callee.object.name === emberImportAliasName) &&
-            types.isIdentifier(callee.property) &&
-            isUnallowedMethod(callee.property.name));
-
-        if (isEmberSet) {
-          const ancestors = context.getAncestors();
-          const computedIndex = ancestors.findIndex(ember.isComputedProp);
-          const setPropertyFunctionIndex = ancestors.findIndex(
-            (ancestor) =>
-              ancestor.type === 'Property' &&
-              ancestor.key.name === 'set' &&
-              types.isFunctionExpression(ancestor.value)
-          );
-
-          if (
-            computedIndex > -1 &&
-            (setPropertyFunctionIndex === -1 || setPropertyFunctionIndex < computedIndex)
-          ) {
-            report(node);
-          }
+        if (!emberUtils.isComputedProp(node)) {
+          return;
         }
+
+        const computedPropertyBody = computedPropertyUtils.getComputedPropertyFunctionBody(node);
+
+        findSideEffects(computedPropertyBody, emberImportAliasName).forEach(report);
       },
     };
   },

lib/rules/require-computed-property-dependencies.js

@@ -1,18 +1,6 @@
 'use strict';
 
-function getTraverser() {
-  let traverser;
-  try {
-    // eslint-disable-next-line node/no-unpublished-require
-    traverser = require('eslint/lib/shared/traverser'); // eslint >= 6
-  } catch (error) {
-    // eslint-disable-next-line node/no-unpublished-require, node/no-missing-require
-    traverser = require('eslint/lib/util/traverser'); // eslint < 6
-  }
-  return traverser;
-}
-
-const Traverser = getTraverser();
+const Traverser = require('../utils/traverser');
 const emberUtils = require('../utils/ember');
 const computedPropertyUtils = require('../utils/computed-properties');
 const types = require('../utils/types');

lib/utils/traverser.js

@@ -0,0 +1,15 @@
+'use strict';
+
+module.exports = getTraverser;
+
+function getTraverser() {
+  let traverser;
+  try {
+    // eslint-disable-next-line node/no-unpublished-require
+    traverser = require('eslint/lib/shared/traverser'); // eslint >= 6
+  } catch (error) {
+    // eslint-disable-next-line node/no-unpublished-require, node/no-missing-require
+    traverser = require('eslint/lib/util/traverser'); // eslint < 6
+  }
+  return traverser;
+}

tests/lib/rules/no-side-effects.js

@@ -25,6 +25,32 @@ eslintTester.run('no-side-effects', rule, {
     'import Ember from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { Foo.set(this, "testAmount", test.length); return ""; });',
 
     'import Ember from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { Foo.setProperties(); return ""; });',
+
+    // Decorators:
+    {
+      code: `
+        class Test {
+          @computed('first', 'last')
+          get fullName() { return this.first + ' ' + this.last; }
+        }
+      `,
+      parser: require.resolve('babel-eslint'),
+      parserOptions: {
+        ecmaVersion: 6,
+        sourceType: 'module',
+        ecmaFeatures: { legacyDecorators: true },
+      },
+    },
+
+    // No computed property function body;
+    'computed()',
+    'computed("test")',
+    'computed("test", function() {})',
+
+    // Not in a computed property:
+    "this.set('x', 123);",
+    'this.setProperties({ x: 123 });',
+    'this.x = 123;',
   ],
   invalid: [
     {
@@ -180,5 +206,55 @@ eslintTester.run('no-side-effects', rule, {
         },
       ],
     },
+
+    // Decorator with getter inside object parameter:
+    {
+      code: `
+        class Test {
+          @computed('key', {
+            get() {
+              this.set('x', 123);
+            },
+            set(key, value) {}
+          })
+          someProp
+        }
+      `,
+      output: null,
+      errors: [
+        {
+          message: ERROR_MESSAGE,
+          type: 'CallExpression',
+        },
+      ],
+      parser: require.resolve('babel-eslint'),
+      parserOptions: {
+        ecmaVersion: 6,
+        sourceType: 'module',
+        ecmaFeatures: { legacyDecorators: true },
+      },
+    },
+    // Decorator with getter function:
+    {
+      code: `
+        class Test {
+          @computed()
+          get someProp() { this.set('x', 123); }
+        }
+      `,
+      output: null,
+      errors: [
+        {
+          message: ERROR_MESSAGE,
+          type: 'CallExpression',
+        },
+      ],
+      parser: require.resolve('babel-eslint'),
+      parserOptions: {
+        ecmaVersion: 6,
+        sourceType: 'module',
+        ecmaFeatures: { legacyDecorators: true },
+      },
+    },
   ],
 });