Repository: ember-cli/eslint-plugin-ember
Pull request #856 (2020-06-16 18:57:41)

Handle nested paths with ES5 setters in `no-side-effects` rule
CC: @mongoose700 


Patch:
diff --git a/lib/rules/no-side-effects.js b/lib/rules/no-side-effects.js
index 51b98aa2..4eb83151 100644
--- a/lib/rules/no-side-effects.js
+++ b/lib/rules/no-side-effects.js
@@ -3,6 +3,7 @@
 const types = require('../utils/types');
 const ember = require('../utils/ember');
 const computedPropertyUtils = require('../utils/computed-properties');
+const propertySetterUtils = require('../utils/property-setter');
 const emberUtils = require('../utils/ember');
 const Traverser = require('../utils/traverser');
 
@@ -27,15 +28,6 @@ function isEmberSet(node, emberImportAliasName) {
   );
 }
 
-function isThisSet(node) {
-  return (
-    types.isAssignmentExpression(node) &&
-    types.isMemberExpression(node.left) &&
-    types.isThisExpression(node.left.object) &&
-    types.isIdentifier(node.left.property)
-  );
-}
-
 /**
  * Recursively finds calls that could be side effects in a computed property function body.
  *
@@ -48,7 +40,7 @@ function findSideEffects(computedPropertyBody, emberImportAliasName) {
 
   new Traverser().traverse(computedPropertyBody, {
     enter(child) {
-      if (isEmberSet(child, emberImportAliasName) || isThisSet(child)) {
+      if (isEmberSet(child, emberImportAliasName) || propertySetterUtils.isThisSet(child)) {
         results.push(child);
       }
     },
diff --git a/lib/utils/property-setter.js b/lib/utils/property-setter.js
new file mode 100644
index 00000000..7c56c63c
--- /dev/null
+++ b/lib/utils/property-setter.js
@@ -0,0 +1,30 @@
+const types = require('./types');
+
+module.exports = {
+  isThisSet,
+};
+
+/**
+ * Checks if an AssignmentExpression looks like `this.x =` or `this.x.y =`.
+ *
+ * @param {Node} node The AssignmentExpression node to check.
+ * @returns {boolean} Whether the node looks as expected.
+ */
+function isThisSet(node) {
+  if (!types.isAssignmentExpression(node)) {
+    return false;
+  }
+
+  let current = node.left;
+  if (!types.isMemberExpression(current)) {
+    return false;
+  }
+  while (types.isMemberExpression(current.object)) {
+    current = current.object;
+  }
+  if (!types.isThisExpression(current.object)) {
+    return false;
+  }
+
+  return true;
+}


Test patch:
diff --git a/tests/lib/rules/no-side-effects.js b/tests/lib/rules/no-side-effects.js
index 869c3843..8c88ba7b 100644
--- a/tests/lib/rules/no-side-effects.js
+++ b/tests/lib/rules/no-side-effects.js
@@ -68,6 +68,7 @@ eslintTester.run('no-side-effects', rule, {
     "this.set('x', 123);",
     'this.setProperties({ x: 123 });',
     'this.x = 123;',
+    'this.x.y = 123;',
   ],
   invalid: [
     {
@@ -297,6 +298,7 @@ eslintTester.run('no-side-effects', rule, {
     },
 
     {
+      // ES5 setter:
       code: 'computed("test", function() { this.x = 123; })',
       output: null,
       errors: [
@@ -306,5 +308,17 @@ eslintTester.run('no-side-effects', rule, {
         },
       ],
     },
+
+    {
+      // ES5 setter with nested path:
+      code: 'computed("test", function() { this.x.y = 123; })',
+      output: null,
+      errors: [
+        {
+          message: ERROR_MESSAGE,
+          type: 'AssignmentExpression',
+        },
+      ],
+    },
   ],
 });
diff --git a/tests/lib/utils/property-setter-test.js b/tests/lib/utils/property-setter-test.js
new file mode 100644
index 00000000..5a254e6d
--- /dev/null
+++ b/tests/lib/utils/property-setter-test.js
@@ -0,0 +1,22 @@
+'use strict';
+
+const babelEslint = require('babel-eslint');
+const propertySetterUtils = require('../../../lib/utils/property-setter');
+
+function parse(code) {
+  return babelEslint.parse(code).body[0];
+}
+
+describe('isThisSet', () => {
+  it('behaves correctly', () => {
+    // False:
+    expect(propertySetterUtils.isThisSet(parse('this.x').expression)).toBeFalsy();
+    expect(propertySetterUtils.isThisSet(parse('this.x()').expression)).toBeFalsy();
+    expect(propertySetterUtils.isThisSet(parse('let x = 123;'))).toBeFalsy();
+    expect(propertySetterUtils.isThisSet(parse('x = 123;'))).toBeFalsy();
+
+    // True:
+    expect(propertySetterUtils.isThisSet(parse('this.x = 123').expression)).toBeTruthy();
+    expect(propertySetterUtils.isThisSet(parse('this.x.y = 123').expression)).toBeTruthy();
+  });
+});