From d4c0d43e85b0057bb869525d94e07d3732f94b3a Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Thu, 8 Jun 2023 12:23:40 -0600 Subject: [PATCH 1/4] finally is transparent to rejection --- .../src/rules/no-floating-promises.ts | 24 +-- .../tests/rules/no-floating-promises.test.ts | 138 ++++++++++++++---- 2 files changed, 126 insertions(+), 36 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-floating-promises.ts b/packages/eslint-plugin/src/rules/no-floating-promises.ts index 40ea3254093..3165d0e3a0f 100644 --- a/packages/eslint-plugin/src/rules/no-floating-promises.ts +++ b/packages/eslint-plugin/src/rules/no-floating-promises.ts @@ -233,8 +233,8 @@ export default util.createRule({ } if (node.type === AST_NODE_TYPES.CallExpression) { - // If the outer expression is a call, it must be either a `.then()` or - // `.catch()` that handles the promise. + // If the outer expression is a call, a `.catch()` or `.then()` with + // rejection handler handles the promise. const catchRejectionHandler = getRejectionHandlerFromCatchCall(node); if (catchRejectionHandler) { @@ -254,10 +254,13 @@ export default util.createRule({ } } - if (isPromiseFinallyCallWithHandler(node)) { - return { isUnhandled: false }; + // `x.finally()` is transparent to resolution of the promise, so check `x`. + const promiseFinally = parsePromiseFinallyCall(node); + if (promiseFinally) { + return isUnhandledPromise(checker, promiseFinally.object); } + // All other cases are unhandled. return { isUnhandled: true }; } else if (node.type === AST_NODE_TYPES.ConditionalExpression) { // We must be getting the promise-like value from one of the branches of the @@ -381,13 +384,12 @@ function getRejectionHandlerFromThenCall( } } -function isPromiseFinallyCallWithHandler( +function parsePromiseFinallyCall( expression: TSESTree.CallExpression, -): boolean { - return ( - expression.callee.type === AST_NODE_TYPES.MemberExpression && +): { object: TSESTree.Expression } | undefined { + return expression.callee.type === AST_NODE_TYPES.MemberExpression && expression.callee.property.type === AST_NODE_TYPES.Identifier && - expression.callee.property.name === 'finally' && - expression.arguments.length >= 1 - ); + expression.callee.property.name === 'finally' + ? { object: expression.callee.object } + : undefined; } diff --git a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts index a8701d74b6e..efd5ffc9f4b 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -31,7 +31,6 @@ async function test() { .catch(() => {}) .finally(() => {}); Promise.resolve('value').catch(() => {}); - Promise.resolve('value').finally(() => {}); return Promise.resolve('value'); } `, @@ -58,7 +57,6 @@ async function test() { .catch(() => {}) .finally(() => {}); Promise.reject(new Error('message')).catch(() => {}); - Promise.reject(new Error('message')).finally(() => {}); return Promise.reject(new Error('message')); } `, @@ -77,7 +75,6 @@ async function test() { .catch(() => {}) .finally(() => {}); (async () => true)().catch(() => {}); - (async () => true)().finally(() => {}); return (async () => true)(); } `, @@ -97,7 +94,6 @@ async function test() { .catch(() => {}) .finally(() => {}); returnsPromise().catch(() => {}); - returnsPromise().finally(() => {}); return returnsPromise(); } `, @@ -106,7 +102,6 @@ async function test() { const x = Promise.resolve(); const y = x.then(() => {}); y.catch(() => {}); - y.finally(() => {}); } `, ` @@ -117,7 +112,6 @@ async function test() { ` async function test() { Promise.resolve().catch(() => {}), 123; - Promise.resolve().finally(() => {}), 123; 123, Promise.resolve().then( () => {}, @@ -160,7 +154,6 @@ async function test() { .catch(() => {}) .finally(() => {}); promiseValue.catch(() => {}); - promiseValue.finally(() => {}); return promiseValue; } `, @@ -193,12 +186,8 @@ async function test() { () => {}, ); promiseIntersection.then(() => {}).catch(() => {}); - promiseIntersection - .then(() => {}) - .catch(() => {}) - .finally(() => {}); + promiseIntersection.then(() => {}).catch(() => {}); promiseIntersection.catch(() => {}); - promiseIntersection.finally(() => {}); return promiseIntersection; } `, @@ -218,7 +207,6 @@ async function test() { .catch(() => {}) .finally(() => {}); canThen.catch(() => {}); - canThen.finally(() => {}); return canThen; } `, @@ -315,7 +303,6 @@ async function test() { .catch(() => {}) .finally(() => {}); promise.catch(() => {}); - promise.finally(() => {}); return promise; } `, @@ -333,7 +320,6 @@ async function test() { ?.then(() => {}) ?.catch(() => {}); returnsPromise()?.catch(() => {}); - returnsPromise()?.finally(() => {}); return returnsPromise(); } `, @@ -465,6 +451,22 @@ Promise.reject().catch(definitelyCallable); `, options: [{ ignoreVoid: false }], }, + { + code: ` +Promise.reject() + .catch(() => {}) + .finally(() => {}); +Promise.reject() + .catch(() => {}) + .finally(() => {}) + .finally(() => {}); +Promise.reject() + .catch(() => {}) + .finally(() => {}) + .finally(() => {}) + .finally(() => {}); + `, + }, ], invalid: [ @@ -612,7 +614,6 @@ async function test() { (async () => true)(); (async () => true)().then(() => {}); (async () => true)().catch(); - (async () => true)().finally(); } `, errors: [ @@ -628,10 +629,6 @@ async function test() { line: 5, messageId: 'floatingVoid', }, - { - line: 6, - messageId: 'floatingVoid', - }, ], }, { @@ -940,7 +937,6 @@ async function test() { promiseIntersection; promiseIntersection.then(() => {}); promiseIntersection.catch(); - promiseIntersection.finally(); } `, errors: [ @@ -956,10 +952,6 @@ async function test() { line: 7, messageId: 'floatingVoid', }, - { - line: 8, - messageId: 'floatingVoid', - }, ], }, { @@ -1587,5 +1579,101 @@ Promise.reject() || 3; }, ], }, + { + code: ` +Promise.reject().finally(() => {}); +Promise.reject() + .finally(() => {}) + .finally(() => {}); +Promise.reject() + .finally(() => {}) + .finally(() => {}) + .finally(() => {}); + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + { + line: 3, + messageId: 'floatingVoid', + }, + { + line: 6, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +Promise.reject() + .then(() => {}) + .finally(() => {}); + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +declare const returnsPromise: () => Promise | null; +returnsPromise()?.finally(() => {}); + `, + errors: [ + { + line: 3, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +const promiseIntersection: Promise & number; +promiseIntersection.finally(() => {}); + `, + errors: [ + { + line: 3, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +Promise.resolve().finally(() => {}), 123; + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +(async () => true)().finally(); + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +Promise.reject(new Error('message')).finally(() => {}); + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + ], + }, ], }); From 81e28f75d0ab46311d78716307c65afae5bdc0a4 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 16 Jul 2023 10:12:49 -0700 Subject: [PATCH 2/4] Unwrap object --- .../eslint-plugin/src/rules/no-floating-promises.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-floating-promises.ts b/packages/eslint-plugin/src/rules/no-floating-promises.ts index 3165d0e3a0f..b0a34e03340 100644 --- a/packages/eslint-plugin/src/rules/no-floating-promises.ts +++ b/packages/eslint-plugin/src/rules/no-floating-promises.ts @@ -255,9 +255,10 @@ export default util.createRule({ } // `x.finally()` is transparent to resolution of the promise, so check `x`. - const promiseFinally = parsePromiseFinallyCall(node); - if (promiseFinally) { - return isUnhandledPromise(checker, promiseFinally.object); + // ("object" in this context is the `x` in `x.finally()`) + const promiseFinallyObject = getObjectFromFinallyCall(node); + if (promiseFinallyObject) { + return isUnhandledPromise(checker, promiseFinallyObject); } // All other cases are unhandled. @@ -384,12 +385,12 @@ function getRejectionHandlerFromThenCall( } } -function parsePromiseFinallyCall( +function getObjectFromFinallyCall( expression: TSESTree.CallExpression, -): { object: TSESTree.Expression } | undefined { +): TSESTree.Expression | undefined { return expression.callee.type === AST_NODE_TYPES.MemberExpression && expression.callee.property.type === AST_NODE_TYPES.Identifier && expression.callee.property.name === 'finally' - ? { object: expression.callee.object } + ? expression.callee.object : undefined; } From 8f14f3469053cb47ae5d7c8250a97877c1606bae Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 16 Jul 2023 10:24:30 -0700 Subject: [PATCH 3/4] split up test cases and put ignoreVoid: false on some --- .../tests/rules/no-floating-promises.test.ts | 77 +++++++------------ 1 file changed, 27 insertions(+), 50 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts index efd5ffc9f4b..0d76221de03 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -456,10 +456,19 @@ Promise.reject().catch(definitelyCallable); Promise.reject() .catch(() => {}) .finally(() => {}); + `, + }, + { + code: ` Promise.reject() .catch(() => {}) .finally(() => {}) .finally(() => {}); + `, + options: [{ ignoreVoid: false }], + }, + { + code: ` Promise.reject() .catch(() => {}) .finally(() => {}) @@ -1582,28 +1591,26 @@ Promise.reject() || 3; { code: ` Promise.reject().finally(() => {}); + `, + errors: [{ line: 2, messageId: 'floatingVoid' }], + }, + { + code: ` Promise.reject() .finally(() => {}) .finally(() => {}); + `, + options: [{ ignoreVoid: false }], + errors: [{ line: 2, messageId: 'floating' }], + }, + { + code: ` Promise.reject() .finally(() => {}) .finally(() => {}) .finally(() => {}); `, - errors: [ - { - line: 2, - messageId: 'floatingVoid', - }, - { - line: 3, - messageId: 'floatingVoid', - }, - { - line: 6, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 2, messageId: 'floatingVoid' }], }, { code: ` @@ -1611,69 +1618,39 @@ Promise.reject() .then(() => {}) .finally(() => {}); `, - errors: [ - { - line: 2, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 2, messageId: 'floatingVoid' }], }, { code: ` declare const returnsPromise: () => Promise | null; returnsPromise()?.finally(() => {}); `, - errors: [ - { - line: 3, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 3, messageId: 'floatingVoid' }], }, { code: ` const promiseIntersection: Promise & number; promiseIntersection.finally(() => {}); `, - errors: [ - { - line: 3, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 3, messageId: 'floatingVoid' }], }, { code: ` Promise.resolve().finally(() => {}), 123; `, - errors: [ - { - line: 2, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 2, messageId: 'floatingVoid' }], }, { code: ` (async () => true)().finally(); `, - errors: [ - { - line: 2, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 2, messageId: 'floatingVoid' }], }, { code: ` Promise.reject(new Error('message')).finally(() => {}); `, - errors: [ - { - line: 2, - messageId: 'floatingVoid', - }, - ], + errors: [{ line: 2, messageId: 'floatingVoid' }], }, ], }); From 6354b309c2a70e630b153a7d0817bb0ca34f79b9 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 16 Jul 2023 10:26:21 -0700 Subject: [PATCH 4/4] remove duplicated line --- packages/eslint-plugin/tests/rules/no-floating-promises.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts index 0d76221de03..c89e4316dd9 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -186,7 +186,6 @@ async function test() { () => {}, ); promiseIntersection.then(() => {}).catch(() => {}); - promiseIntersection.then(() => {}).catch(() => {}); promiseIntersection.catch(() => {}); return promiseIntersection; }