Repository: ember-cli/eslint-plugin-ember (js)

Issue:
Enhancement - `no-get` rule and auto-fix
As is mentioned [here](https://github.com/ArtixZ/eslint-plugin-ember/blob/e497078f33f1423e448ede8cfc90b18d7f8f0723/tests/lib/rules/no-get.js#L56) in the test file, we currently don't identify or auto-fix `get` on a non-string. This means when the non-string is a variable name, we deem it as valid.

This ticket propose that we identify and auto-fix for those use cases. 

Expectations:

`obj.get(5)` ===> `obj[5]`   // `obj.get(5)` is legal as I tested.

`obj.get(foo)` ===> `obj[foo]`







Pull request description:
Support autofix of numerical property access and ternary expressions in `no-get` rule
This PR is a partial response to this [issue](https://github.com/ember-cli/eslint-plugin-ember/issues/1840#issue-1677525527) (partially fixes #1840) regarding enhancements for the `no-get` rule, along with another enhancement I propose.

- Add support for numerical property access `this.get(5)` ===> `this[5]` and `get(obj, 5)` ===> `obj[5]`
- Add test cases that prove `foo1 === foo2 ? obj.get('bar') : obj.get('baz')` ===> `foo1 === foo2 ? obj.bar : obj.baz` already works as is
- Add support for ternary expressions that have string/number literals as alternate and consequent (i.e. guaranteed to resolve to a literal) `this.get(foo1 === foo2 ? 'foo' : 'bar')` ===> `foo1 === foo2 ? this.foo : this.bar` and `get(foo, foo1 === foo2 ? 'bar' : 'baz')` ===> `foo1 === foo2 ? foo.bar : foo.baz`

I attempted to fix cases with variables or other dynamic expressions as the property argument `obj.get(foo)` ===> `obj[foo]`, but I now recommend that these cases **_not_** be autofixed. The reason for this is `foo` or any other dynamic expression could resolve to a string representation of chained property access (ex. `'bar.baz'`), in which case `obj[foo] !== obj.get(foo)` since `obj.get(foo)` would resolve at runtime as `obj.bar?.baz` while `obj[foo]` would resolve as `obj['bar.baz']`. Hence, autofixing these cases could cause bugs in existing code.

Patch:
diff --git a/lib/rules/no-get.js b/lib/rules/no-get.js
index b01988d1..43c513f1 100644
--- a/lib/rules/no-get.js
+++ b/lib/rules/no-get.js
@@ -23,7 +23,7 @@ function isValidJSPath(str) {
   return str.split('.').every((part) => isValidJSVariableName(part) || isValidJSArrayIndex(part));
 }
 
-function reportGet({ node, context, path, useOptionalChaining, objectText }) {
+function reportGet({ node, context, path, useOptionalChaining, objectText, sourceCode }) {
   const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
   context.report({
     node,
@@ -36,6 +36,7 @@ function reportGet({ node, context, path, useOptionalChaining, objectText }) {
         useOptionalChaining,
         isInLeftSideOfAssignmentExpression,
         objectText,
+        sourceCode,
       });
     },
   });
@@ -48,61 +49,60 @@ function fixGet({
   useOptionalChaining,
   isInLeftSideOfAssignmentExpression,
   objectText,
+  sourceCode,
 }) {
+  // Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
+  const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;
+
   const getResultIsChained = node.parent.type === 'MemberExpression' && node.parent.object === node;
 
   // If the result of get is chained, we can safely autofix nests paths without using optional chaining.
   // In the left side of an assignment, we can safely autofix nested paths without using optional chaining.
   const shouldIgnoreOptionalChaining = getResultIsChained || isInLeftSideOfAssignmentExpression;
 
-  if (path.includes('.') && !useOptionalChaining && !shouldIgnoreOptionalChaining) {
-    // Not safe to autofix nested properties because some properties in the path might be null or undefined.
-    return null;
-  }
-
-  if (!isValidJSPath(path)) {
-    // Do not autofix since the path would not be a valid JS path.
-    return null;
-  }
-
-  if (path.match(/lastObject/g)?.length > 1) {
-    // Do not autofix when multiple `lastObject` are chained.
-    return null;
-  }
+  if (types.isConditionalExpression(path)) {
+    const newConsequentExpression = convertLiteralTypePath({
+      path: path.consequent.value,
+      useOptionalChaining,
+      shouldIgnoreOptionalChaining,
+      objectText,
+    });
+    const newAlternateExpression = convertLiteralTypePath({
+      path: path.alternate.value,
+      useOptionalChaining,
+      shouldIgnoreOptionalChaining,
+      objectText,
+    });
+
+    // this means the overall expression can't be fixed
+    if (newConsequentExpression === null || newAlternateExpression === null) {
+      return null;
+    }
 
-  let replacementPath = shouldIgnoreOptionalChaining ? path : path.replace(/\./g, '?.');
+    let replacementText = `${sourceCode.getText(
+      path.test
+    )} ? ${objectTextSafe}${newConsequentExpression} : ${objectTextSafe}${newAlternateExpression}`;
 
-  // Replace any array element access (foo.1 => foo[1] or foo?.[1]).
-  replacementPath = replacementPath
-    .replace(/\.(\d+)/g, shouldIgnoreOptionalChaining ? '[$1]' : '.[$1]') // Usages in middle of path.
-    .replace(/^(\d+)\??\./, shouldIgnoreOptionalChaining ? '[$1].' : '[$1]?.') // Usage at beginning of path.
-    .replace(/^(\d+)$/, '[$1]'); // Usage as entire string.
+    if (shouldIgnoreOptionalChaining) {
+      replacementText = `(${replacementText})`;
+    }
 
-  // Replace any array element access using `firstObject` and `lastObject` (foo.firstObject => foo[0] or foo?.[0]).
-  replacementPath = replacementPath
-    .replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
-    .replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
-    .replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
-    .replace(
-      /\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
-      (_, offset) =>
-        `${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
-          0,
-          offset
-        )}.length - 1]`
-    )
-    .replace(
-      /^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
-      `[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
-    )
-    .replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
+    return fixer.replaceText(node, replacementText);
+  }
 
-  // Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
-  const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;
+  const replacementPath = convertLiteralTypePath({
+    path,
+    useOptionalChaining,
+    shouldIgnoreOptionalChaining,
+    objectText,
+  });
 
-  const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';
+  // null means it can't be fixed
+  if (replacementPath === null) {
+    return null;
+  }
 
-  return fixer.replaceText(node, `${objectTextSafe}${objectPathSeparator}${replacementPath}`);
+  return fixer.replaceText(node, `${objectTextSafe}${replacementPath}`);
 }
 
 function reportGetProperties({ context, node, objectText, properties }) {
@@ -319,6 +319,90 @@ module.exports = {
           });
         }
 
+        if (
+          types.isMemberExpression(node.callee) &&
+          (types.isThisExpression(node.callee.object) || catchUnsafeObjects) &&
+          types.isIdentifier(node.callee.property) &&
+          node.callee.property.name === 'get' &&
+          node.arguments.length === 1 &&
+          node.arguments[0].type === 'Literal' &&
+          typeof node.arguments[0].value === 'number'
+        ) {
+          // Example: this.get(5);
+          const sourceCode = context.getSourceCode();
+          reportGet({
+            node,
+            context,
+            path: node.arguments[0].value,
+            isImportedGet: false,
+            objectText: sourceCode.getText(node.callee.object),
+          });
+        }
+
+        if (
+          types.isIdentifier(node.callee) &&
+          node.callee.name === importedGetName &&
+          node.arguments.length === 2 &&
+          (types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
+          node.arguments[1].type === 'Literal' &&
+          typeof node.arguments[1].value === 'number'
+        ) {
+          // Example: get(this, 5);
+          const sourceCode = context.getSourceCode();
+          reportGet({
+            node,
+            context,
+            path: node.arguments[1].value,
+            isImportedGet: true,
+            objectText: sourceCode.getText(node.arguments[0]),
+          });
+        }
+
+        if (
+          types.isMemberExpression(node.callee) &&
+          (types.isThisExpression(node.callee.object) || catchUnsafeObjects) &&
+          types.isIdentifier(node.callee.property) &&
+          node.callee.property.name === 'get' &&
+          node.arguments.length === 1 &&
+          types.isConditionalExpression(node.arguments[0]) &&
+          types.isLiteral(node.arguments[0].consequent) &&
+          types.isLiteral(node.arguments[0].alternate)
+        ) {
+          // Example: this.get(foo ? 'bar' : 'baz');
+          const sourceCode = context.getSourceCode();
+          reportGet({
+            node,
+            context,
+            path: node.arguments[0],
+            isImportedGet: false,
+            objectText: sourceCode.getText(node.callee.object),
+            useOptionalChaining,
+            sourceCode,
+          });
+        }
+
+        if (
+          types.isIdentifier(node.callee) &&
+          node.callee.name === importedGetName &&
+          node.arguments.length === 2 &&
+          (types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
+          types.isConditionalExpression(node.arguments[1]) &&
+          types.isLiteral(node.arguments[1].consequent) &&
+          types.isLiteral(node.arguments[1].alternate)
+        ) {
+          // Example: get(foo, bar ? 'baz' : 'biz');
+          const sourceCode = context.getSourceCode();
+          reportGet({
+            node,
+            context,
+            path: node.arguments[1],
+            isImportedGet: true,
+            objectText: sourceCode.getText(node.arguments[0]),
+            useOptionalChaining,
+            sourceCode,
+          });
+        }
+
         // **************************
         // getProperties
         // **************************
@@ -371,3 +455,60 @@ function validateGetPropertiesArguments(args, ignoreNestedPaths) {
       types.isStringLiteral(argument) && (!argument.value.includes('.') || !ignoreNestedPaths)
   );
 }
+
+function convertLiteralTypePath({
+  path,
+  useOptionalChaining,
+  shouldIgnoreOptionalChaining,
+  objectText,
+}) {
+  if (typeof path === 'number') {
+    return `[${path}]`;
+  }
+
+  if (path.includes('.') && !useOptionalChaining && !shouldIgnoreOptionalChaining) {
+    // Not safe to autofix nested properties because some properties in the path might be null or undefined.
+    return null;
+  }
+
+  if (!isValidJSPath(path)) {
+    // Do not autofix since the path would not be a valid JS path.
+    return null;
+  }
+
+  if (path.match(/lastObject/g)?.length > 1) {
+    // Do not autofix when multiple `lastObject` are chained.
+    return null;
+  }
+
+  let replacementPath = shouldIgnoreOptionalChaining ? path : path.replace(/\./g, '?.');
+
+  // Replace any array element access (foo.1 => foo[1] or foo?.[1]).
+  replacementPath = replacementPath
+    .replace(/\.(\d+)/g, shouldIgnoreOptionalChaining ? '[$1]' : '.[$1]') // Usages in middle of path.
+    .replace(/^(\d+)\??\./, shouldIgnoreOptionalChaining ? '[$1].' : '[$1]?.') // Usage at beginning of path.
+    .replace(/^(\d+)$/, '[$1]'); // Usage as entire string.
+
+  // Replace any array element access using `firstObject` and `lastObject` (foo.firstObject => foo[0] or foo?.[0]).
+  replacementPath = replacementPath
+    .replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
+    .replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
+    .replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
+    .replace(
+      /\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
+      (_, offset) =>
+        `${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
+          0,
+          offset
+        )}.length - 1]`
+    )
+    .replace(
+      /^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
+      `[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
+    )
+    .replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.
+
+  const objectPathSeparator = replacementPath.startsWith('[') ? '' : '.';
+
+  return `${objectPathSeparator}${replacementPath}`;
+}


Test patch:
diff --git a/tests/lib/rules/no-get.js b/tests/lib/rules/no-get.js
index cdad804e..dba89e6a 100644
--- a/tests/lib/rules/no-get.js
+++ b/tests/lib/rules/no-get.js
@@ -53,10 +53,8 @@ ruleTester.run('no-get', rule, {
     "this.get('foo', 'bar');",
     "import { get } from '@ember/object'; get(this, 'foo', 'bar');",
 
-    // Non-string parameter.
-    'this.get(5);',
+    // Non-string, non-numerical parameter.
     'this.get(MY_PROP);',
-    "import { get } from '@ember/object'; get(this, 5);",
     "import { get } from '@ember/object'; get(this, MY_PROP);",
 
     // Unknown sub-function call:
@@ -72,6 +70,16 @@ ruleTester.run('no-get', rule, {
     // Missing import:
     "get(this, 'foo');",
 
+    // Ternary expressions with non-literal consequent or alternate
+    'this.get(foo ? bar : baz)',
+    `
+    import { get } from '@ember/object';
+    import { somethingElse } from '@ember/object';
+    import { random } from 'random';
+
+    const buzz = get(foo, bar ? baz : biz);
+    `,
+
     // **************************
     // getProperties
     // **************************
@@ -941,5 +949,176 @@ ruleTester.run('no-get', rule, {
       `,
       errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
     },
+    // Accessing numerical property with get
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+      get(foo, 5);
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+      foo[5];
+      `,
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: 'this.get(5);',
+      output: 'this[5];',
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    // Logical and conditional expressions
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const bar = baz ? get(foo, 'biz') : get(foo, 'buzz');
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const bar = baz ? foo.biz : foo.buzz;
+      `,
+      errors: [
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+      ],
+    },
+    {
+      code: "foo ? this.get('bar') : this.get('baz')",
+      output: 'foo ? this.bar : this.baz',
+      errors: [
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+      ],
+    },
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const bar = get(foo, 'biz') || get(foo, 'buzz');
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const bar = foo.biz || foo.buzz;
+      `,
+      errors: [
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+      ],
+    },
+    {
+      code: "this.get('bar') || this.get('baz')",
+      output: 'this.bar || this.baz',
+      errors: [
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+        { message: ERROR_MESSAGE_GET, type: 'CallExpression' },
+      ],
+    },
+    // Ternary expressions with literal consequent and alternate
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = get(foo, bar ? 'biz' : 'baz');
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = bar ? foo.biz : foo.baz;
+      `,
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: "this.get(foo ? 'bar' : 'baz')",
+      output: 'foo ? this.bar : this.baz',
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = get(foo, bar ? 5 : 'baz');
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = bar ? foo[5] : foo.baz;
+      `,
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: "this.get(foo ? 5 : 'baz')",
+      output: 'foo ? this[5] : this.baz',
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    // Ternary expressions with literal consequent and alternate with optional chaining
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = get(foo, bar ? 'biz.baz' : 'baz.biz');
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      const buzz = bar ? foo.biz?.baz : foo.baz?.biz;
+      `,
+      options: [{ useOptionalChaining: true }],
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: "this.get(foo ? 'bar.baz' : 'baz.bar')",
+      output: 'foo ? this.bar?.baz : this.baz?.bar',
+      options: [{ useOptionalChaining: true }],
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    // Ternary expressions with left-hand side of assignment expression
+    {
+      code: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      get(foo, bar ? 'biz' : 'baz').buzz = 'something';
+      `,
+      output: `
+      import { get } from '@ember/object';
+      import { somethingElse } from '@ember/object';
+      import { random } from 'random';
+
+      (bar ? foo.biz : foo.baz).buzz = 'something';
+      `,
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      code: "this.get(foo ? 'bar' : 'baz').buzz = 'something';",
+      output: "(foo ? this.bar : this.baz).buzz = 'something';",
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
   ],
 });