From efdd5213ceaef332cf0b2c26573176f844d22a09 Mon Sep 17 00:00:00 2001 From: Soobin Bak Date: Mon, 14 Sep 2020 10:17:48 +0900 Subject: [PATCH] fix(eslint-plugin): [return-await] don't error for `in-try-catch` if the return is in a `catch` without a `finally` (#2356) --- .../eslint-plugin/docs/rules/return-await.md | 66 ++++++++++++++++++- .../eslint-plugin/src/rules/return-await.ts | 58 ++++++++++++++-- .../tests/rules/return-await.test.ts | 26 ++++++++ 3 files changed, 143 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/return-await.md b/packages/eslint-plugin/docs/rules/return-await.md index 1f0adc71ec1..3c621d25104 100644 --- a/packages/eslint-plugin/docs/rules/return-await.md +++ b/packages/eslint-plugin/docs/rules/return-await.md @@ -28,6 +28,12 @@ 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: + +- 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. Examples of **incorrect** code with `in-try-catch`: @@ -39,10 +45,38 @@ async function invalidInTryCatch1() { } async function invalidInTryCatch2() { - return await Promise.resolve('try'); + try { + throw new Error('error'); + } catch (e) { + return await Promise.resolve('catch'); + } } async function invalidInTryCatch3() { + try { + throw new Error('error'); + } catch (e) { + return Promise.resolve('catch'); + } finally { + console.log('cleanup'); + } +} + +async function invalidInTryCatch4() { + try { + throw new Error('error'); + } catch (e) { + throw new Error('error2'); + } finally { + return await Promise.resolve('finally'); + } +} + +async function invalidInTryCatch5() { + return await Promise.resolve('try'); +} + +async function invalidInTryCatch6() { return await 'value'; } ``` @@ -57,10 +91,38 @@ async function validInTryCatch1() { } async function validInTryCatch2() { - return Promise.resolve('try'); + try { + throw new Error('error'); + } catch (e) { + return Promise.resolve('catch'); + } } async function validInTryCatch3() { + try { + throw new Error('error'); + } catch (e) { + return await Promise.resolve('catch'); + } finally { + console.log('cleanup'); + } +} + +async function validInTryCatch4() { + try { + throw new Error('error'); + } catch (e) { + throw new Error('error2'); + } finally { + return Promise.resolve('finally'); + } +} + +async function validInTryCatch5() { + return Promise.resolve('try'); +} + +async function validInTryCatch6() { return 'value'; } ``` diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index 17bc9c0e54e..edbfcb593ed 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -57,23 +57,63 @@ export default util.createRule({ }; } - function inTryCatch(node: ts.Node): boolean { + function inTry(node: ts.Node): boolean { + let ancestor = node.parent; + + while (ancestor && !ts.isFunctionLike(ancestor)) { + if (tsutils.isTryStatement(ancestor)) { + return true; + } + + ancestor = ancestor.parent; + } + + return false; + } + + function inCatch(node: ts.Node): boolean { + let ancestor = node.parent; + + while (ancestor && !ts.isFunctionLike(ancestor)) { + if (tsutils.isCatchClause(ancestor)) { + return true; + } + + ancestor = ancestor.parent; + } + + return false; + } + + function isReturnPromiseInFinally(node: ts.Node): boolean { let ancestor = node.parent; while (ancestor && !ts.isFunctionLike(ancestor)) { if ( - tsutils.isTryStatement(ancestor) || - tsutils.isCatchClause(ancestor) + tsutils.isTryStatement(ancestor.parent) && + tsutils.isBlock(ancestor) && + ancestor.parent.end === ancestor.end ) { return true; } - ancestor = ancestor.parent; } return false; } + function hasFinallyBlock(node: ts.Node): boolean { + let ancestor = node.parent; + + while (ancestor && !ts.isFunctionLike(ancestor)) { + if (tsutils.isTryStatement(ancestor)) { + return !!ancestor.finallyBlock; + } + ancestor = ancestor.parent; + } + return false; + } + // function findTokensToRemove() function removeAwait( @@ -163,7 +203,7 @@ export default util.createRule({ } if (option === 'in-try-catch') { - const isInTryCatch = inTryCatch(expression); + const isInTryCatch = inTry(expression) || inCatch(expression); if (isAwait && !isInTryCatch) { context.report({ messageId: 'disallowedPromiseAwait', @@ -171,6 +211,14 @@ export default util.createRule({ fix: fixer => removeAwait(fixer, node), }); } else if (!isAwait && isInTryCatch) { + if (inCatch(expression) && !hasFinallyBlock(expression)) { + return; + } + + if (isReturnPromiseInFinally(expression)) { + return; + } + context.report({ messageId: 'requiredPromiseAwait', node, diff --git a/packages/eslint-plugin/tests/rules/return-await.test.ts b/packages/eslint-plugin/tests/rules/return-await.test.ts index 671374fc9bd..887768a6106 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -112,6 +112,32 @@ ruleTester.run('return-await', rule, { } `, }, + { + options: ['in-try-catch'], + code: ` + async function test() { + try { + throw 'foo'; + } catch (e) { + return Promise.resolve(1); + } + } + `, + }, + { + options: ['in-try-catch'], + code: ` + async function test() { + try { + throw 'foo'; + } catch (e) { + throw 'foo2'; + } finally { + return Promise.resolve(1); + } + } + `, + }, { options: ['in-try-catch'], code: `