From 3d6ed66e002395d5d73bad557d7e9a29eb2a7df6 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 12 Apr 2024 13:54:15 -0600 Subject: [PATCH 1/5] [no-return-await] Clean up the logic for in-try-catch detection and make autofixes safe --- .../eslint-plugin/docs/rules/return-await.mdx | 55 +- .../eslint-plugin/src/rules/return-await.ts | 183 ++++-- .../tests/rules/return-await.test.ts | 523 +++++++++++++++--- 3 files changed, 600 insertions(+), 161 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.mdx b/packages/eslint-plugin/docs/rules/return-await.mdx index 3e0599dc1f0e..3c759b45aee7 100644 --- a/packages/eslint-plugin/docs/rules/return-await.mdx +++ b/packages/eslint-plugin/docs/rules/return-await.mdx @@ -26,13 +26,15 @@ const defaultOptions: Options = 'in-try-catch'; ### `in-try-catch` -Requires that a returned promise must be `await`ed in `try-catch-finally` blocks, and disallows it elsewhere. -Specifically: +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. -- if you `return` a promise within a `try`, then it must be `await`ed. -- 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. +Listing the error-handling cases exhaustively: + +- 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. Examples of code with `in-try-catch`: @@ -42,23 +44,34 @@ Examples of code with `in-try-catch`: ```ts option='"in-try-catch"' async function invalidInTryCatch1() { try { - return Promise.resolve('try'); - } catch (e) {} + return Promise.reject('try'); + } catch (e) { + // Doesn't execute due to missing await. + } } async function invalidInTryCatch2() { try { throw new Error('error'); } catch (e) { - return await Promise.resolve('catch'); + // Unnecessary await; rejections here don't impact control flow. + return await Promise.reject('catch'); } } +// Prints 'starting async work', 'cleanup', 'async work done'. async function invalidInTryCatch3() { + async function doAsyncWork(): Promise { + console.log('starting async work'); + await new Promise(resolve => setTimeout(resolve, 1000)); + console.log('async work done'); + } + try { throw new Error('error'); } catch (e) { - return Promise.resolve('catch'); + // Missing await. + return doAsyncWork(); } finally { console.log('cleanup'); } @@ -70,7 +83,8 @@ async function invalidInTryCatch4() { } catch (e) { throw new Error('error2'); } finally { - return await Promise.resolve('finally'); + // Unnecessary await; rejections here don't impact control flow. + return await Promise.reject('finally'); } } @@ -89,23 +103,32 @@ async function invalidInTryCatch6() { ```ts option='"in-try-catch"' async function validInTryCatch1() { try { - return await Promise.resolve('try'); - } catch (e) {} + return await Promise.reject('try'); + } catch (e) { + // Executes as expected. + } } async function validInTryCatch2() { try { throw new Error('error'); } catch (e) { - return Promise.resolve('catch'); + return Promise.reject('catch'); } } +// Prints 'starting async work', 'async work done', 'cleanup'. async function validInTryCatch3() { + async function doAsyncWork(): Promise { + console.log('starting async work'); + await new Promise(resolve => setTimeout(resolve, 1000)); + console.log('async work done'); + } + try { throw new Error('error'); } catch (e) { - return await Promise.resolve('catch'); + return await doAsyncWork(); } finally { console.log('cleanup'); } @@ -117,7 +140,7 @@ async function validInTryCatch4() { } catch (e) { throw new Error('error2'); } finally { - return Promise.resolve('finally'); + return Promise.reject('finally'); } } diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index d86b0874f8f3..b0b8d7bc382e 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -41,6 +41,10 @@ export default createRule({ 'Returning an awaited promise is not allowed in this context.', requiredPromiseAwait: 'Returning an awaited promise is required in this context.', + requiredPromiseAwaitSuggestion: + 'Add `await` before the expression. Use caution as this may impact control flow.', + disallowedPromiseAwaitSuggestion: + 'Remove `await` before the expression. Use caution as this may impact control flow.', }, schema: [ { @@ -68,64 +72,88 @@ export default createRule({ scopeInfoStack.pop(); } - function inTry(node: ts.Node): boolean { - let ancestor = node.parent as ts.Node | undefined; - - while (ancestor && !ts.isFunctionLike(ancestor)) { - if (ts.isTryStatement(ancestor)) { - return true; - } - - ancestor = ancestor.parent; + /** + * Tests whether a node is inside of an explicit error handling context + * (try/catch/finally) in a way that throwing an exception will have an + * impact on the program's control flow. + */ + function affectsExplicitErrorHandling(node: ts.Node): boolean { + // If an error-handling block is followed by another error-handling block, + // control flow is affected by whether promises in it are awaited or not. + // Otherwise, we need to check recursively for nested try statements until + // we get to the top level of a function or the program. If by then, + // there's no offending error-handling blocks, it doesn't affect control + // flow. + const tryAncestorResult = findContainingTryStatement(node); + if (tryAncestorResult == null) { + return false; } - return false; - } - - function inCatch(node: ts.Node): boolean { - let ancestor = node.parent as ts.Node | undefined; + const { tryStatement, block } = tryAncestorResult; - while (ancestor && !ts.isFunctionLike(ancestor)) { - if (ts.isCatchClause(ancestor)) { + switch (block) { + case 'try': + // Try blocks are always followed by either a catch or finally, + // so exceptions thrown here always affect control flow. return true; - } - - ancestor = ancestor.parent; - } - - return false; - } - - function isReturnPromiseInFinally(node: ts.Node): boolean { - let ancestor = node.parent as ts.Node | undefined; + case 'catch': + // Exceptions thrown in catch blocks followed by a finally block affect + // control flow. + if (tryStatement.finallyBlock != null) { + return true; + } - while (ancestor && !ts.isFunctionLike(ancestor)) { - if ( - ts.isTryStatement(ancestor.parent) && - ts.isBlock(ancestor) && - ancestor.parent.end === ancestor.end - ) { - return true; + // Otherwise recurse. + return affectsExplicitErrorHandling(tryStatement); + case 'finally': + return affectsExplicitErrorHandling(tryStatement); + default: { + const __never: never = block; + throw new Error(`Unexpected block type: ${String(__never)}`); } - ancestor = ancestor.parent; } + } - return false; + interface FindContainingTryStatementResult { + tryStatement: ts.TryStatement; + block: 'try' | 'catch' | 'finally'; } - function hasFinallyBlock(node: ts.Node): boolean { + /** + * A try _statement_ is the whole thing that encompasses try block, + * catch clause, and finally block. This function finds the nearest + * enclosing try statement (if present) for a given node, and reports which + * part of the try statement the node is in. + */ + function findContainingTryStatement( + node: ts.Node, + ): FindContainingTryStatementResult | undefined { + let child = node; let ancestor = node.parent as ts.Node | undefined; while (ancestor && !ts.isFunctionLike(ancestor)) { if (ts.isTryStatement(ancestor)) { - return !!ancestor.finallyBlock; + let block: 'try' | 'catch' | 'finally'; + if (child === ancestor.tryBlock) { + block = 'try'; + } else if (child === ancestor.catchClause) { + block = 'catch'; + } else if (child === ancestor.finallyBlock) { + block = 'finally'; + } else { + throw new Error( + 'Child of a try statement must be a try block, catch clause, or finally block', + ); + } + + return { tryStatement: ancestor, block }; } + child = ancestor; ancestor = ancestor.parent; } - return false; - } - // function findTokensToRemove() + return undefined; + } function removeAwait( fixer: TSESLint.RuleFixer, @@ -222,13 +250,29 @@ export default createRule({ return; } + const affectsErrorHandling = affectsExplicitErrorHandling(expression); + const useAutoFix = !affectsErrorHandling; + if (option === 'always') { if (!isAwait && isThenable) { + const fix = ( + fixer: TSESLint.RuleFixer, + ): TSESLint.RuleFix | TSESLint.RuleFix[] => + insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)); + context.report({ messageId: 'requiredPromiseAwait', node, - fix: fixer => - insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)), + ...(useAutoFix + ? { fix } + : { + suggest: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + fix, + }, + ], + }), }); } @@ -237,10 +281,21 @@ export default createRule({ if (option === 'never') { if (isAwait) { + const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null => + removeAwait(fixer, node); context.report({ messageId: 'disallowedPromiseAwait', node, - fix: fixer => removeAwait(fixer, node), + ...(useAutoFix + ? { fix } + : { + suggest: [ + { + messageId: 'disallowedPromiseAwaitSuggestion', + fix, + }, + ], + }), }); } @@ -248,27 +303,41 @@ export default createRule({ } if (option === 'in-try-catch') { - const isInTryCatch = inTry(expression) || inCatch(expression); - if (isAwait && !isInTryCatch) { + if (isAwait && !affectsErrorHandling) { + const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null => + removeAwait(fixer, node); context.report({ messageId: 'disallowedPromiseAwait', node, - fix: fixer => removeAwait(fixer, node), + ...(useAutoFix + ? { fix } + : { + suggest: [ + { + messageId: 'disallowedPromiseAwaitSuggestion', + fix, + }, + ], + }), }); - } else if (!isAwait && isInTryCatch) { - if (inCatch(expression) && !hasFinallyBlock(expression)) { - return; - } - - if (isReturnPromiseInFinally(expression)) { - return; - } - + } else if (!isAwait && affectsErrorHandling) { + const fix = ( + fixer: TSESLint.RuleFixer, + ): TSESLint.RuleFix | TSESLint.RuleFix[] => + insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)); context.report({ messageId: 'requiredPromiseAwait', node, - fix: fixer => - insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)), + ...(useAutoFix + ? { fix } + : { + suggest: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + fix, + }, + ], + }), }); } diff --git a/packages/eslint-plugin/tests/rules/return-await.test.ts b/packages/eslint-plugin/tests/rules/return-await.test.ts index a26481d20881..4a8349031096 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -235,6 +235,7 @@ ruleTester.run('return-await', rule, { } `, }, + // https://github.com/typescript-eslint/typescript-eslint/issues/8663 { code: ` async function test() { @@ -245,11 +246,53 @@ ruleTester.run('return-await', rule, { } return await nested(); } catch (error) { - return await Promise.resolve('error'); + return Promise.resolve('error'); } } `, }, + { + code: ` +async function f() { + try { + } catch { + try { + } catch { + return Promise.reject(); + } + } +} + `, + }, + { + code: ` +async function f() { + try { + } finally { + try { + } catch { + return Promise.reject(); + } + } +} + `, + }, + { + code: ` +async function f() { + try { + } finally { + try { + } finally { + try { + } catch { + return Promise.reject(); + } + } + } +} + `, + }, ], invalid: [ { @@ -404,25 +447,47 @@ async function test() { } } `, - output: ` + output: null, + errors: [ + { + line: 4, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { return await Promise.resolve(1); } catch (e) { - return await Promise.resolve(2); + return Promise.resolve(2); } finally { console.log('cleanup'); } } `, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', + }, + ], }, { 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'); + } + } + `, + }, + ], }, ], }, @@ -498,25 +563,47 @@ async function test() { } } `, - output: ` + output: null, + errors: [ + { + line: 4, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { return await Promise.resolve(1); } catch (e) { - return await Promise.resolve(2); + return Promise.resolve(2); } finally { console.log('cleanup'); } } `, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', + }, + ], }, { 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'); + } + } + `, + }, + ], }, ], }, @@ -571,25 +658,47 @@ async function test() { } } `, - output: ` + output: null, + errors: [ + { + line: 4, + messageId: 'disallowedPromiseAwait', + suggestions: [ + { + messageId: 'disallowedPromiseAwaitSuggestion', + output: ` async function test() { try { return Promise.resolve(1); } catch (e) { - return Promise.resolve(2); + return await Promise.resolve(2); } finally { console.log('cleanup'); } } `, - errors: [ - { - line: 4, - messageId: 'disallowedPromiseAwait', + }, + ], }, { line: 6, messageId: 'disallowedPromiseAwait', + suggestions: [ + { + messageId: 'disallowedPromiseAwaitSuggestion', + output: ` + async function test() { + try { + return await Promise.resolve(1); + } catch (e) { + return Promise.resolve(2); + } finally { + console.log('cleanup'); + } + } + `, + }, + ], }, ], }, @@ -644,25 +753,47 @@ async function test() { } } `, - output: ` + output: null, + errors: [ + { + line: 4, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { return await Promise.resolve(1); } catch (e) { - return await Promise.resolve(2); + return Promise.resolve(2); } finally { console.log('cleanup'); } } `, - errors: [ - { - line: 4, - messageId: 'requiredPromiseAwait', + }, + ], }, { 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'); + } + } + `, + }, + ], }, ], }, @@ -859,7 +990,15 @@ async function test(): Promise { } } `, - output: ` + output: null, + errors: [ + { + line: 5, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test(): Promise { const res = await fetch('...'); try { @@ -869,10 +1008,8 @@ async function test(): Promise { } } `, - errors: [ - { - line: 5, - messageId: 'requiredPromiseAwait', + }, + ], }, ], }, @@ -892,7 +1029,15 @@ async function test(): Promise { } } `, - output: ` + output: null, + errors: [ + { + line: 10, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function test() { try { const callback1 = function () {}; @@ -907,10 +1052,8 @@ async function test(): Promise { } } `, - errors: [ - { - line: 10, - messageId: 'requiredPromiseAwait', + }, + ], }, ], }, @@ -923,7 +1066,15 @@ async function test(): Promise { } catch {} } `, - output: ` + output: null, + errors: [ + { + line: 5, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function bar() {} async function foo() { try { @@ -931,10 +1082,8 @@ async function test(): Promise { } catch {} } `, - errors: [ - { - line: 5, - messageId: 'requiredPromiseAwait', + }, + ], }, ], }, @@ -947,7 +1096,15 @@ async function test(): Promise { } catch {} } `, - output: ` + output: null, + errors: [ + { + line: 5, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` async function bar() {} async function foo() { try { @@ -955,68 +1112,102 @@ async function test(): Promise { } catch {} } `, + }, + ], + }, + ], + }, + { + code: ` +async function bar() {} +async function func1() { + try { + return null ?? bar(); + } catch {} +} + `, + output: null, errors: [ { line: 5, messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +async function bar() {} +async function func1() { + try { + return await (null ?? bar()); + } catch {} +} + `, + }, + ], }, ], }, { code: ` - async function bar() {} - async function func1() { - try { - return null ?? bar(); - } catch {} - } - async function func2() { - try { - return 1 && bar(); - } catch {} - } - const foo = { - bar: async function () {}, - }; - async function func3() { - try { - return foo.bar(); - } catch {} - } - `, - output: ` - async function bar() {} - async function func1() { - try { - return await (null ?? bar()); - } catch {} - } - async function func2() { - try { - return await (1 && bar()); - } catch {} - } - const foo = { - bar: async function () {}, - }; - async function func3() { - try { - return await foo.bar(); - } catch {} - } +async function bar() {} +async function func2() { + try { + return 1 && bar(); + } catch {} +} `, + output: null, errors: [ { line: 5, messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +async function bar() {} +async function func2() { + try { + return await (1 && bar()); + } catch {} +} + `, + }, + ], }, + ], + }, + { + code: ` +const foo = { + bar: async function () {}, +}; +async function func3() { + try { + return foo.bar(); + } catch {} +} + `, + output: null, + errors: [ { - line: 10, - messageId: 'requiredPromiseAwait', - }, - { - line: 18, + line: 7, messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +const foo = { + bar: async function () {}, +}; +async function func3() { + try { + return await foo.bar(); + } catch {} +} + `, + }, + ], }, ], }, @@ -1033,7 +1224,15 @@ async function test(): Promise { } } `, - output: ` + output: null, + errors: [ + { + line: 8, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` class X { async bar() { return; @@ -1044,11 +1243,159 @@ async function test(): Promise { } catch {} } } + `, + }, + ], + }, + ], + }, + { + code: ` + async function test() { + const res = await Promise.resolve('{}'); + try { + async function nested() { + return Promise.resolve('ok'); + } + return await nested(); + } catch (error) { + return await Promise.resolve('error'); + } + } `, errors: [ { - line: 8, + line: 10, + messageId: 'disallowedPromiseAwait', + }, + ], + output: ` + async function test() { + const res = await Promise.resolve('{}'); + try { + async function nested() { + return Promise.resolve('ok'); + } + return await nested(); + } catch (error) { + return Promise.resolve('error'); + } + } + `, + }, + { + code: ` +async function f() { + try { + try { + } finally { + // affects error handling of outer catch + return Promise.reject(); + } + } catch {} +} + `, + output: null, + errors: [ + { + line: 7, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +async function f() { + try { + try { + } finally { + // affects error handling of outer catch + return await Promise.reject(); + } + } catch {} +} + `, + }, + ], + }, + ], + }, + { + code: ` +async function f() { + try { + try { + } catch { + // affects error handling of outer catch + return Promise.reject(); + } + } catch {} +} + `, + output: null, + errors: [ + { + line: 7, messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +async function f() { + try { + try { + } catch { + // affects error handling of outer catch + return await Promise.reject(); + } + } catch {} +} + `, + }, + ], + }, + ], + }, + { + code: ` +async function f() { + try { + } catch { + try { + } finally { + try { + } catch { + return Promise.reject(); + } + } + } finally { + } +} + `, + output: null, + errors: [ + { + line: 9, + messageId: 'requiredPromiseAwait', + suggestions: [ + { + messageId: 'requiredPromiseAwaitSuggestion', + output: ` +async function f() { + try { + } catch { + try { + } finally { + try { + } catch { + return await Promise.reject(); + } + } + } finally { + } +} + `, + }, + ], }, ], }, From 189a6b45c6d35e36fb5166ac85756943597e41e8 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Tue, 30 Apr 2024 02:06:48 -0600 Subject: [PATCH 2/5] spell --- .cspell.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.cspell.json b/.cspell.json index dc41dd9403fb..a87cc3363c0c 100644 --- a/.cspell.json +++ b/.cspell.json @@ -49,8 +49,8 @@ "words": [ "Airbnb", "Airbnb's", - "ambiently", "allowdefaultprojectforfiles", + "ambiently", "Armano", "astexplorer", "Astro", @@ -147,6 +147,7 @@ "tsvfs", "typedef", "typedefs", + "unawaited", "unfixable", "unoptimized", "unprefixed", From 2abfe6a1a5efa8519de78388339c27d99297b174 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Tue, 30 Apr 2024 02:10:01 -0600 Subject: [PATCH 3/5] snapshot --- .../return-await.shot | 47 ++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) 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 bc0d087ad505..d6b583edbe7f 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 @@ -6,25 +6,37 @@ Options: "in-try-catch" async function invalidInTryCatch1() { try { - return Promise.resolve('try'); - ~~~~~~~~~~~~~~~~~~~~~~ Returning an awaited promise is required in this context. - } catch (e) {} + return Promise.reject('try'); + ~~~~~~~~~~~~~~~~~~~~~ Returning an awaited promise is required in this context. + } catch (e) { + // Doesn't execute due to missing await. + } } async function invalidInTryCatch2() { try { throw new Error('error'); } catch (e) { - return await Promise.resolve('catch'); + // Unnecessary await; rejections here don't impact control flow. + return await Promise.reject('catch'); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Returning an awaited promise is not allowed in this context. } } +// Prints 'starting async work', 'cleanup', 'async work done'. async function invalidInTryCatch3() { + async function doAsyncWork(): Promise { + console.log('starting async work'); + await new Promise(resolve => setTimeout(resolve, 1000)); + console.log('async work done'); + } + try { throw new Error('error'); } catch (e) { - return Promise.resolve('catch'); - ~~~~~~~~~~~~~~~~~~~~~~~~ Returning an awaited promise is required in this context. + // Missing await. + return doAsyncWork(); + ~~~~~~~~~~~~~ Returning an awaited promise is required in this context. } finally { console.log('cleanup'); } @@ -36,7 +48,9 @@ async function invalidInTryCatch4() { } catch (e) { throw new Error('error2'); } finally { - return await Promise.resolve('finally'); + // Unnecessary await; rejections here don't impact control flow. + return await Promise.reject('finally'); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Returning an awaited promise is not allowed in this context. } } @@ -58,23 +72,32 @@ Options: "in-try-catch" async function validInTryCatch1() { try { - return await Promise.resolve('try'); - } catch (e) {} + return await Promise.reject('try'); + } catch (e) { + // Executes as expected. + } } async function validInTryCatch2() { try { throw new Error('error'); } catch (e) { - return Promise.resolve('catch'); + return Promise.reject('catch'); } } +// Prints 'starting async work', 'async work done', 'cleanup'. async function validInTryCatch3() { + async function doAsyncWork(): Promise { + console.log('starting async work'); + await new Promise(resolve => setTimeout(resolve, 1000)); + console.log('async work done'); + } + try { throw new Error('error'); } catch (e) { - return await Promise.resolve('catch'); + return await doAsyncWork(); } finally { console.log('cleanup'); } @@ -86,7 +109,7 @@ async function validInTryCatch4() { } catch (e) { throw new Error('error2'); } finally { - return Promise.resolve('finally'); + return Promise.reject('finally'); } } From 1777ac99590a28dee9003430803d8bbdbcc65a7d Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Thu, 2 May 2024 22:39:41 -0600 Subject: [PATCH 4/5] nicer diff --- .../eslint-plugin/src/rules/return-await.ts | 104 +++++++----------- 1 file changed, 39 insertions(+), 65 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index b0b8d7bc382e..d84b79370c44 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -230,22 +230,14 @@ export default createRule({ if (isAwait && !isThenable) { // any/unknown could be thenable; do not auto-fix const useAutoFix = !(isTypeAnyType(type) || isTypeUnknownType(type)); - const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null => - removeAwait(fixer, node); context.report({ messageId: 'nonPromiseAwait', node, - ...(useAutoFix - ? { fix } - : { - suggest: [ - { - messageId: 'nonPromiseAwait', - fix, - }, - ], - }), + ...fixOrSuggest(useAutoFix, { + messageId: 'nonPromiseAwait', + fix: fixer => removeAwait(fixer, node), + }), }); return; } @@ -255,24 +247,18 @@ export default createRule({ if (option === 'always') { if (!isAwait && isThenable) { - const fix = ( - fixer: TSESLint.RuleFixer, - ): TSESLint.RuleFix | TSESLint.RuleFix[] => - insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)); - context.report({ messageId: 'requiredPromiseAwait', node, - ...(useAutoFix - ? { fix } - : { - suggest: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - fix, - }, - ], - }), + ...fixOrSuggest(useAutoFix, { + messageId: 'requiredPromiseAwaitSuggestion', + fix: fixer => + insertAwait( + fixer, + node, + isHigherPrecedenceThanAwait(expression), + ), + }), }); } @@ -281,21 +267,13 @@ export default createRule({ if (option === 'never') { if (isAwait) { - const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null => - removeAwait(fixer, node); context.report({ messageId: 'disallowedPromiseAwait', node, - ...(useAutoFix - ? { fix } - : { - suggest: [ - { - messageId: 'disallowedPromiseAwaitSuggestion', - fix, - }, - ], - }), + ...fixOrSuggest(useAutoFix, { + messageId: 'disallowedPromiseAwaitSuggestion', + fix: fixer => removeAwait(fixer, node), + }), }); } @@ -304,40 +282,27 @@ export default createRule({ if (option === 'in-try-catch') { if (isAwait && !affectsErrorHandling) { - const fix = (fixer: TSESLint.RuleFixer): TSESLint.RuleFix | null => - removeAwait(fixer, node); context.report({ messageId: 'disallowedPromiseAwait', node, - ...(useAutoFix - ? { fix } - : { - suggest: [ - { - messageId: 'disallowedPromiseAwaitSuggestion', - fix, - }, - ], - }), + ...fixOrSuggest(useAutoFix, { + messageId: 'disallowedPromiseAwaitSuggestion', + fix: fixer => removeAwait(fixer, node), + }), }); } else if (!isAwait && affectsErrorHandling) { - const fix = ( - fixer: TSESLint.RuleFixer, - ): TSESLint.RuleFix | TSESLint.RuleFix[] => - insertAwait(fixer, node, isHigherPrecedenceThanAwait(expression)); context.report({ messageId: 'requiredPromiseAwait', node, - ...(useAutoFix - ? { fix } - : { - suggest: [ - { - messageId: 'requiredPromiseAwaitSuggestion', - fix, - }, - ], - }), + ...fixOrSuggest(useAutoFix, { + messageId: 'requiredPromiseAwaitSuggestion', + fix: fixer => + insertAwait( + fixer, + node, + isHigherPrecedenceThanAwait(expression), + ), + }), }); } @@ -390,3 +355,12 @@ export default createRule({ }; }, }); + +function fixOrSuggest( + useFix: boolean, + suggestion: TSESLint.SuggestionReportDescriptor, +): + | { fix: TSESLint.ReportFixFunction } + | { suggest: TSESLint.SuggestionReportDescriptor[] } { + return useFix ? { fix: suggestion.fix } : { suggest: [suggestion] }; +} From 3bda3a55b861e9d446ce1caa39ff3b4986b84314 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 26 May 2024 16:45:08 -0700 Subject: [PATCH 5/5] adjust unreachable case --- packages/eslint-plugin/src/rules/return-await.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index d84b79370c44..98264e704789 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -10,6 +10,7 @@ import { isAwaitKeyword, isTypeAnyType, isTypeUnknownType, + nullThrows, } from '../util'; import { getOperatorPrecedence } from '../util/getOperatorPrecedence'; @@ -133,20 +134,22 @@ export default createRule({ while (ancestor && !ts.isFunctionLike(ancestor)) { if (ts.isTryStatement(ancestor)) { - let block: 'try' | 'catch' | 'finally'; + let block: 'try' | 'catch' | 'finally' | undefined; if (child === ancestor.tryBlock) { block = 'try'; } else if (child === ancestor.catchClause) { block = 'catch'; } else if (child === ancestor.finallyBlock) { block = 'finally'; - } else { - throw new Error( - 'Child of a try statement must be a try block, catch clause, or finally block', - ); } - return { tryStatement: ancestor, block }; + return { + tryStatement: ancestor, + block: nullThrows( + block, + 'Child of a try statement must be a try block, catch clause, or finally block', + ), + }; } child = ancestor; ancestor = ancestor.parent;