fix: avoid false positive involving this keyword in filterBy/mapBy value in require-computed-macros rule

Author: bmish

lib/rules/require-computed-macros.js

@@ -4,6 +4,7 @@ const types = require('../utils/types');
 const emberUtils = require('../utils/ember');
 const propertyGetterUtils = require('../utils/property-getter');
 const assert = require('assert');
+const scopeReferencesThis = require('../utils/scope-references-this');
 
 //------------------------------------------------------------------------------
 // Rule Definition
@@ -303,9 +304,15 @@ module.exports = {
           }
         } else if (propertyGetterUtils.isSimpleThisExpression(statement)) {
           reportSingleArg(nodeToReport, statement, 'reads');
-        } else if (isThisPropertyFunctionCall(statement, 'filterBy')) {
+        } else if (
+          isThisPropertyFunctionCall(statement, 'filterBy') &&
+          !statement.arguments.some(scopeReferencesThis)
+        ) {
           reportFunctionCall(nodeToReport, statement, 'filterBy');
-        } else if (isThisPropertyFunctionCall(statement, 'mapBy')) {
+        } else if (
+          isThisPropertyFunctionCall(statement, 'mapBy') &&
+          !statement.arguments.some(scopeReferencesThis)
+        ) {
           reportFunctionCall(nodeToReport, statement, 'mapBy');
         }
       },

lib/utils/scope-references-this.js

@@ -0,0 +1,37 @@
+'use strict';
+
+const Traverser = require('../utils/traverser');
+
+/**
+ * Determines whether this AST node uses the `this` of the surrounding context.
+ *
+ * @param {ASTNode} node
+ * @returns {boolean}
+ */
+function scopeReferencesThis(node) {
+  let result = false;
+
+  new Traverser().traverse(node, {
+    enter(node) {
+      switch (node.type) {
+        case 'FunctionDeclaration':
+        case 'FunctionExpression':
+          this.skip();
+          break;
+
+        case 'ThisExpression':
+          result = true;
+          this.break();
+          break;
+
+        default:
+          // Ignored.
+          break;
+      }
+    },
+  });
+
+  return result;
+}
+
+module.exports = scopeReferencesThis;

tests/lib/rules/require-computed-macros.js

@@ -68,6 +68,7 @@ ruleTester.run('require-computed-macros', rule, {
     // GT
     "gt('x', 123)",
     'computed(function() { return SOME_VAR > OTHER_VAR; })',
+    'computed(function() { return this.x > this.y; })',
 
     // GTE
     "gte('x', 123)",
@@ -91,14 +92,19 @@ ruleTester.run('require-computed-macros', rule, {
     'computed(function() { return SOME_VAR === "Hello"; })',
     'computed(function() { return this.prop === MY_VAR; })',
     "computed(function() { return this.get('prop') === MY_VAR; })",
+    'computed(function() { return this.prop === this.otherProp; })',
 
     // FILTERBY
     "filterBy('chores', 'done', true)",
+    'computed(function() { return this.chores.filterBy(this.otherProp, true); })', // Ignored because value depends on function's `this`.
+    "computed(function() { return this.chores.filterBy('done', this.otherProp); })", // Ignored because value depends on function's `this`.
 
     // MAPBY
     "mapBy('children', 'age')",
     "computed(function() { return this.children?.mapBy('age'); })", // Ignored because function might not exist.
     "computed(function() { return this.nested?.children.mapBy('age'); })", // Ignored because function might not exist.
+    'computed(function() { return this.children.mapBy(this.otherProp); })', // Ignored because value depends on function's `this`.
+    'computed(function() { return this.children.mapBy(someFunction(this.otherProp)); })', // Ignored because value depends on function's `this`.
 
     // Decorator (these are ignored when the `includeNativeGetters` option is off):
     "class Test { @computed('x') get someProp() { return this.x; } }",

tests/lib/utils/scope-references-this-test.js

@@ -0,0 +1,27 @@
+'use strict';
+
+const babelEslint = require('babel-eslint');
+const scopeReferencesThis = require('../../../lib/utils/scope-references-this');
+
+function parse(code) {
+  return babelEslint.parse(code).body[0];
+}
+
+describe('scopeReferencesThis', function () {
+  it('recognizes simple cases`', function () {
+    expect(scopeReferencesThis(parse('this'))).toBeTruthy(); // `this` uses `this`
+    expect(scopeReferencesThis(parse('"this"'))).toBeFalsy(); // the string "this" does not use this
+  });
+
+  it('can find nested `this`', function () {
+    expect(scopeReferencesThis(parse('if (a) { this } else { null }'))).toBeTruthy(); // if statement uses this
+    expect(scopeReferencesThis(parse('() => this'))).toBeTruthy(); // arrow function uses outer this
+  });
+
+  it('does not consider `this` within non-arrow functions', function () {
+    expect(scopeReferencesThis(parse('function foo() { return this; }'))).toBeFalsy(); // function uses different this
+    expect(scopeReferencesThis(parse('function foo() { return () => this; }'))).toBeFalsy(); // inner arrow function uses different this'
+    expect(scopeReferencesThis(parse('() => function() { return this; }'))).toBeFalsy(); // inner function uses different this
+    expect(scopeReferencesThis(parse('({ a() { this } })'))).toBeFalsy(); // object method uses different this
+  });
+});