Repository: ember-cli/eslint-plugin-ember
Pull request #879 (2020-07-02 23:22:26)

Autofix nested paths in the left side of an assignment without using optional chaining in the `no-get` rule
CC: @mongoose700 


Patch:
diff --git a/lib/rules/no-get.js b/lib/rules/no-get.js
index 711caff1..dab651f3 100644
--- a/lib/rules/no-get.js
+++ b/lib/rules/no-get.js
@@ -2,14 +2,20 @@
 
 const types = require('../utils/types');
 const emberUtils = require('../utils/ember');
+const utils = require('../utils/utils');
 const assert = require('assert');
 
-function makeErrorMessageForGet(property, { isImportedGet, useOptionalChaining } = {}) {
+function makeErrorMessageForGet(
+  property,
+  { isImportedGet, useOptionalChaining, isInLeftSideOfAssignmentExpression } = {}
+) {
   const original = isImportedGet ? `get(this, '${property}')` : `this.get('${property}')`;
 
   const replacements = [
     `this.${property}`,
-    useOptionalChaining ? `this.${property.replace(/\./g, '?.')}` : undefined,
+    useOptionalChaining && !isInLeftSideOfAssignmentExpression
+      ? `this.${property.replace(/\./g, '?.')}`
+      : undefined,
   ]
     .filter(Boolean)
     .join('` or `');
@@ -28,8 +34,23 @@ function isValidJSPath(str) {
   return str.split('.').every(isValidJSVariableName);
 }
 
-function fix(node, fixer, path, useOptionalChaining) {
-  if (path.includes('.') && !useOptionalChaining) {
+function reportGet({ node, context, path, isImportedGet, useOptionalChaining }) {
+  const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
+  context.report({
+    node,
+    message: makeErrorMessageForGet(path, {
+      isImportedGet,
+      useOptionalChaining,
+      isInLeftSideOfAssignmentExpression,
+    }),
+    fix(fixer) {
+      return fixGet({ node, fixer, path, useOptionalChaining, isInLeftSideOfAssignmentExpression });
+    },
+  });
+}
+
+function fixGet({ node, fixer, path, useOptionalChaining, isInLeftSideOfAssignmentExpression }) {
+  if (path.includes('.') && !useOptionalChaining && !isInLeftSideOfAssignmentExpression) {
     // Not safe to autofix nested properties because some properties in the path might be null or undefined.
     return null;
   }
@@ -39,8 +60,10 @@ function fix(node, fixer, path, useOptionalChaining) {
     return null;
   }
 
-  // If we get to this point and there are periods present, convert them to the optional chaining operator.
-  return fixer.replaceText(node, `this.${path.replace(/\./g, '?.')}`);
+  // In the left side of an assignment, we can safely autofix nested paths without using optional chaining.
+  const replacementPath = isInLeftSideOfAssignmentExpression ? path : path.replace(/\./g, '?.');
+
+  return fixer.replaceText(node, `this.${replacementPath}`);
 }
 
 module.exports = {
@@ -159,13 +182,12 @@ module.exports = {
           (!node.arguments[0].value.includes('.') || !ignoreNestedPaths)
         ) {
           // Example: this.get('foo');
-          const path = node.arguments[0].value;
-          context.report({
+          reportGet({
             node,
-            message: makeErrorMessageForGet(path, { isImportedGet: false, useOptionalChaining }),
-            fix(fixer) {
-              return fix(node, fixer, path, useOptionalChaining);
-            },
+            context,
+            path: node.arguments[0].value,
+            isImportedGet: false,
+            useOptionalChaining,
           });
         }
 
@@ -178,13 +200,12 @@ module.exports = {
           (!node.arguments[1].value.includes('.') || !ignoreNestedPaths)
         ) {
           // Example: get(this, 'foo');
-          const path = node.arguments[1].value;
-          context.report({
+          reportGet({
             node,
-            message: makeErrorMessageForGet(path, { isImportedGet: true, useOptionalChaining }),
-            fix(fixer) {
-              return fix(node, fixer, path, useOptionalChaining);
-            },
+            context,
+            path: node.arguments[1].value,
+            isImportedGet: true,
+            useOptionalChaining,
           });
         }
 
diff --git a/lib/rules/require-computed-property-dependencies.js b/lib/rules/require-computed-property-dependencies.js
index e5385ed5..33164e1e 100644
--- a/lib/rules/require-computed-property-dependencies.js
+++ b/lib/rules/require-computed-property-dependencies.js
@@ -166,31 +166,6 @@ function findInjectedServiceNames(node) {
   return results;
 }
 
-/**
- * Check whether a node is inside the left side of an AssignmentExpression.
- *
- * Example:
- * - this.x.y = 123;
- * Nodes in the left side of this example:
- * - this
- * - x
- * - y
- *
- * @param {Node} node - node to check
- * @returns {boolean} - whether the node is inside the left side of the given AssignmentExpression
- */
-function isInLeftSideOfAssignmentExpression(node) {
-  return Boolean(
-    utils.getAncestor(
-      node,
-      (ancestor) =>
-        ancestor.parent &&
-        types.isAssignmentExpression(ancestor.parent) &&
-        ancestor === ancestor.parent.left
-    )
-  );
-}
-
 /**
  * Recursively finds all `this.property` usages.
  *
@@ -208,7 +183,7 @@ function findThisGetCalls(node) {
           (types.isCallExpression(parent) || types.isOptionalCallExpression(parent)) &&
           parent.callee === child
         ) &&
-        !isInLeftSideOfAssignmentExpression(child) && // Ignore the left side (x) of an assignment: this.x = 123;
+        !utils.isInLeftSideOfAssignmentExpression(child) && // Ignore the left side (x) of an assignment: this.x = 123;
         propertyGetterUtils.isSimpleThisExpression(child)
       ) {
         results.push(child);
diff --git a/lib/utils/utils.js b/lib/utils/utils.js
index c4c263b4..87f33990 100644
--- a/lib/utils/utils.js
+++ b/lib/utils/utils.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const {
+  isAssignmentExpression,
   isCallExpression,
   isIdentifier,
   isLiteral,
@@ -18,6 +19,7 @@ module.exports = {
   getSize,
   isEmptyMethod,
   isGlobalCallExpression,
+  isInLeftSideOfAssignmentExpression,
   parseArgs,
   parseCallee,
 };
@@ -219,3 +221,28 @@ function getAncestor(node, predicate) {
 
   return null;
 }
+
+/**
+ * Check whether a node is inside the left side of an AssignmentExpression.
+ *
+ * Example:
+ * - this.x.y = 123;
+ * Nodes in the left side of this example:
+ * - this
+ * - x
+ * - y
+ *
+ * @param {Node} node - node to check
+ * @returns {boolean} - whether the node is inside the left side of the given AssignmentExpression
+ */
+function isInLeftSideOfAssignmentExpression(node) {
+  return Boolean(
+    getAncestor(
+      node,
+      (ancestor) =>
+        ancestor.parent &&
+        isAssignmentExpression(ancestor.parent) &&
+        ancestor === ancestor.parent.left
+    )
+  );
+}


Test patch:
diff --git a/tests/lib/rules/no-get.js b/tests/lib/rules/no-get.js
index 7b379aca..2c382ffe 100644
--- a/tests/lib/rules/no-get.js
+++ b/tests/lib/rules/no-get.js
@@ -337,6 +337,38 @@ ruleTester.run('no-get', rule, {
         },
       ],
     },
+    {
+      // Optional chaining is not valid in the left side of an assignment,
+      // and we can safely autofix nested paths without it anyway.
+      code: "this.get('foo.bar')[123] = 'hello world';",
+      options: [{ useOptionalChaining: true }],
+      output: "this.foo.bar[123] = 'hello world';",
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo.bar', {
+            isImportedGet: false,
+            useOptionalChaining: false,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
+    {
+      // We can safely autofix nested paths in the left side of an assignment,
+      // even when the `useOptionalChaining` option is off.
+      code: "this.get('foo.bar')[123] = 'hello world';",
+      options: [{ useOptionalChaining: false }],
+      output: "this.foo.bar[123] = 'hello world';",
+      errors: [
+        {
+          message: makeErrorMessageForGet('foo.bar', {
+            isImportedGet: false,
+            useOptionalChaining: false,
+          }),
+          type: 'CallExpression',
+        },
+      ],
+    },
 
     {
       // Reports violation after (classic) proxy class.