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

Issue:
False positive with `require-computed-macros` rule using `this.` for filterBy/mapBy value
This example should not use the `filterBy` macro because `this.token` needs to remain inside a function:

```js
myProp: computed('items.@each.token', 'token', function () {
  return this.items.filterBy('token', this.token);
}),
```

Pull request:
Fix false positive involving `this` keyword with `filterBy` / `mapBy` in `require-computed-macros` rule
This example should not use the `filterBy` macro because `this.token` needs to remain inside a function:

```js
myProp: computed('items.@each.token', 'token', function () {
  return this.items.filterBy('token', this.token);
}),
```

CC: @mongoose700 

Fixes #869.

Patch:
diff --git a/lib/rules/require-computed-macros.js b/lib/rules/require-computed-macros.js
index 3fc76717..78b5bb10 100644
--- a/lib/rules/require-computed-macros.js
+++ b/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');
         }
       },
diff --git a/lib/utils/scope-references-this.js b/lib/utils/scope-references-this.js
new file mode 100644
index 00000000..91f9635d
--- /dev/null
+++ b/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;


Test patch:
diff --git a/lib/rules/no-invalid-test-waiters.js b/lib/rules/no-invalid-test-waiters.js
index ba95b7f0..270ba569 100644
--- a/lib/rules/no-invalid-test-waiters.js
+++ b/lib/rules/no-invalid-test-waiters.js
@@ -42,10 +42,7 @@ module.exports = {
 
     return {
       ImportDeclaration(node) {
-        if (node.source.value === 'ember-test-waiters') {
-          buildWaiter =
-            buildWaiter || getImportIdentifier(node, 'ember-test-waiters', 'buildWaiter');
-        }
+        buildWaiter = getImportIdentifier(node, 'ember-test-waiters', 'buildWaiter');
       },
 
       CallExpression(node) {
diff --git a/tests/lib/rules/no-invalid-test-waiters.js b/tests/lib/rules/no-invalid-test-waiters.js
index e92bfffc..d4496056 100644
--- a/tests/lib/rules/no-invalid-test-waiters.js
+++ b/tests/lib/rules/no-invalid-test-waiters.js
@@ -16,8 +16,6 @@ ruleTester.run('no-invalid-test-waiters', rule, {
   valid: [
     `
     import { buildWaiter } from 'ember-test-waiters';
-    import { random } from 'random';
-    import { somethingElse } from 'ember-test-waiters';
 
     let myWaiter = buildWaiter('waiterName');
   `,
@@ -54,8 +52,6 @@ ruleTester.run('no-invalid-test-waiters', rule, {
     {
       code: `
       import { buildWaiter } from 'ember-test-waiters';
-      import { random } from 'random';
-      import { somethingElse } from 'ember-test-waiters';
 
       function useWaiter() {
           let myOtherWaiter = buildWaiter('the second');
diff --git a/tests/lib/rules/require-computed-macros.js b/tests/lib/rules/require-computed-macros.js
index 232fd26f..16305b0e 100644
--- a/tests/lib/rules/require-computed-macros.js
+++ b/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; } }",
diff --git a/tests/lib/utils/scope-references-this-test.js b/tests/lib/utils/scope-references-this-test.js
new file mode 100644
index 00000000..4bb2ae66
--- /dev/null
+++ b/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
+  });
+});