Merge pull request #914 from bmish/no-side-effects-fix

Improve `set()` detection logic in `no-side-effects` rule to avoid false positives, catch missed cases, and check imports
ember-cli · Aug 14, 2020 · 384ccdd · 384ccdd
2 parents f6fa1aa + 9e618c6
commit 384ccdd
Show file tree

Hide file tree

Showing 2 changed files with 133 additions and 172 deletions.
diff --git a/lib/rules/no-side-effects.js b/lib/rules/no-side-effects.js
@@ -11,36 +11,77 @@ const { getImportIdentifier } = require('../utils/import');
 // General rule - Don't introduce side-effects in computed properties
 //------------------------------------------------------------------------------
 
-function isUnallowedMethod(name) {
-  return ['set', 'setProperties'].includes(name);
+// Ember.set(this, 'foo', 123)
+function isEmberSetThis(node, importedEmberName) {
+  return (
+    types.isCallExpression(node) &&
+    types.isMemberExpression(node.callee) &&
+    types.isIdentifier(node.callee.object) &&
+    node.callee.object.name === importedEmberName &&
+    types.isIdentifier(node.callee.property) &&
+    ['set', 'setProperties'].includes(node.callee.property.name) &&
+    node.arguments.length > 0 &&
+    memberExpressionBeginWithThis(node.arguments[0])
+  );
+}
+
+// set(this, 'foo', 123)
+function isImportedSetThis(node, importedSetName, importedSetPropertiesName) {
+  return (
+    types.isCallExpression(node) &&
+    types.isIdentifier(node.callee) &&
+    [importedSetName, importedSetPropertiesName].includes(node.callee.name) &&
+    node.arguments.length > 0 &&
+    memberExpressionBeginWithThis(node.arguments[0])
+  );
 }
 
-function isEmberSet(node, emberImportAliasName) {
-  const callee = node.callee;
+// this.set('foo', 123)
+// this.prop.set('foo', 123)
+function isThisSet(node) {
   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))
+    types.isCallExpression(node) &&
+    types.isMemberExpression(node.callee) &&
+    types.isIdentifier(node.callee.property) &&
+    ['set', 'setProperties'].includes(node.callee.property.name) &&
+    memberExpressionBeginWithThis(node.callee.object)
   );
 }
 
+function memberExpressionBeginWithThis(node) {
+  if (types.isThisExpression(node)) {
+    return true;
+  } else if (types.isMemberExpression(node)) {
+    return memberExpressionBeginWithThis(node.object);
+  }
+  return false;
+}
+
 /**
  * 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
+ * @param {string} importedEmberName
+ * @param {string} importedSetName
+ * @param {string} importedSetPropertiesName
  * @returns {Array<ASTNode>}
  */
-function findSideEffects(computedPropertyBody, emberImportAliasName) {
+function findSideEffects(
+  computedPropertyBody,
+  importedEmberName,
+  importedSetName,
+  importedSetPropertiesName
+) {
   const results = [];
 
   new Traverser().traverse(computedPropertyBody, {
     enter(child) {
-      if (isEmberSet(child, emberImportAliasName) || propertySetterUtils.isThisSet(child)) {
+      if (
+        isEmberSetThis(child, importedEmberName) || // Ember.set(this, 'foo', 123)
+        isImportedSetThis(child, importedSetName, importedSetPropertiesName) || // set(this, 'foo', 123)
+        isThisSet(child) || // this.set('foo', 123)
+        propertySetterUtils.isThisSet(child) // this.foo = 123;
+      ) {
         results.push(child);
       }
     },
@@ -70,6 +111,8 @@ module.exports = {
   create(context) {
     let importedEmberName;
     let importedComputedName;
+    let importedSetName;
+    let importedSetPropertiesName;
 
     const report = function (node) {
       context.report(node, ERROR_MESSAGE);
@@ -83,6 +126,10 @@ module.exports = {
         if (node.source.value === '@ember/object') {
           importedComputedName =
             importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
+          importedSetName = importedSetName || getImportIdentifier(node, '@ember/object', 'set');
+          importedSetPropertiesName =
+            importedSetPropertiesName ||
+            getImportIdentifier(node, '@ember/object', 'setProperties');
         }
       },
 
@@ -93,7 +140,12 @@ module.exports = {
 
         const computedPropertyBody = computedPropertyUtils.getComputedPropertyFunctionBody(node);
 
-        findSideEffects(computedPropertyBody, importedEmberName).forEach(report);
+        findSideEffects(
+          computedPropertyBody,
+          importedEmberName,
+          importedSetName,
+          importedSetPropertiesName
+        ).forEach(report);
       },
 
       Identifier(node) {
@@ -103,7 +155,12 @@ module.exports = {
 
         const computedPropertyBody = computedPropertyUtils.getComputedPropertyFunctionBody(node);
 
-        findSideEffects(computedPropertyBody, importedEmberName).forEach(report);
+        findSideEffects(
+          computedPropertyBody,
+          importedEmberName,
+          importedSetName,
+          importedSetPropertiesName
+        ).forEach(report);
       },
     };
   },