diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index ea5304bdfc39..9be63cc9600c 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -21,23 +21,43 @@ The extended rule is named `return-await` instead of `no-return-await` because t ## Options ```ts -type Options = 'in-try-catch' | 'always' | 'never'; +type Options = + | 'in-try-catch' + | 'always' + | 'error-handling-correctness-only' + | 'never'; const defaultOptions: Options = 'in-try-catch'; ``` +The options in this rule distinguish between "ordinary contexts" and "error-handling contexts". +An error-handling context is anywhere where returning an unawaited promise would cause unexpected control flow regarding exceptions/rejections. +See detailed examples in the sections for each option. + +- If you return a promise within a `try` block, it should be awaited in order to trigger subsequent `catch` or `finally` blocks as expected. +- If you return a promise within a `catch` block, and there _is_ a `finally` block, it should be awaited in order to trigger the `finally` block as expected. +- If you return a promise between a `using` or `await using` declaration and the end of its scope, it should be awaited, since it behaves equivalently to code wrapped in a `try` block followed by a `finally`. + +Ordinary contexts are anywhere else a promise may be returned. +The choice of whether to await a returned promise in an ordinary context is mostly stylistic. + +With these terms defined, the options may be summarized as follows: + +| Option | Ordinary Context
(stylistic preference 🎨) | Error-Handling Context
(catches bugs 🐛) | Should I use this option? | +| :-------------------------------: | :----------------------------------------------: | :----------------------------------------------------------: | :--------------------------------------------------------: | +| `always` | `return await promise;` | `return await promise;` | ✅ Yes! | +| `in-try-catch` | `return promise;` | `return await promise;` | ✅ Yes! | +| `error-handling-correctness-only` | don't care 🤷 | `return await promise;` | 🟡 Okay to use, but the above options would be preferable. | +| `never` | `return promise;` | `return promise;`
(⚠️ This behavior may be harmful ⚠️) | ❌ No. This option is deprecated. | + ### `in-try-catch` -In cases where returning an unawaited promise would cause unexpected error-handling control flow, the rule enforces that `await` must be used. -Otherwise, the rule enforces that `await` must _not_ be used. +In error-handling contexts, the rule enforces that returned promises must be awaited. +In ordinary contexts, the rule enforces that returned promises _must not_ be awaited. -Listing the error-handling cases exhaustively: +This is a good option if you prefer the shorter `return promise` form for stylistic reasons, wherever it's safe to use. -- if you `return` a promise within a `try`, then it must be `await`ed, since it will always be followed by a `catch` or `finally`. -- if you `return` a promise within a `catch`, and there is _no_ `finally`, then it must _not_ be `await`ed. -- if you `return` a promise within a `catch`, and there _is_ a `finally`, then it _must_ be `await`ed. -- if you `return` a promise within a `finally`, then it must not be `await`ed. -- if you `return` a promise between a `using` or `await using` declaration and the end of its scope, it must be `await`ed, since it behaves equivalently to code wrapped in a `try` block. +Examples of code with `in-try-catch`: @@ -169,7 +189,9 @@ async function validInTryCatch7() { ### `always` -Requires that all returned promises are `await`ed. +Requires that all returned promises be awaited. + +This is a good option if you like the consistency of simply always awaiting promises, or prefer not having to consider the distinction between error-handling contexts and ordinary contexts. Examples of code with `always`: @@ -214,9 +236,49 @@ async function validAlways3() { +### `error-handling-correctness-only` + +In error-handling contexts, the rule enforces that returned promises must be awaited. +In ordinary contexts, the rule does not enforce any particular behavior around whether returned promises are awaited. + +This is a good option if you only want to benefit from rule's ability to catch control flow bugs in error-handling contexts, but don't want to enforce a particular style otherwise. + +:::info +We recommend you configure either `in-try-catch` or `always` instead of this option. +While the choice of whether to await promises outside of error-handling contexts is mostly stylistic, it's generally best to be consistent. +::: + +Examples of additional correct code with `error-handling-correctness-only`: + + + + +```ts option='"error-handling-correctness-only"' +async function asyncFunction(): Promise { + if (Math.random() < 0.5) { + return await Promise.resolve(); + } else { + return Promise.resolve(); + } +} +``` + + + + ### `never` -Disallows all `await`ing any returned promises. +Disallows awaiting any returned promises. + +:::warning + +This option is deprecated and will be removed in a future major version of typescript-eslint. + +The `never` option introduces undesirable behavior in error-handling contexts. +If you prefer to minimize returning awaited promises, consider instead using `in-try-catch` instead, which also generally bans returning awaited promises, but only where it is _safe_ not to await a promise. + +See more details at [typescript-eslint#9433](https://github.com/typescript-eslint/typescript-eslint/issues/9433). +::: Examples of code with `never`: diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index ab4534d2a269..493bc1ec184c 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -24,6 +24,12 @@ interface ScopeInfo { owningFunc: FunctionNode; } +type Option = + | 'in-try-catch' + | 'always' + | 'never' + | 'error-handling-correctness-only'; + export default createRule({ name: 'return-await', meta: { @@ -50,7 +56,12 @@ export default createRule({ schema: [ { type: 'string', - enum: ['in-try-catch', 'always', 'never'], + enum: [ + 'in-try-catch', + 'always', + 'never', + 'error-handling-correctness-only', + ] satisfies Option[], }, ], }, @@ -271,92 +282,70 @@ export default createRule({ const type = checker.getTypeAtLocation(child); const isThenable = tsutils.isThenableType(checker, expression, type); - if (!isAwait && !isThenable) { - return; - } - - if (isAwait && !isThenable) { - // any/unknown could be thenable; do not auto-fix - const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type)); - - context.report({ - messageId: 'nonPromiseAwait', - node, - ...fixOrSuggest(useAutoFix, { - messageId: 'nonPromiseAwait', - fix: fixer => removeAwait(fixer, node), - }), - }); - return; - } + // handle awaited _non_thenables - const affectsErrorHandling = - affectsExplicitErrorHandling(expression) || - affectsExplicitResourceManagement(node); - const useAutoFix = !affectsErrorHandling; + if (!isThenable) { + if (isAwait) { + // any/unknown could be thenable; do not auto-fix + const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type)); - if (option === 'always') { - if (!isAwait && isThenable) { context.report({ - messageId: 'requiredPromiseAwait', + messageId: 'nonPromiseAwait', node, ...fixOrSuggest(useAutoFix, { - messageId: 'requiredPromiseAwaitSuggestion', - fix: fixer => - insertAwait( - fixer, - node, - isHigherPrecedenceThanAwait(expression), - ), + messageId: 'nonPromiseAwait', + fix: fixer => removeAwait(fixer, node), }), }); } - return; } - if (option === 'never') { - if (isAwait) { - context.report({ - messageId: 'disallowedPromiseAwait', - node, - ...fixOrSuggest(useAutoFix, { - messageId: 'disallowedPromiseAwaitSuggestion', - fix: fixer => removeAwait(fixer, node), - }), - }); - } + // At this point it's definitely a thenable. - return; - } + const affectsErrorHandling = + affectsExplicitErrorHandling(expression) || + affectsExplicitResourceManagement(node); + const useAutoFix = !affectsErrorHandling; - if (option === 'in-try-catch') { - if (isAwait && !affectsErrorHandling) { - context.report({ - messageId: 'disallowedPromiseAwait', - node, - ...fixOrSuggest(useAutoFix, { - messageId: 'disallowedPromiseAwaitSuggestion', - fix: fixer => removeAwait(fixer, node), - }), - }); - } else if (!isAwait && affectsErrorHandling) { - context.report({ - messageId: 'requiredPromiseAwait', - node, - ...fixOrSuggest(useAutoFix, { - messageId: 'requiredPromiseAwaitSuggestion', - fix: fixer => - insertAwait( - fixer, - node, - isHigherPrecedenceThanAwait(expression), - ), - }), - }); - } + const ruleConfiguration = getConfiguration(option as Option); - return; + const shouldAwaitInCurrentContext = affectsErrorHandling + ? ruleConfiguration.errorHandlingContext + : ruleConfiguration.ordinaryContext; + + switch (shouldAwaitInCurrentContext) { + case "don't-care": + break; + case 'await': + if (!isAwait) { + context.report({ + messageId: 'requiredPromiseAwait', + node, + ...fixOrSuggest(useAutoFix, { + messageId: 'requiredPromiseAwaitSuggestion', + fix: fixer => + insertAwait( + fixer, + node, + isHigherPrecedenceThanAwait(expression), + ), + }), + }); + } + break; + case 'no-await': + if (isAwait) { + context.report({ + messageId: 'disallowedPromiseAwait', + node, + ...fixOrSuggest(useAutoFix, { + messageId: 'disallowedPromiseAwaitSuggestion', + fix: fixer => removeAwait(fixer, node), + }), + }); + } + break; } } @@ -406,6 +395,38 @@ export default createRule({ }, }); +type WhetherToAwait = 'await' | 'no-await' | "don't-care"; + +interface RuleConfiguration { + ordinaryContext: WhetherToAwait; + errorHandlingContext: WhetherToAwait; +} + +function getConfiguration(option: Option): RuleConfiguration { + switch (option) { + case 'always': + return { + ordinaryContext: 'await', + errorHandlingContext: 'await', + }; + case 'never': + return { + ordinaryContext: 'no-await', + errorHandlingContext: 'no-await', + }; + case 'error-handling-correctness-only': + return { + ordinaryContext: "don't-care", + errorHandlingContext: 'await', + }; + case 'in-try-catch': + return { + ordinaryContext: 'no-await', + errorHandlingContext: 'await', + }; + } +} + function fixOrSuggest( useFix: boolean, suggestion: TSESLint.SuggestionReportDescriptor, diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/return-await.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/return-await.shot index 1bd15b8a41e8..d10c743c959c 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/return-await.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/return-await.shot @@ -178,6 +178,20 @@ async function validAlways3() { `; exports[`Validating rule docs return-await.mdx code examples ESLint output 5`] = ` +"Correct +Options: "error-handling-correctness-only" + +async function asyncFunction(): Promise { + if (Math.random() < 0.5) { + return await Promise.resolve(); + } else { + return Promise.resolve(); + } +} +" +`; + +exports[`Validating rule docs return-await.mdx code examples ESLint output 6`] = ` "Incorrect Options: "never" @@ -200,7 +214,7 @@ async function invalidNever3() { " `; -exports[`Validating rule docs return-await.mdx code examples ESLint output 6`] = ` +exports[`Validating rule docs return-await.mdx code examples ESLint output 7`] = ` "Correct Options: "never" diff --git a/packages/eslint-plugin/tests/rules/return-await.test.ts b/packages/eslint-plugin/tests/rules/return-await.test.ts index bc527b805e57..98175766e733 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -1,4 +1,5 @@ import { noFormat, RuleTester } from '@typescript-eslint/rule-tester'; +import type { InvalidTestCase } from '@typescript-eslint/utils/ts-eslint'; import rule from '../../src/rules/return-await'; import { getFixturesRootDir } from '../RuleTester'; @@ -56,6 +57,25 @@ ruleTester.run('return-await', rule, { } } `, + { + code: ` + async function test() { + if (Math.random() < 0.33) { + return await Promise.resolve(1); + } else if (Math.random() < 0.5) { + return Promise.resolve(2); + } + + try { + } catch (e) { + return await Promise.resolve(3); + } finally { + console.log('cleanup'); + } + } + `, + options: ['error-handling-correctness-only'], + }, ` async function test() { try { @@ -548,8 +568,11 @@ async function test() { }, ], }, - { - code: ` + + ...['error-handling-correctness-only', 'always', 'in-try-catch'].map( + option => + ({ + code: ` async function test() { try { return Promise.resolve(1); @@ -560,15 +583,15 @@ async function test() { } } `, - output: null, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', - suggestions: [ + output: null, + errors: [ { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` + line: 4, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { return await Promise.resolve(1); @@ -579,16 +602,16 @@ async function test() { } } `, + }, + ], }, - ], - }, - { - line: 6, - messageId: 'requiredPromiseAwait', - suggestions: [ { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` + line: 6, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { return Promise.resolve(1); @@ -599,11 +622,17 @@ async function test() { } } `, + }, + ], }, ], - }, - ], - }, + options: [option], + }) satisfies InvalidTestCase< + 'requiredPromiseAwait' | 'requiredPromiseAwaitSuggestion', + [string] + >, + ), + { code: ` async function test() { @@ -663,63 +692,6 @@ async function test() { }, ], }, - { - options: ['in-try-catch'], - code: ` - async function test() { - try { - return Promise.resolve(1); - } catch (e) { - return Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - output: null, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', - suggestions: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` - async function test() { - try { - return await Promise.resolve(1); - } catch (e) { - return Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - }, - ], - }, - { - line: 6, - messageId: 'requiredPromiseAwait', - suggestions: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` - async function test() { - try { - return Promise.resolve(1); - } catch (e) { - return await Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - }, - ], - }, - ], - }, { options: ['in-try-catch'], code: ` @@ -853,63 +825,6 @@ async function test() { }, ], }, - { - options: ['always'], - code: ` - async function test() { - try { - return Promise.resolve(1); - } catch (e) { - return Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - output: null, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', - suggestions: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` - async function test() { - try { - return await Promise.resolve(1); - } catch (e) { - return Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - }, - ], - }, - { - line: 6, - messageId: 'requiredPromiseAwait', - suggestions: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - output: ` - async function test() { - try { - return Promise.resolve(1); - } catch (e) { - return await Promise.resolve(2); - } finally { - console.log('cleanup'); - } - } - `, - }, - ], - }, - ], - }, { options: ['always'], code: ` diff --git a/packages/eslint-plugin/tests/schema-snapshots/return-await.shot b/packages/eslint-plugin/tests/schema-snapshots/return-await.shot index 5d79a331bf33..7096730066db 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/return-await.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/return-await.shot @@ -6,7 +6,12 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos [ { - "enum": ["always", "in-try-catch", "never"], + "enum": [ + "always", + "error-handling-correctness-only", + "in-try-catch", + "never" + ], "type": "string" } ] @@ -14,6 +19,8 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos # TYPES: -type Options = ['always' | 'in-try-catch' | 'never']; +type Options = [ + 'always' | 'error-handling-correctness-only' | 'in-try-catch' | 'never', +]; " `;