Propose new rule: no-unsafe-this-access

README.md

@@ -158,6 +158,7 @@ Rules are grouped by category to help you understand their purpose. Each rule ha
 | [no-incorrect-calls-with-inline-anonymous-functions](./docs/rules/no-incorrect-calls-with-inline-anonymous-functions.md) | disallow inline anonymous functions as arguments to `debounce`, `once`, and `scheduleOnce` | ✅ |  |  |
 | [no-invalid-debug-function-arguments](./docs/rules/no-invalid-debug-function-arguments.md) | disallow usages of Ember's `assert()` / `warn()` / `deprecate()` functions that have the arguments passed in the wrong order. | ✅ |  |  |
 | [no-restricted-property-modifications](./docs/rules/no-restricted-property-modifications.md) | disallow modifying the specified properties |  | 🔧 |  |
+| [no-unsafe-this-access-in-async-function](./docs/rules/no-unsafe-this-access-in-async-function.md) | disallow `this` access after await unless destruction protection is present |  | 🔧 |  |
 | [require-fetch-import](./docs/rules/require-fetch-import.md) | enforce explicit import for `fetch()` |  |  |  |
 
 ### Routes

docs/rules/no-unsafe-this-access-in-async-function.md

@@ -0,0 +1,49 @@
+# no-unsafe-this-access-in-async-function
+
+🔧 The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule.
+
+With async behaviors, accessing `this` after an `await` may be unsafe. For example, an unsafe `this`-access situation may occur if a component is torn down before an async function runs to completion. When the function resumes execution, if the component is already torn down, `this` may be undefined. This also comes up in testing where the whole application is torn down and a function may try to access other framework objects, which will be unavailable when the application is torn down.
+
+## Rule Details
+
+TODO: what the rule does goes here
+
+## Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+// TODO: Example 1
+```
+
+```js
+// TODO: Example 2
+```
+
+Examples of **correct** code for this rule:
+
+```js
+// TODO: Example 1
+```
+
+```js
+// TODO: Example 2
+```
+
+## Migration
+
+TODO: suggest any fast/automated techniques for fixing violations in a large codebase
+
+* TODO: suggestion on how to fix violations using find-and-replace / regexp
+* TODO: suggestion on how to fix violations using a codemod
+
+## Related Rules
+
+* [TODO-related-rule-name1](related-rule-name1.md)
+* [TODO-related-rule-name2](related-rule-name2.md)
+
+## References
+
+* TODO: link to relevant documentation goes here)
+* TODO: link to relevant function spec goes here
+* TODO: link to relevant guide goes here

lib/rules/no-unsafe-this-access-in-async-function.js

@@ -0,0 +1,173 @@
+'use strict';
+
+const ERROR_MESSAGE =
+  // eslint-disable-next-line eslint-plugin/prefer-placeholders
+  'Unsafe `this` access after `await`. ' +
+  'Guard against accessing data on destroyed objects with `@ember/destroyable` `isDestroyed` and `isDestroying`';
+
+const types = require('../utils/types');
+const { getImportIdentifier } = require('../utils/import');
+
+// Test here:
+// https://astexplorer.net/#/gist/e364803b7c576e08f232839bf3c17287/15913876e050a1ca02af71932e14b14242e36ead
+
+/**
+ * These objects have their own destroyable APIs on `this`
+ */
+const FRAMEWORK_EXTENDABLES = [
+  {
+    importPath: '@glimmer/component',
+  },
+  {
+    importPath: '@ember/component',
+  },
+  {
+    importPath: '@ember/component/helper',
+  },
+  {
+    importPath: '@ember/routing/route',
+  },
+  {
+    importPath: '@ember/controller',
+  },
+];
+
+/**
+ * These objects don't have their own destroyable APIs but are
+ * wired in to the framework via associateDestroyableChild
+ */
+const KNOWN_DESTROYABLES = [
+  {
+    importPath: 'ember-modifier',
+  },
+];
+
+// if already has protection, also early return
+// two forms:
+//   - isDestroying(this) || isDestroyed(this) // on any destroyable object
+//   - this.isDestroying || this.isDestroyed  // available on most framework objects
+function isProtection(node) {
+  const fns = new Set(['isDestroying', 'isDestroyed']);
+
+  switch (node.type) {
+    case 'CallExpression': {
+      return (
+        fns.has(node.callee.name) &&
+        node.arguments.length === 1 &&
+        node.arguments[0].type === 'ThisExpression'
+      );
+    }
+    case 'MemberExpression':
+      return node.object.type === 'ThisExpression' && fns.has(node.property.name);
+    default:
+      console.log('unhandled protection check', node);
+  }
+
+  return false;
+}
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+  KNOWN_DESTROYABLES,
+  FRAMEWORK_EXTENDABLES,
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'disallow `this` access after await unless destruction protection is present',
+      category: 'Miscellaneous',
+      // Move to recommended in next major?
+      recommended: false,
+      url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-unsafe-this-access-in-async-function.md',
+    },
+    fixable: 'code',
+    schema: [],
+  },
+
+  create(context) {
+    const inFunction = [];
+    const inClass = [];
+    let encounteredAwait;
+    let lastProtection;
+
+    // https://eslint.org/docs/developer-guide/working-with-rules#contextgetsourcecode
+    const source = context.getSourceCode();
+
+    return {
+      ClassDeclaration(node) {
+        inClass.push(node);
+      },
+      'ClassDeclaration:exit'(node) {
+        inClass.pop();
+      },
+      FunctionExpression(node) {
+        inFunction.push(node);
+        encounteredAwait = null;
+      },
+      'FunctionExpression:exit'(node) {
+        inFunction.pop();
+      },
+      IfStatement(node) {
+        const { test } = node;
+
+        switch (test.type) {
+          case 'LogicalExpression': {
+            const { left, right } = test;
+
+            if (isProtection(left) || isProtection(right)) {
+              lastProtection = node;
+              encounteredAwait = null;
+            }
+            break;
+          }
+          default:
+            console.log('unhandled if statestatement', node);
+        }
+      },
+      AwaitExpression(node) {
+        if (inClass.length === 0) {
+          return;
+        }
+        if (inFunction.length === 0) {
+          return;
+        }
+
+        encounteredAwait = node.parent;
+      },
+      MemberExpression(node) {
+        if (node.object.type !== 'ThisExpression') {
+          return;
+        }
+        if (!encounteredAwait) {
+          return;
+        }
+
+        context.report({
+          node: node.object,
+          message: ERROR_MESSAGE,
+
+          // https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes
+          *fix(fixer) {
+            if (!encounteredAwait) {
+              return;
+            }
+
+            const toFix = encounteredAwait;
+            encounteredAwait = null;
+
+            const protection = '\nif (isDestroying(this) || isDestroyed(this)) return;';
+            const original = source.getText(toFix);
+
+            yield fixer.replaceText(toFix, original + protection);
+
+            // extend range of the fix to the range
+            yield fixer.insertTextBefore(toFix, '');
+            yield fixer.insertTextAfter(toFix, '');
+          },
+        });
+      },
+    };
+  },
+};

tests/lib/rules/no-unsafe-this-access-in-async-function.js

@@ -0,0 +1,116 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/no-unsafe-this-access-in-async-function');
+const RuleTester = require('eslint').RuleTester;
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 2020,
+    sourceType: 'module',
+  },
+  parser: require.resolve('@babel/eslint-parser'),
+});
+
+function eachDefaultExport(builder, rest = {}) {
+  const paths = [...rule.FRAMEWORK_EXTENDABLES, ...rule.KNOWN_DESTROYABLES].map(
+    (x) => x.importPath
+  );
+  const results = [];
+
+  for (const importPath of paths) {
+    const specifier = 'X';
+    const testCase = {
+      ...rest,
+      code: `import ${specifier} from '${importPath}'\n\n${builder(specifier)}`,
+    };
+
+    results.push(testCase);
+  }
+
+  return results;
+}
+
+ruleTester.run('no-unsafe-this-access-in-async-function', rule, {
+  valid: [
+    `class {
+      async foo() {
+        await Promise.resolve();
+        this.foo;
+      }
+    }`,
+    eachDefaultExport(
+      (parentClass) => `
+      class extends ${parentClass} {
+        async foo() {
+          await Promise.resolve();
+
+          if (this.isDestroying || this.isDestroyed) return;
+
+          this.hello();
+        }
+      }
+    `
+    ),
+    eachDefaultExport(
+      (parentClass) => `
+      import { isDestroying, isDestroyed } from '@ember/destroyable';
+
+      class extends ${parentClass} {
+        async foo() {
+          await Promise.resolve();
+
+          if (isDestroying(this) || isDestroyed(this)) return;
+
+          this.hello();
+        }
+      }
+    `
+    ),
+    eachDefaultExport(
+      (parentClass) => `
+      import { isDestroying as A, isDestroyed as B } from '@ember/destroyable';
+
+      class extends ${parentClass} {
+        async foo() {
+          await Promise.resolve();
+
+          if (B(this) || A(this)) return;
+
+          this.hello();
+        }
+      }
+    `
+    ),
+  ],
+  invalid: [
+    {
+      code: `
+        import Component from '@glimmer/component';
+
+        class extends Component {
+          async foo() {
+            await Promise.resolve();
+            this.foo;
+          }
+        }
+      `,
+      output: `
+        import Component from '@glimmer/component';
+
+        class extends Component {
+          async foo() {
+            await Promise.resolve();
+            if (this.isDestroyed || this.isDestroying) return;
+            this.foo;
+          }
+        }
+      `,
+    },
+  ],
+});

yarn.lock

@@ -1607,15 +1607,10 @@ camelcase@^6.2.0:
   resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-6.2.0.tgz#924af881c9d525ac9d87f40d964e5cea982a1809"
   integrity sha512-c7wVvbw3f37nuobQNtgsgG9POC9qMbNuMQmTCqZv23b6MIz0fcYpBiOlv9gEN/hdLdnZTDQhg6e9Dq5M1vKvfg==
 
-caniuse-lite@^1.0.30001219:
-  version "1.0.30001223"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001223.tgz#39b49ff0bfb3ee3587000d2f66c47addc6e14443"
-  integrity sha512-k/RYs6zc/fjbxTjaWZemeSmOjO0JJV+KguOBA3NwPup8uzxM1cMhR2BD9XmO86GuqaqTCO8CgkgH9Rz//vdDiA==
-
-caniuse-lite@^1.0.30001286:
-  version "1.0.30001298"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001298.tgz#0e690039f62e91c3ea581673d716890512e7ec52"
-  integrity sha512-AcKqikjMLlvghZL/vfTHorlQsLDhGRalYf1+GmWCf5SCMziSGjRYQW/JEksj14NaYHIR6KIhrFAy0HV5C25UzQ==
+caniuse-lite@^1.0.30001219, caniuse-lite@^1.0.30001286:
+  version "1.0.30001361"
+  resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001361.tgz"
+  integrity sha512-ybhCrjNtkFji1/Wto6SSJKkWk6kZgVQsDq5QI83SafsF6FXv2JB4df9eEdH6g8sdGgqTXrFLjAxqBGgYoU3azQ==
 
 chalk@4.1.2, chalk@^4.0.0, chalk@^4.1.0, chalk@^4.1.1, chalk@^4.1.2:
   version "4.1.2"