From 1384bf533252b339af9ba4226267aeac2fc94396 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 15 Jun 2024 14:47:42 -0600 Subject: [PATCH 01/14] first stab --- .../eslint-plugin/docs/rules/return-await.mdx | 29 +++ .../eslint-plugin/src/rules/return-await.ts | 57 ++++-- .../tests/rules/return-await.test.ts | 185 +++++------------- 3 files changed, 122 insertions(+), 149 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 3c759b45aee7..1ceeeb82bbf6 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -203,6 +203,35 @@ async function validAlways3() { +### `error-handling-correctness-only` + +Same as `in-try-catch`, except _only_ enforces Promises be awaited within the +error handling contexts explained under that option. Outside of error-handling +contexts, returned Promises are ignored, whether awaited or not. + +:::warning +We recommend you configure either `in-try-catch` or `always` instead of this option. +While the choice or awaiting or not awaiting promises outside of error-handling contexts is mostly stylistic, it is 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. diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 98264e704789..6ae1660197ff 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -50,7 +50,12 @@ export default createRule({ schema: [ { type: 'string', - enum: ['in-try-catch', 'always', 'never'], + enum: [ + 'in-try-catch', + 'always', + 'never', + 'error-handling-correctness-only', + ], }, ], }, @@ -283,29 +288,53 @@ export default createRule({ return; } + if (option === 'error-handling-correctness-only') { + if (!isAwait && affectsErrorHandling) { + context.report({ + messageId: 'requiredPromiseAwait', + node, + // unconditional since always impacts error handling + suggest: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + fix: fixer => + insertAwait( + fixer, + node, + isHigherPrecedenceThanAwait(expression), + ), + }, + ], + }); + + return; + } + } + if (option === 'in-try-catch') { if (isAwait && !affectsErrorHandling) { context.report({ messageId: 'disallowedPromiseAwait', node, - ...fixOrSuggest(useAutoFix, { - messageId: 'disallowedPromiseAwaitSuggestion', - fix: fixer => removeAwait(fixer, node), - }), + // unconditional since never impacts error handling + 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), - ), - }), + // unconditional since always impacts error handling + suggest: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + fix: fixer => + insertAwait( + fixer, + node, + isHigherPrecedenceThanAwait(expression), + ), + }, + ], }); } diff --git a/packages/eslint-plugin/tests/rules/return-await.test.ts b/packages/eslint-plugin/tests/rules/return-await.test.ts index 4a8349031096..7fc612b68715 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -2,6 +2,7 @@ import { noFormat, RuleTester } from '@typescript-eslint/rule-tester'; import rule from '../../src/rules/return-await'; import { getFixturesRootDir } from '../RuleTester'; +import { InvalidTestCase } from '@typescript-eslint/utils/ts-eslint'; const rootDir = getFixturesRootDir(); @@ -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 { @@ -435,8 +455,11 @@ async function test() { }, ], }, - { - code: ` + + ...['error-handling-correctness-only', 'always', 'in-try-catch'].map( + option => + ({ + code: ` async function test() { try { return Promise.resolve(1); @@ -447,15 +470,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); @@ -466,16 +489,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); @@ -486,11 +509,17 @@ async function test() { } } `, + }, + ], }, ], - }, - ], - }, + options: [option], + }) satisfies InvalidTestCase< + 'requiredPromiseAwait' | 'requiredPromiseAwaitSuggestion', + [string] + >, + ), + { code: ` async function test() { @@ -550,63 +579,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: ` @@ -740,63 +712,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: ` From bc8310fd71ae307404eb90140a8a61a6ce3c568e Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 15 Jun 2024 15:23:10 -0600 Subject: [PATCH 02/14] fix CI stuff --- packages/eslint-plugin/src/rules/return-await.ts | 4 ++-- .../return-await.shot | 16 +++++++++++++++- .../tests/rules/return-await.test.ts | 6 +++--- .../tests/schema-snapshots/return-await.shot | 11 +++++++++-- 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 6ae1660197ff..63eac88e486b 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -297,7 +297,7 @@ export default createRule({ suggest: [ { messageId: 'requiredPromiseAwaitSuggestion', - fix: fixer => + fix: (fixer): TSESLint.RuleFix | TSESLint.RuleFix[] => insertAwait( fixer, node, @@ -327,7 +327,7 @@ export default createRule({ suggest: [ { messageId: 'requiredPromiseAwaitSuggestion', - fix: fixer => + fix: (fixer): TSESLint.RuleFix | TSESLint.RuleFix[] => insertAwait( fixer, node, 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 d6b583edbe7f..0308d9c81385 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 @@ -167,6 +167,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" @@ -189,7 +203,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 7fc612b68715..84d33ba2b5cd 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -1,8 +1,8 @@ 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'; -import { InvalidTestCase } from '@typescript-eslint/utils/ts-eslint'; const rootDir = getFixturesRootDir(); @@ -62,10 +62,10 @@ ruleTester.run('return-await', rule, { async function test() { if (Math.random() < 0.33) { return await Promise.resolve(1); - } else if (Math.random () < 0.5) { + } else if (Math.random() < 0.5) { return Promise.resolve(2); } - + try { } catch (e) { return await Promise.resolve(3); 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', +]; " `; From 7c5340f45062477da956d6102e266dbb44be0af6 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 17 Jun 2024 20:43:44 -0600 Subject: [PATCH 03/14] Update packages/eslint-plugin/src/rules/return-await.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Josh Goldberg ✨ --- packages/eslint-plugin/src/rules/return-await.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 63eac88e486b..55c3a00198b2 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -293,7 +293,7 @@ export default createRule({ context.report({ messageId: 'requiredPromiseAwait', node, - // unconditional since always impacts error handling + // We always report here, as it always impacts error handling suggest: [ { messageId: 'requiredPromiseAwaitSuggestion', From 624eeee7cc7b589d18eb9057364451ed22f60fec Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 17 Jun 2024 21:06:16 -0600 Subject: [PATCH 04/14] change comments and stuff --- packages/eslint-plugin/src/rules/return-await.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 55c3a00198b2..78b4bdf3a1ba 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -251,14 +251,13 @@ export default createRule({ } const affectsErrorHandling = affectsExplicitErrorHandling(expression); - const useAutoFix = !affectsErrorHandling; if (option === 'always') { if (!isAwait && isThenable) { context.report({ messageId: 'requiredPromiseAwait', node, - ...fixOrSuggest(useAutoFix, { + ...fixOrSuggest(!affectsErrorHandling, { messageId: 'requiredPromiseAwaitSuggestion', fix: fixer => insertAwait( @@ -278,7 +277,7 @@ export default createRule({ context.report({ messageId: 'disallowedPromiseAwait', node, - ...fixOrSuggest(useAutoFix, { + ...fixOrSuggest(!affectsErrorHandling, { messageId: 'disallowedPromiseAwaitSuggestion', fix: fixer => removeAwait(fixer, node), }), @@ -293,7 +292,7 @@ export default createRule({ context.report({ messageId: 'requiredPromiseAwait', node, - // We always report here, as it always impacts error handling + // Suggest, don't fix, since this affects error handling control flow. suggest: [ { messageId: 'requiredPromiseAwaitSuggestion', @@ -316,14 +315,14 @@ export default createRule({ context.report({ messageId: 'disallowedPromiseAwait', node, - // unconditional since never impacts error handling + // Safe to fix, since this doesn't affect error handling control flow. fix: fixer => removeAwait(fixer, node), }); } else if (!isAwait && affectsErrorHandling) { context.report({ messageId: 'requiredPromiseAwait', node, - // unconditional since always impacts error handling + // Suggest, don't fix, since this affects error handling control flow. suggest: [ { messageId: 'requiredPromiseAwaitSuggestion', From 93c66bee40a55677a978db2e8c2019702a56698f Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 24 Jun 2024 20:40:59 -0600 Subject: [PATCH 05/14] add table to docs --- .../eslint-plugin/docs/rules/return-await.mdx | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 8231a2ee870d..dc566c2bf048 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -21,7 +21,11 @@ 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'; ``` @@ -289,3 +293,12 @@ async function validNever3() { + +### Options Summary + +| Option | Ordinary Context
(stylistic preference) | Error Handling Context
(catches subtle control flow bugs) | Should I use? | +| :-----------------------------: | :-------------------------------------------: | :-------------------------------------------------------------: | :--------------------------------------------------------: | +| 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;` | ❌ No! | From 59192fc98596104187d420d7e639c834d2b5cfae Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 24 Jun 2024 21:07:59 -0600 Subject: [PATCH 06/14] dry it up --- .../eslint-plugin/src/rules/return-await.ts | 172 +++++++++--------- 1 file changed, 82 insertions(+), 90 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 841bae24903e..41a839f16c83 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: { @@ -55,7 +61,7 @@ export default createRule({ 'always', 'never', 'error-handling-correctness-only', - ], + ] satisfies Option[], }, ], }, @@ -67,6 +73,38 @@ export default createRule({ const scopeInfoStack: ScopeInfo[] = []; + 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 enterFunction(node: FunctionNode): void { scopeInfoStack.push({ hasAsync: node.async, @@ -276,115 +314,69 @@ 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)); + // handle awaited _non_thenables - context.report({ - messageId: 'nonPromiseAwait', - node, - ...fixOrSuggest(useAutoFix, { - messageId: 'nonPromiseAwait', - fix: fixer => removeAwait(fixer, node), - }), - }); - return; - } - - const affectsErrorHandling = - affectsExplicitErrorHandling(expression) || - affectsExplicitResourceManagement(node); + 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(!affectsErrorHandling, { - messageId: 'requiredPromiseAwaitSuggestion', - fix: fixer => - insertAwait( - fixer, - node, - isHigherPrecedenceThanAwait(expression), - ), + ...fixOrSuggest(useAutoFix, { + messageId: 'nonPromiseAwait', + fix: fixer => removeAwait(fixer, node), }), }); } - return; } - if (option === 'never') { - if (isAwait) { - context.report({ - messageId: 'disallowedPromiseAwait', - node, - ...fixOrSuggest(!affectsErrorHandling, { - messageId: 'disallowedPromiseAwaitSuggestion', - fix: fixer => removeAwait(fixer, node), - }), - }); - } + // At this point it's definitely a thenable. - return; - } + const affectsErrorHandling = + affectsExplicitErrorHandling(expression) || + affectsExplicitResourceManagement(node); - if (option === 'error-handling-correctness-only') { - if (!isAwait && affectsErrorHandling) { - context.report({ - messageId: 'requiredPromiseAwait', - node, - // Suggest, don't fix, since this affects error handling control flow. - suggest: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - fix: (fixer): TSESLint.RuleFix | TSESLint.RuleFix[] => - insertAwait( - fixer, - node, - isHigherPrecedenceThanAwait(expression), - ), - }, - ], - }); + const ruleConfiguration = getConfiguration(option as Option); - return; - } - } + const shouldAwaitInCurrentContext = affectsErrorHandling + ? ruleConfiguration.errorHandlingContext + : ruleConfiguration.ordinaryContext; - if (option === 'in-try-catch') { - if (isAwait && !affectsErrorHandling) { - context.report({ - messageId: 'disallowedPromiseAwait', - node, - // Safe to fix, since this doesn't affect error handling control flow. - fix: fixer => removeAwait(fixer, node), - }); - } else if (!isAwait && affectsErrorHandling) { - context.report({ - messageId: 'requiredPromiseAwait', - node, - // Suggest, don't fix, since this affects error handling control flow. - suggest: [ - { + switch (shouldAwaitInCurrentContext) { + case "don't-care": + break; + case 'await': + if (!isAwait) { + context.report({ + messageId: 'requiredPromiseAwait', + node, + ...fixOrSuggest(!affectsErrorHandling, { messageId: 'requiredPromiseAwaitSuggestion', - fix: (fixer): TSESLint.RuleFix | TSESLint.RuleFix[] => + fix: fixer => insertAwait( fixer, node, isHigherPrecedenceThanAwait(expression), ), - }, - ], - }); - } - - return; + }), + }); + } + break; + case 'no-await': + if (isAwait) { + context.report({ + messageId: 'disallowedPromiseAwait', + node, + ...fixOrSuggest(!affectsErrorHandling, { + messageId: 'disallowedPromiseAwaitSuggestion', + fix: fixer => removeAwait(fixer, node), + }), + }); + } + break; } } From c2bac5cf0847a4bdb92711c751e6c1266ad353c5 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 24 Jun 2024 21:21:47 -0600 Subject: [PATCH 07/14] revert useAutoFix --- packages/eslint-plugin/src/rules/return-await.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 41a839f16c83..3623b9bba426 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -338,6 +338,7 @@ export default createRule({ const affectsErrorHandling = affectsExplicitErrorHandling(expression) || affectsExplicitResourceManagement(node); + const useAutoFix = !affectsErrorHandling; const ruleConfiguration = getConfiguration(option as Option); @@ -353,7 +354,7 @@ export default createRule({ context.report({ messageId: 'requiredPromiseAwait', node, - ...fixOrSuggest(!affectsErrorHandling, { + ...fixOrSuggest(useAutoFix, { messageId: 'requiredPromiseAwaitSuggestion', fix: fixer => insertAwait( @@ -370,7 +371,7 @@ export default createRule({ context.report({ messageId: 'disallowedPromiseAwait', node, - ...fixOrSuggest(!affectsErrorHandling, { + ...fixOrSuggest(useAutoFix, { messageId: 'disallowedPromiseAwaitSuggestion', fix: fixer => removeAwait(fixer, node), }), From 5150439e373f5273de30fdce7597f582c0ffdeaa Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 24 Jun 2024 23:04:56 -0600 Subject: [PATCH 08/14] Update packages/eslint-plugin/docs/rules/return-await.mdx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Josh Goldberg ✨ --- packages/eslint-plugin/docs/rules/return-await.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index dc566c2bf048..7f0400898d67 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -226,7 +226,7 @@ contexts, returned Promises are ignored, whether awaited or not. :::warning We recommend you configure either `in-try-catch` or `always` instead of this option. -While the choice or awaiting or not awaiting promises outside of error-handling contexts is mostly stylistic, it is best to be consistent. +While the choice of whether to awaiting 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`: From d050fe4783b51b684d1a7b52f2390a08ea1b8a1b Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 24 Jun 2024 23:06:15 -0600 Subject: [PATCH 09/14] to awaiting --- packages/eslint-plugin/docs/rules/return-await.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 7f0400898d67..8eaa5e54b8dd 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -226,7 +226,7 @@ contexts, returned Promises are ignored, whether awaited or not. :::warning We recommend you configure either `in-try-catch` or `always` instead of this option. -While the choice of whether to awaiting promises outside of error-handling contexts is mostly stylistic, it's generally best to be consistent. +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`: From c12dfebde74f631ae3cb6a793d2ed61996740375 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 8 Jul 2024 01:42:57 -0600 Subject: [PATCH 10/14] changes and deprecation --- .../eslint-plugin/docs/rules/return-await.mdx | 62 ++++++++++++------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 8eaa5e54b8dd..b48d28344099 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -30,18 +30,32 @@ type Options = const defaultOptions: Options = 'in-try-catch'; ``` -### `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: -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. +| 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. | -Listing the error-handling cases exhaustively: +### `in-try-catch` + +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. -- 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`: @@ -173,7 +187,7 @@ async function validInTryCatch7() { ### `always` -Requires that all returned promises are `await`ed. +Requires that all returned promises are awaited. Examples of code with `always`: @@ -220,11 +234,10 @@ async function validAlways3() { ### `error-handling-correctness-only` -Same as `in-try-catch`, except _only_ enforces Promises be awaited within the -error handling contexts explained under that option. Outside of error-handling -contexts, returned Promises are ignored, whether awaited or not. +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. -:::warning +:::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. ::: @@ -249,7 +262,17 @@ async function asyncFunction(): Promise { ### `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`: @@ -293,12 +316,3 @@ async function validNever3() { - -### Options Summary - -| Option | Ordinary Context
(stylistic preference) | Error Handling Context
(catches subtle control flow bugs) | Should I use? | -| :-----------------------------: | :-------------------------------------------: | :-------------------------------------------------------------: | :--------------------------------------------------------: | -| 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;` | ❌ No! | From 5ea1b81ce3874ad0e684c1d4581c51a6808719d4 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 8 Jul 2024 01:57:31 -0600 Subject: [PATCH 11/14] mention motivation for each option in its section --- packages/eslint-plugin/docs/rules/return-await.mdx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index b48d28344099..256932390c28 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -55,6 +55,8 @@ With these terms defined, the options may be summarized as follows: 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. +This is a good option if you prefer the shorter `return promise` form for stylistic reasons, wherever it's safe to use. + Examples of code with `in-try-catch`: @@ -189,6 +191,8 @@ async function validInTryCatch7() { Requires that all returned promises are 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`: @@ -237,6 +241,8 @@ async function validAlways3() { 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. From 584aad9a0a838520f4a4132a374ab236811d5f58 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 8 Jul 2024 02:03:06 -0600 Subject: [PATCH 12/14] subjunctive --- packages/eslint-plugin/docs/rules/return-await.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 256932390c28..4897e6d4bad3 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -189,7 +189,7 @@ async function validInTryCatch7() { ### `always` -Requires that all returned promises are awaited. +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. From b27b673cb8427026ce7dea60d0c9aa1d335226f4 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Tue, 16 Jul 2024 15:25:44 -0600 Subject: [PATCH 13/14] Update packages/eslint-plugin/docs/rules/return-await.mdx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Josh Goldberg ✨ --- packages/eslint-plugin/docs/rules/return-await.mdx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 4897e6d4bad3..9be63cc9600c 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -43,12 +43,12 @@ The choice of whether to await a returned promise in an ordinary context is most 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. | +| 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` From f5a40604703b2da513e266419af1e689c6ed1379 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Thu, 18 Jul 2024 11:48:31 -0600 Subject: [PATCH 14/14] move things out of closure --- .../eslint-plugin/src/rules/return-await.ts | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 3623b9bba426..493bc1ec184c 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -73,38 +73,6 @@ export default createRule({ const scopeInfoStack: ScopeInfo[] = []; - 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 enterFunction(node: FunctionNode): void { scopeInfoStack.push({ hasAsync: node.async, @@ -427,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,