Add missing tests (#91)

* Add missing tests

This adds two failing tests to highlight bugs and a passing but
previously missing test.

The missing test added is for calling `set` which was destructured from
`import Ember from 'ember'`

One failing test is to handle the case where `Ember.computed` is called
from an import that isn't named `Ember`, eg
```
import E from 'ember';
E.computed()
```

Another failing test illustrates that ember import bindings are
clobbered if followed by any other default import, as in:
```
import Ember from 'ember';
import AnythingElse from 'anywhere';
```

* Clean up no-side-effect-cp rule.

Fixes a number of the errors previously identified, as well as a few
additional ones (tests added in this commit).

Author: hjdivad

lib/rules/no-side-effect-cp.js

@@ -2,12 +2,28 @@
  * @fileOverview Disallow use of computed properties that include side-effect producing calls.
  */
 
+const { get } = require('../utils/get');
 const { getCaller, cleanCaller } = require('../utils/caller');
 const isCPGetter = require('../utils/computed-property');
 const { getEmberImportBinding, collectImportBindings } = require('../utils/imports');
 const { collectObjectPatternBindings } = require('../utils/destructed-binding');
 
-let SIDE_EFFECTS = ['this.send', 'this.sendAction', 'this.sendEvent', 'Em.sendEvent', 'Ember.sendEvent', 'this.trigger', 'this.set', 'Ember.set', 'Em.set', 'this.setProperties', 'Ember.setProperties', 'Em.setProperties'];
+function sideEffectsForEmber(importName) {
+  return [
+    `${importName}.sendEvent`,
+    `${importName}.set`,
+    `${importName}.setProperties`
+  ];
+}
+
+const SIDE_EFFECTS = Object.freeze([].concat(
+  'this.send',
+  'this.sendAction',
+  'this.trigger',
+  sideEffectsForEmber('this'),
+  sideEffectsForEmber('Em'),
+  sideEffectsForEmber('Ember')
+));
 
 const MESSAGE = 'Do not send events or actions in Computed Properties. This will cause data flow issues in the application, where the accessing of a property causes some side-effect. You should only send actions on behalf of user initiated events. Please see the following guide for more information: https://github.com/ember-best-practices/eslint-plugin-ember-best-practices/blob/master/guides/rules/no-side-effect-cp.md';
 
@@ -21,45 +37,54 @@ module.exports = {
     message: MESSAGE
   },
   create(context) {
-    let inCPGettter = false;
-    let bindings = [];
-    let emberImportBinding;
+    let inCPGetter = false;
+    let importedSideEffectBindings = [];
+    let emberImportBinding, importedCPBinding;
+    let sideEffects = SIDE_EFFECTS.slice();
 
     return {
       ImportDeclaration(node) {
-        bindings = collectImportBindings(node, {
+        importedSideEffectBindings = importedSideEffectBindings.concat(collectImportBindings(node, {
           '@ember/object': ['set', 'setProperties'],
           '@ember/object/events': ['sendEvent']
-        });
+        }));
       },
 
       ObjectPattern(node) {
         if (emberImportBinding) {
-          SIDE_EFFECTS = SIDE_EFFECTS.concat(collectObjectPatternBindings(node, {
+          sideEffects = sideEffects.concat(collectObjectPatternBindings(node, {
             [emberImportBinding]: ['set', 'setProperties', 'sendEvent']
           }));
         }
       },
 
       ImportDefaultSpecifier(node) {
-        emberImportBinding = getEmberImportBinding(node);
+        let localEmberImportBinding = getEmberImportBinding(node);
+        if (localEmberImportBinding) {
+          emberImportBinding = localEmberImportBinding;
+          sideEffects = sideEffects.concat(sideEffectsForEmber(emberImportBinding));
+        }
+
+        if (get(node, 'parent.source.value') === '@ember/computed') {
+          importedCPBinding = node.local.name;
+        }
       },
 
       FunctionExpression(node) {
-        if (isCPGetter(node)) {
-          inCPGettter = true;
+        if (isCPGetter(node, emberImportBinding, importedCPBinding)) {
+          inCPGetter = node;
         }
       },
       'FunctionExpression:exit'(node) {
-        if (isCPGetter(node)) {
-          inCPGettter = false;
+        if (node === inCPGetter) {
+          inCPGetter = null;
         }
       },
 
       CallExpression(node) {
-        if (inCPGettter) {
+        if (inCPGetter) {
           let caller = cleanCaller(getCaller(node));
-          let hasSideEffect = SIDE_EFFECTS.includes(caller) || bindings.includes(caller);
+          let hasSideEffect = sideEffects.includes(caller) || importedSideEffectBindings.includes(caller);
           if (hasSideEffect) {
             context.report(node, MESSAGE);
           }

lib/utils/computed-property.js

@@ -1,12 +1,12 @@
 const { get } = require('./get');
 const { getCaller } = require('./caller');
 
-function isCPDesc(node, method) {
+function isCPDesc(node, importedEmberName, importedCPName) {
   if (node.type !== 'FunctionExpression') {
     return false;
   }
 
-  if (get(node, 'parent.key.name') !== method) {
+  if (get(node, 'parent.key.name') !== 'get') {
     return false;
   }
 
@@ -22,12 +22,12 @@ function isCPDesc(node, method) {
     return false;
   }
   let callee = greatGrandParent.callee;
-  if (greatGrandParent.type === 'Identifier' && callee.name === 'computed') {
+  if (greatGrandParent.type === 'Identifier' && (callee.name === 'computed' || callee.name === importedCPName)) {
     return true;
   }
   if (callee.type === 'MemberExpression') {
     let caller = getCaller(callee);
-    return caller === 'Ember.computed' || caller === 'Em.computed';
+    return caller === 'Ember.computed' || caller === 'Em.computed' || caller === `${importedEmberName}.computed`;
   }
   return false; // don't know how you could get here
 }
@@ -40,7 +40,7 @@ function isPrototypeExtCP(node) {
   return get(node, 'parent.property.name');
 }
 
-function isCPAccessor(node) {
+function isCPAccessor(node, importedEmberName, importedCPName) {
   if (node.type !== 'FunctionExpression') {
     return false;
   }
@@ -51,18 +51,20 @@ function isCPAccessor(node) {
   }
 
   let callee = parent.callee;
-  if (callee.type === 'Identifier' && callee.name === 'computed') {
+  if (callee.type === 'Identifier' && (callee.name === 'computed' || callee.name === importedCPName)) {
     return true;
   }
 
   if (callee.type === 'MemberExpression') {
     let caller = getCaller(callee);
-    return caller === 'Ember.computed' || caller === 'Em.computed';
+    return caller === 'Ember.computed' || caller === 'Em.computed' || caller === `${importedEmberName}.computed`;
   }
   return false;
 }
 
-module.exports = function isCPGetter(node) {
-  return isCPDesc(node, 'get') || isCPAccessor(node) || isPrototypeExtCP(node);
+module.exports = function isCPGetter(node, importedEmberName, importedCPName) {
+  return isCPDesc(node, importedEmberName, importedCPName)
+    || isCPAccessor(node, importedEmberName, importedCPName)
+    || isPrototypeExtCP(node);
 };
 

tests/lib/rules/no-side-effect-cp.js

@@ -26,8 +26,22 @@ ruleTester.run('no-side-efffect-cp', rule, {
       code: `
         import EmberObject from '@ember/object';
         export default EmberObject();`
+    },
+    {
+      code: `
+        import lodash from 'lodash';
+        import Ember from 'ember';
+        const { set } = lodash;
+        export default Ember.Component({
+          foo: 'bar',
+          baz: false,
+          bar: Ember.computed('foo', function() {
+            set('baz', 'wat');
+          })
+        });`
     }
   ],
+
   invalid: [
     {
       code: `
@@ -228,6 +242,55 @@ ruleTester.run('no-side-efffect-cp', rule, {
       errors: [{
         message: MESSAGE
       }]
+    },
+    // This test must come before any others that `import Ember from 'ember'`
+    // to illustrate the problem of the import binding being saved in module
+    // scope.
+    // see #91
+    {
+      code: `
+        import Ember from 'ember';
+        import SomethingElse from 'something-else';
+        const { set } = Ember;
+          export default Ember.Component({
+            foo: 'bar',
+            baz: false,
+            bar: Ember.computed('foo', function() {
+              set('baz', 'wat');
+            })
+          });`,
+      errors: [{
+        message: MESSAGE
+      }]
+    },
+    {
+      code: `
+        import Ember from 'ember';
+        const { set } = Ember;
+          export default Ember.Component({
+            foo: 'bar',
+            baz: false,
+            bar: Ember.computed('foo', function() {
+              set('baz', 'wat');
+            })
+          });`,
+      errors: [{
+        message: MESSAGE
+      }]
+    },
+    {
+      code: `
+        import E from 'ember';
+          export default E.Component({
+            foo: 'bar',
+            baz: false,
+            bar: E.computed('foo', function() {
+              E.set('baz', 'wat');
+            })
+          });`,
+      errors: [{
+        message: MESSAGE
+      }]
     }
   ]
 });