diff --git a/README.md b/README.md index 1108cf8b82..645513b847 100644 --- a/README.md +++ b/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 diff --git a/docs/rules/no-unsafe-this-access-in-async-function.md b/docs/rules/no-unsafe-this-access-in-async-function.md new file mode 100644 index 0000000000..2aa006a813 --- /dev/null +++ b/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 diff --git a/lib/rules/no-unsafe-this-access-in-async-function.js b/lib/rules/no-unsafe-this-access-in-async-function.js new file mode 100644 index 0000000000..f78ef0c371 --- /dev/null +++ b/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, ''); + }, + }); + }, + }; + }, +}; diff --git a/tests/lib/rules/no-unsafe-this-access-in-async-function.js b/tests/lib/rules/no-unsafe-this-access-in-async-function.js new file mode 100644 index 0000000000..820f6fbb70 --- /dev/null +++ b/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; + } + } + `, + }, + ], +}); diff --git a/yarn.lock b/yarn.lock index 79ce2cc7f9..ce900d0255 100644 --- a/yarn.lock +++ b/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"