Updating no-side-effects rule to better detect sets inside of blocks (#…

…260)

* Updating no-side-effects rule to better detect sets inside of block statements within computeds. Fixes #259.

* Fixing false positive errors when using set property in computed object.

* Adding extra test cases.

* Testing for Ember.set more accurately.

lib/rules/no-side-effects.js

@@ -1,6 +1,7 @@
 'use strict';
 
 const utils = require('../utils/utils');
+const ember = require('../utils/ember');
 
 //------------------------------------------------------------------------------
 // General rule - Don't introduce side-effects in computed properties
@@ -18,42 +19,48 @@ module.exports = {
   },
 
   create(context) {
+    let emberImportAliasName;
+
     const message = 'Don\'t introduce side-effects in computed properties';
 
     const report = function (node) {
       context.report(node, message);
     };
 
     return {
+      ImportDeclaration(node) {
+        if (!emberImportAliasName) {
+          emberImportAliasName = ember.getEmberImportAliasName(node);
+        }
+      },
+
       CallExpression(node) {
         const callee = node.callee;
-        const fnNodes = utils.findNodes(node.arguments, 'FunctionExpression');
-
-        if (callee && callee.name === 'computed' && fnNodes) {
-          fnNodes.forEach((fnNode) => {
-            const fnBody = fnNode.body ? fnNode.body.body : fnNode.body;
-            const fnExpressions = utils.findNodes(fnBody, 'ExpressionStatement');
-
-            fnExpressions.forEach((fnExpression) => {
-              const fnCallee = fnExpression.expression.callee;
-
-              if (
-                utils.isIdentifier(fnCallee) &&
-                fnCallee.name === 'set') {
-                report(fnExpression);
-                return;
-              }
-
-              if (
-                utils.isMemberExpression(fnCallee) &&
-                utils.isThisExpression(fnCallee.object) &&
-                utils.isIdentifier(fnCallee.property) &&
-                fnCallee.property.name === 'set'
-              ) {
-                report(fnExpression);
-              }
-            });
-          });
+        const isEmberSet = (
+          utils.isIdentifier(callee) &&
+          callee.name === 'set'
+        ) || (
+          utils.isMemberExpression(callee) &&
+          (utils.isThisExpression(callee.object) || callee.object.name === 'Ember' || callee.object.name === emberImportAliasName) &&
+          utils.isIdentifier(callee.property) &&
+          callee.property.name === 'set'
+        );
+
+        if (isEmberSet) {
+          const ancestors = context.getAncestors();
+          const computedIndex = ancestors.findIndex(ember.isComputedProp);
+          const setPropertyFunctionIndex = ancestors.findIndex(ancestor => (
+            ancestor.type === 'Property' &&
+            ancestor.key.name === 'set' &&
+            utils.isFunctionExpression(ancestor.value)
+          ));
+
+          if (
+            computedIndex > -1 &&
+            (setPropertyFunctionIndex === -1 || setPropertyFunctionIndex < computedIndex)
+          ) {
+            report(node);
+          }
         }
       },
     };

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

@@ -16,6 +16,18 @@ eslintTester.run('no-side-effects', rule, {
       code: 'testAmount: alias("test.length")',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
     },
+    {
+      code: 'testAmount: computed("test.length", { get() { return ""; }, set() { set(this, "testAmount", test.length); }})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+    },
+    {
+      code: 'let foo = computed("test", function() { someMap.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+    },
+    {
+      code: 'import Ember from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { Foo.set(this, "testAmount", test.length); return ""; });',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' }
+    }
   ],
   invalid: [
     {
@@ -31,5 +43,47 @@ eslintTester.run('no-side-effects', rule, {
         message: 'Don\'t introduce side-effects in computed properties',
       }],
     },
+    {
+      code: 'prop: computed("test", function() {if (get(this, "testAmount")) { set(this, "testAmount", test.length); } return "";})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'testAmount: computed("test.length", { get() { set(this, "testAmount", test.length); }, set() { set(this, "testAmount", test.length); }})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'testAmount: computed("test.length", function() { const setSomething = () => { set(this, "testAmount", test.length); }; setSomething(); })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'let foo = computed("test", function() { Ember.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'import Foo from "ember"; let foo = computed("test", function() { Foo.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'import EmberFoo from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { EmberFoo.set(this, "testAmount", test.length); return ""; });',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    }
   ],
 });