From 606a52cefcecd594df6edc359bff291b835169f2 Mon Sep 17 00:00:00 2001 From: kirkwaiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Sat, 15 Jul 2023 16:51:33 -0700 Subject: [PATCH] fix(eslint-plugin): [no-floating-promises] false negative calling .then with second argument undefined (#6881) * fix(eslint-plugin): [no-floating-promises] False negative calling .then with second argument undefined (Issue #6850) * use getTypeAtLocation correctly * clean up duplicate * correct mistake in IIFE abbreviation * purge the word invocated from the codebase --- .../docs/rules/no-floating-promises.md | 2 +- .../src/rules/no-floating-promises.ts | 142 ++++++++++---- .../tests/rules/no-floating-promises.test.ts | 174 +++++++++++++++++- .../no-floating-promises.shot | 4 +- 4 files changed, 274 insertions(+), 48 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-floating-promises.md b/packages/eslint-plugin/docs/rules/no-floating-promises.md index 289e42f903c..246a50be096 100644 --- a/packages/eslint-plugin/docs/rules/no-floating-promises.md +++ b/packages/eslint-plugin/docs/rules/no-floating-promises.md @@ -83,7 +83,7 @@ With this option set to `true`, and if you are using `no-void`, you should turn ### `ignoreIIFE` -This allows you to skip checking of async IIFEs (Immediately Invocated function Expressions). +This allows you to skip checking of async IIFEs (Immediately Invoked function Expressions). Examples of **correct** code for this rule with `{ ignoreIIFE: true }`: diff --git a/packages/eslint-plugin/src/rules/no-floating-promises.ts b/packages/eslint-plugin/src/rules/no-floating-promises.ts index 00c86a09c11..40ea3254093 100644 --- a/packages/eslint-plugin/src/rules/no-floating-promises.ts +++ b/packages/eslint-plugin/src/rules/no-floating-promises.ts @@ -15,9 +15,21 @@ type Options = [ type MessageId = | 'floating' + | 'floatingVoid' + | 'floatingUselessRejectionHandler' + | 'floatingUselessRejectionHandlerVoid' | 'floatingFixAwait' - | 'floatingFixVoid' - | 'floatingVoid'; + | 'floatingFixVoid'; + +const messageBase = + 'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.'; + +const messageBaseVoid = + 'Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler' + + ' or be explicitly marked as ignored with the `void` operator.'; + +const messageRejectionHandler = + 'A rejection handler that is not a function will be ignored.'; export default util.createRule({ name: 'no-floating-promises', @@ -30,13 +42,14 @@ export default util.createRule({ }, hasSuggestions: true, messages: { - floating: - 'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.', + floating: messageBase, floatingFixAwait: 'Add await operator.', - floatingVoid: - 'Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler' + - ' or be explicitly marked as ignored with the `void` operator.', + floatingVoid: messageBaseVoid, floatingFixVoid: 'Add void operator to ignore.', + floatingUselessRejectionHandler: + messageBase + ' ' + messageRejectionHandler, + floatingUselessRejectionHandlerVoid: + messageBaseVoid + ' ' + messageRejectionHandler, }, schema: [ { @@ -48,7 +61,7 @@ export default util.createRule({ }, ignoreIIFE: { description: - 'Whether to ignore async IIFEs (Immediately Invocated Function Expressions).', + 'Whether to ignore async IIFEs (Immediately Invoked Function Expressions).', type: 'boolean', }, }, @@ -80,11 +93,18 @@ export default util.createRule({ expression = expression.expression; } - if (isUnhandledPromise(checker, expression)) { + const { isUnhandled, nonFunctionHandler } = isUnhandledPromise( + checker, + expression, + ); + + if (isUnhandled) { if (options.ignoreVoid) { context.report({ node, - messageId: 'floatingVoid', + messageId: nonFunctionHandler + ? 'floatingUselessRejectionHandlerVoid' + : 'floatingVoid', suggest: [ { messageId: 'floatingFixVoid', @@ -110,7 +130,9 @@ export default util.createRule({ } else { context.report({ node, - messageId: 'floating', + messageId: nonFunctionHandler + ? 'floatingUselessRejectionHandler' + : 'floating', suggest: [ { messageId: 'floatingFixAwait', @@ -168,16 +190,31 @@ export default util.createRule({ ); } + function isValidRejectionHandler(rejectionHandler: TSESTree.Node): boolean { + return ( + services.program + .getTypeChecker() + .getTypeAtLocation( + services.esTreeNodeToTSNodeMap.get(rejectionHandler), + ) + .getCallSignatures().length > 0 + ); + } + function isUnhandledPromise( checker: ts.TypeChecker, node: TSESTree.Node, - ): boolean { + ): { isUnhandled: boolean; nonFunctionHandler?: boolean } { // First, check expressions whose resulting types may not be promise-like if (node.type === AST_NODE_TYPES.SequenceExpression) { // Any child in a comma expression could return a potentially unhandled // promise, so we check them all regardless of whether the final returned // value is promise-like. - return node.expressions.some(item => isUnhandledPromise(checker, item)); + return ( + node.expressions + .map(item => isUnhandledPromise(checker, item)) + .find(result => result.isUnhandled) ?? { isUnhandled: false } + ); } if ( @@ -192,24 +229,45 @@ export default util.createRule({ // Check the type. At this point it can't be unhandled if it isn't a promise if (!isPromiseLike(checker, services.esTreeNodeToTSNodeMap.get(node))) { - return false; + return { isUnhandled: false }; } 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. - return ( - !isPromiseCatchCallWithHandler(node) && - !isPromiseThenCallWithRejectionHandler(node) && - !isPromiseFinallyCallWithHandler(node) - ); + + const catchRejectionHandler = getRejectionHandlerFromCatchCall(node); + if (catchRejectionHandler) { + if (isValidRejectionHandler(catchRejectionHandler)) { + return { isUnhandled: false }; + } else { + return { isUnhandled: true, nonFunctionHandler: true }; + } + } + + const thenRejectionHandler = getRejectionHandlerFromThenCall(node); + if (thenRejectionHandler) { + if (isValidRejectionHandler(thenRejectionHandler)) { + return { isUnhandled: false }; + } else { + return { isUnhandled: true, nonFunctionHandler: true }; + } + } + + if (isPromiseFinallyCallWithHandler(node)) { + return { isUnhandled: false }; + } + + 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 // ternary. Check them directly. - return ( - isUnhandledPromise(checker, node.alternate) || - isUnhandledPromise(checker, node.consequent) - ); + const alternateResult = isUnhandledPromise(checker, node.alternate); + if (alternateResult.isUnhandled) { + return alternateResult; + } else { + return isUnhandledPromise(checker, node.consequent); + } } else if ( node.type === AST_NODE_TYPES.MemberExpression || node.type === AST_NODE_TYPES.Identifier || @@ -218,18 +276,20 @@ export default util.createRule({ // If it is just a property access chain or a `new` call (e.g. `foo.bar` or // `new Promise()`), the promise is not handled because it doesn't have the // necessary then/catch call at the end of the chain. - return true; + return { isUnhandled: true }; } else if (node.type === AST_NODE_TYPES.LogicalExpression) { - return ( - isUnhandledPromise(checker, node.left) || - isUnhandledPromise(checker, node.right) - ); + const leftResult = isUnhandledPromise(checker, node.left); + if (leftResult.isUnhandled) { + return leftResult; + } else { + return isUnhandledPromise(checker, node.right); + } } // We conservatively return false for all other types of expressions because // we don't want to accidentally fail if the promise is handled internally but // we just can't tell. - return false; + return { isUnhandled: false }; } }, }); @@ -291,26 +351,34 @@ function isFunctionParam( return false; } -function isPromiseCatchCallWithHandler( +function getRejectionHandlerFromCatchCall( expression: TSESTree.CallExpression, -): boolean { - return ( +): TSESTree.CallExpressionArgument | undefined { + if ( expression.callee.type === AST_NODE_TYPES.MemberExpression && expression.callee.property.type === AST_NODE_TYPES.Identifier && expression.callee.property.name === 'catch' && expression.arguments.length >= 1 - ); + ) { + return expression.arguments[0]; + } else { + return undefined; + } } -function isPromiseThenCallWithRejectionHandler( +function getRejectionHandlerFromThenCall( expression: TSESTree.CallExpression, -): boolean { - return ( +): TSESTree.CallExpressionArgument | undefined { + if ( expression.callee.type === AST_NODE_TYPES.MemberExpression && expression.callee.property.type === AST_NODE_TYPES.Identifier && expression.callee.property.name === 'then' && expression.arguments.length >= 2 - ); + ) { + return expression.arguments[1]; + } else { + return undefined; + } } function isPromiseFinallyCallWithHandler( 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 8ddf9aef5b9..a8701d74b6e 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -147,7 +147,7 @@ async function test() { `, ` async function test() { - const promiseValue: Promise; + declare const promiseValue: Promise; await promiseValue; promiseValue.then( @@ -166,7 +166,7 @@ async function test() { `, ` async function test() { - const promiseUnion: Promise | number; + declare const promiseUnion: Promise | number; await promiseUnion; promiseUnion.then( @@ -185,7 +185,7 @@ async function test() { `, ` async function test() { - const promiseIntersection: Promise & number; + declare const promiseIntersection: Promise & number; await promiseIntersection; promiseIntersection.then( @@ -228,7 +228,7 @@ async function test() { await (Math.random() > 0.5 ? foo : 0); await (Math.random() > 0.5 ? bar : 0); - const intersectionPromise: Promise & number; + declare const intersectionPromise: Promise & number; await intersectionPromise; } `, @@ -458,6 +458,13 @@ async function foo() { `, options: [{ ignoreVoid: false }], }, + { + code: ` +declare const definitelyCallable: () => void; +Promise.reject().catch(definitelyCallable); + `, + options: [{ ignoreVoid: false }], + }, ], invalid: [ @@ -883,7 +890,7 @@ async function test() { { code: ` async function test() { - const promiseValue: Promise; + declare const promiseValue: Promise; promiseValue; promiseValue.then(() => {}); @@ -913,7 +920,7 @@ async function test() { { code: ` async function test() { - const promiseUnion: Promise | number; + declare const promiseUnion: Promise | number; promiseUnion; } @@ -928,7 +935,7 @@ async function test() { { code: ` async function test() { - const promiseIntersection: Promise & number; + declare const promiseIntersection: Promise & number; promiseIntersection; promiseIntersection.then(() => {}); @@ -1143,7 +1150,7 @@ async function test() { { code: ` (async function () { - const promiseIntersection: Promise & number; + declare const promiseIntersection: Promise & number; promiseIntersection; promiseIntersection.then(() => {}); promiseIntersection.catch(); @@ -1429,5 +1436,156 @@ async function foo() { }, ], }, + { + code: ` +declare const maybeCallable: string | (() => void); +declare const definitelyCallable: () => void; +Promise.resolve().then(() => {}, undefined); +Promise.resolve().then(() => {}, null); +Promise.resolve().then(() => {}, 3); +Promise.resolve().then(() => {}, maybeCallable); +Promise.resolve().then(() => {}, definitelyCallable); + +Promise.resolve().catch(undefined); +Promise.resolve().catch(null); +Promise.resolve().catch(3); +Promise.resolve().catch(maybeCallable); +Promise.resolve().catch(definitelyCallable); + `, + errors: [ + { + line: 4, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 5, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 6, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 7, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 10, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 11, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 12, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + { + line: 13, + messageId: 'floatingUselessRejectionHandlerVoid', + }, + ], + }, + { + code: ` +Promise.reject() || 3; + `, + errors: [ + { + line: 2, + messageId: 'floatingVoid', + }, + ], + }, + { + code: ` +void Promise.resolve().then(() => {}, undefined); + `, + options: [{ ignoreVoid: false }], + errors: [ + { + line: 2, + messageId: 'floatingUselessRejectionHandler', + }, + ], + }, + { + code: ` +declare const maybeCallable: string | (() => void); +Promise.resolve().then(() => {}, maybeCallable); + `, + options: [{ ignoreVoid: false }], + errors: [ + { + line: 3, + messageId: 'floatingUselessRejectionHandler', + }, + ], + }, + { + code: ` +declare const maybeCallable: string | (() => void); +declare const definitelyCallable: () => void; +Promise.resolve().then(() => {}, undefined); +Promise.resolve().then(() => {}, null); +Promise.resolve().then(() => {}, 3); +Promise.resolve().then(() => {}, maybeCallable); +Promise.resolve().then(() => {}, definitelyCallable); + +Promise.resolve().catch(undefined); +Promise.resolve().catch(null); +Promise.resolve().catch(3); +Promise.resolve().catch(maybeCallable); +Promise.resolve().catch(definitelyCallable); + `, + options: [{ ignoreVoid: false }], + errors: [ + { + line: 4, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 5, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 6, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 7, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 10, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 11, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 12, + messageId: 'floatingUselessRejectionHandler', + }, + { + line: 13, + messageId: 'floatingUselessRejectionHandler', + }, + ], + }, + { + code: ` +Promise.reject() || 3; + `, + options: [{ ignoreVoid: false }], + errors: [ + { + line: 2, + messageId: 'floating', + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/no-floating-promises.shot b/packages/eslint-plugin/tests/schema-snapshots/no-floating-promises.shot index 1218cb2d329..ac6669f651b 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/no-floating-promises.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/no-floating-promises.shot @@ -9,7 +9,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos "additionalProperties": false, "properties": { "ignoreIIFE": { - "description": "Whether to ignore async IIFEs (Immediately Invocated Function Expressions).", + "description": "Whether to ignore async IIFEs (Immediately Invoked Function Expressions).", "type": "boolean" }, "ignoreVoid": { @@ -26,7 +26,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { - /** Whether to ignore async IIFEs (Immediately Invocated Function Expressions). */ + /** Whether to ignore async IIFEs (Immediately Invoked Function Expressions). */ ignoreIIFE?: boolean; /** Whether to ignore \`void\` expressions. */ ignoreVoid?: boolean;