Merge pull request #912 from bmish/no-get-imported-fn-this

Add `catchSafeObjects` option (default false) to `no-get` rule to catch usages of the imported get/getProperties functions on non-this objects

docs/rules/no-get.md

@@ -94,6 +94,7 @@ This rule takes an optional object containing:
 * `boolean` -- `ignoreGetProperties` -- whether the rule should ignore `getProperties` (default `false`)
 * `boolean` -- `ignoreNestedPaths` -- whether the rule should ignore `this.get('some.nested.property')` (default `false`)
 * `boolean` -- `useOptionalChaining` -- whether the rule should use the [optional chaining operator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) `?.` to autofix nested paths such as `this.get('some.nested.property')` to `this.some?.nested?.property` (when this option is off, these nested paths won't be autofixed at all) (default `false`)
+* `boolean` -- `catchSafeObjects` -- whether the rule should catch `get(foo, 'bar')` (default `false`, TODO: enable in next major release)
 
 ## Related Rules
 

lib/rules/no-get.js

@@ -23,18 +23,32 @@ function isValidJSPath(str) {
   return str.split('.').every((part) => isValidJSVariableName(part) || isValidJSArrayIndex(part));
 }
 
-function reportGet({ node, context, path, useOptionalChaining }) {
+function reportGet({ node, context, path, useOptionalChaining, objectText }) {
   const isInLeftSideOfAssignmentExpression = utils.isInLeftSideOfAssignmentExpression(node);
   context.report({
     node,
     message: ERROR_MESSAGE_GET,
     fix(fixer) {
-      return fixGet({ node, fixer, path, useOptionalChaining, isInLeftSideOfAssignmentExpression });
+      return fixGet({
+        node,
+        fixer,
+        path,
+        useOptionalChaining,
+        isInLeftSideOfAssignmentExpression,
+        objectText,
+      });
     },
   });
 }
 
-function fixGet({ node, fixer, path, useOptionalChaining, isInLeftSideOfAssignmentExpression }) {
+function fixGet({
+  node,
+  fixer,
+  path,
+  useOptionalChaining,
+  isInLeftSideOfAssignmentExpression,
+  objectText,
+}) {
   if (path.includes('.') && !useOptionalChaining && !isInLeftSideOfAssignmentExpression) {
     // Not safe to autofix nested properties because some properties in the path might be null or undefined.
     return null;
@@ -54,7 +68,10 @@ function fixGet({ node, fixer, path, useOptionalChaining, isInLeftSideOfAssignme
     isInLeftSideOfAssignmentExpression ? '[$1]' : '.[$1]'
   );
 
-  return fixer.replaceText(node, `this.${replacementPath}`);
+  // Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
+  const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;
+
+  return fixer.replaceText(node, `${objectTextSafe}.${replacementPath}`);
 }
 
 module.exports = {
@@ -85,6 +102,10 @@ module.exports = {
             type: 'boolean',
             default: false,
           },
+          catchSafeObjects: {
+            type: 'boolean',
+            default: false,
+          },
         },
         additionalProperties: false,
       },
@@ -95,6 +116,7 @@ module.exports = {
     const ignoreGetProperties = context.options[0] && context.options[0].ignoreGetProperties;
     const ignoreNestedPaths = context.options[0] && context.options[0].ignoreNestedPaths;
     const useOptionalChaining = context.options[0] && context.options[0].useOptionalChaining;
+    const catchSafeObjects = context.options[0] && context.options[0].catchSafeObjects;
 
     if (ignoreNestedPaths && useOptionalChaining) {
       assert(
@@ -191,24 +213,27 @@ module.exports = {
             path: node.arguments[0].value,
             isImportedGet: false,
             useOptionalChaining,
+            objectText: 'this',
           });
         }
 
         if (
           types.isIdentifier(node.callee) &&
           node.callee.name === importedGetName &&
           node.arguments.length === 2 &&
-          types.isThisExpression(node.arguments[0]) &&
+          (types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
           types.isStringLiteral(node.arguments[1]) &&
           (!node.arguments[1].value.includes('.') || !ignoreNestedPaths)
         ) {
           // Example: get(this, 'foo');
+          const sourceCode = context.getSourceCode();
           reportGet({
             node,
             context,
             path: node.arguments[1].value,
             isImportedGet: true,
             useOptionalChaining,
+            objectText: sourceCode.getText(node.arguments[0]),
           });
         }
 
@@ -234,7 +259,7 @@ module.exports = {
         if (
           types.isIdentifier(node.callee) &&
           node.callee.name === importedGetPropertiesName &&
-          types.isThisExpression(node.arguments[0]) &&
+          (types.isThisExpression(node.arguments[0]) || catchSafeObjects) &&
           validateGetPropertiesArguments(node.arguments.slice(1), ignoreNestedPaths)
         ) {
           // Example: getProperties(this, 'foo', 'bar');

tests/lib/rules/no-get.js

@@ -89,6 +89,7 @@ ruleTester.run('no-get', rule, {
 
     // Not `this`.
     "myObject.getProperties('prop1', 'prop2');",
+    "import { getProperties } from '@ember/object'; getProperties(myObject, 'prop1', 'prop2');",
 
     // Not `getProperties`.
     "this.foo('prop1', 'prop2');",
@@ -204,6 +205,20 @@ ruleTester.run('no-get', rule, {
       `,
       errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
     },
+    {
+      // Calling the imported function on an unknown object (without `this`).
+      code: "import { get } from '@ember/object'; get(foo1.foo2, 'bar');",
+      options: [{ catchSafeObjects: true }],
+      output: "import { get } from '@ember/object'; foo1.foo2.bar;",
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
+    {
+      // Calling the imported function on an unknown object (without `this`) with an object argument that needs parenthesis.
+      code: "import { get } from '@ember/object'; get(foo || {}, 'bar');",
+      options: [{ catchSafeObjects: true }],
+      output: "import { get } from '@ember/object'; (foo || {}).bar;",
+      errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
+    },
     {
       // With renamed import:
       code: "import { get as g } from '@ember/object'; g(this, 'foo');",
@@ -272,6 +287,13 @@ ruleTester.run('no-get', rule, {
       output: null,
       errors: [{ message: ERROR_MESSAGE_GET_PROPERTIES, type: 'CallExpression' }],
     },
+    {
+      // Calling the imported function on an unknown object (without `this`).
+      code: "import { getProperties } from '@ember/object'; getProperties(foo, 'prop1', 'prop2');",
+      options: [{ catchSafeObjects: true }],
+      output: null,
+      errors: [{ message: ERROR_MESSAGE_GET_PROPERTIES, type: 'CallExpression' }],
+    },
     {
       // With renamed import:
       code: "import { getProperties as gp } from '@ember/object'; gp(this, 'prop1', 'prop2');",