From 5857356962060b19aa792bee778f9167ee54154b Mon Sep 17 00:00:00 2001 From: kirkwaiblinger <53019676+kirkwaiblinger@users.noreply.github.com> Date: Fri, 29 Dec 2023 11:42:35 -0700 Subject: [PATCH] feat(eslint-plugin): [no-floating-promises] flag result of .map(async) (#7897) * flag arrays of promises * remove comment * trying to trigger CI * Substantial simplification * add some grody test cases for generics * flip if/else * add detail to test case * switch to isArrayLikeType * Revert "switch to isArrayLikeType" This reverts commit 2afe79401108b9b0429c06a787bf9e7af0c0f1b4. * add checks for tuples * One more test case! * else if * Remove unnecessary whitespace * Remove invalid test case * Add to docs * Add test cases around array that's fine but tuple that isn't * tweaks --- .../docs/rules/no-floating-promises.md | 15 ++ .../src/rules/no-floating-promises.ts | 75 ++++++-- .../tests/rules/no-floating-promises.test.ts | 161 +++++++++++++++++- 3 files changed, 238 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-floating-promises.md b/packages/eslint-plugin/docs/rules/no-floating-promises.md index c08216cd794..e137a85aaf9 100644 --- a/packages/eslint-plugin/docs/rules/no-floating-promises.md +++ b/packages/eslint-plugin/docs/rules/no-floating-promises.md @@ -17,6 +17,13 @@ Valid ways of handling a Promise-valued statement include: - Calling its `.then()` with two arguments - Calling its `.catch()` with one argument +This rule also reports when an Array containing Promises is created and not properly handled. The main way to resolve this is by using one of the Promise concurrency methods to create a single Promise, then handling that according to the procedure above. These methods include: + +- `Promise.all()`, +- `Promise.allSettled()`, +- `Promise.any()` +- `Promise.race()` + :::tip `no-floating-promises` only detects unhandled Promise _statements_. See [`no-misused-promises`](./no-misused-promises.md) for detecting code that provides Promises to _logical_ locations such as if statements. @@ -40,6 +47,8 @@ returnsPromise().then(() => {}); Promise.reject('value').catch(); Promise.reject('value').finally(); + +[1, 2, 3].map(async x => x + 1); ``` ### ✅ Correct @@ -59,6 +68,8 @@ returnsPromise().then( Promise.reject('value').catch(() => {}); await Promise.reject('value').finally(() => {}); + +await Promise.all([1, 2, 3].map(async x => x + 1)); ``` ## Options @@ -106,3 +117,7 @@ You might consider using `void`s and/or [ESLint disable comments](https://eslint ## Related To - [`no-misused-promises`](./no-misused-promises.md) + +## Further Reading + +- ["Using Promises" MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises). Note especially the sections on [Promise rejection events](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises#promise_rejection_events) and [Composition](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises#composition). diff --git a/packages/eslint-plugin/src/rules/no-floating-promises.ts b/packages/eslint-plugin/src/rules/no-floating-promises.ts index 9c1e9f785c1..b3ac6529699 100644 --- a/packages/eslint-plugin/src/rules/no-floating-promises.ts +++ b/packages/eslint-plugin/src/rules/no-floating-promises.ts @@ -23,7 +23,9 @@ type MessageId = | 'floatingUselessRejectionHandler' | 'floatingUselessRejectionHandlerVoid' | 'floatingFixAwait' - | 'floatingFixVoid'; + | 'floatingFixVoid' + | 'floatingPromiseArray' + | 'floatingPromiseArrayVoid'; const messageBase = 'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.'; @@ -35,6 +37,13 @@ const messageBaseVoid = const messageRejectionHandler = 'A rejection handler that is not a function will be ignored.'; +const messagePromiseArray = + "An array of Promises may be unintentional. Consider handling the promises' fulfillment or rejection with Promise.all or similar."; + +const messagePromiseArrayVoid = + "An array of Promises may be unintentional. Consider handling the promises' fulfillment or rejection with Promise.all or similar," + + ' or explicitly marking the expression as ignored with the `void` operator.'; + export default createRule({ name: 'no-floating-promises', meta: { @@ -54,6 +63,8 @@ export default createRule({ messageBase + ' ' + messageRejectionHandler, floatingUselessRejectionHandlerVoid: messageBaseVoid + ' ' + messageRejectionHandler, + floatingPromiseArray: messagePromiseArray, + floatingPromiseArrayVoid: messagePromiseArrayVoid, }, schema: [ { @@ -97,13 +108,18 @@ export default createRule({ expression = expression.expression; } - const { isUnhandled, nonFunctionHandler } = isUnhandledPromise( - checker, - expression, - ); + const { isUnhandled, nonFunctionHandler, promiseArray } = + isUnhandledPromise(checker, expression); if (isUnhandled) { - if (options.ignoreVoid) { + if (promiseArray) { + context.report({ + node, + messageId: options.ignoreVoid + ? 'floatingPromiseArrayVoid' + : 'floatingPromiseArray', + }); + } else if (options.ignoreVoid) { context.report({ node, messageId: nonFunctionHandler @@ -205,7 +221,11 @@ export default createRule({ function isUnhandledPromise( checker: ts.TypeChecker, node: TSESTree.Node, - ): { isUnhandled: boolean; nonFunctionHandler?: boolean } { + ): { + isUnhandled: boolean; + nonFunctionHandler?: boolean; + promiseArray?: 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 @@ -228,8 +248,16 @@ export default createRule({ return isUnhandledPromise(checker, node.argument); } + const tsNode = services.esTreeNodeToTSNodeMap.get(node); + // Check the type. At this point it can't be unhandled if it isn't a promise - if (!isPromiseLike(checker, services.esTreeNodeToTSNodeMap.get(node))) { + // or array thereof. + + if (isPromiseArray(checker, tsNode)) { + return { isUnhandled: true, promiseArray: true }; + } + + if (!isPromiseLike(checker, tsNode)) { return { isUnhandled: false }; } @@ -295,12 +323,39 @@ export default createRule({ }, }); +function isPromiseArray(checker: ts.TypeChecker, node: ts.Node): boolean { + const type = checker.getTypeAtLocation(node); + for (const ty of tsutils + .unionTypeParts(type) + .map(t => checker.getApparentType(t))) { + if (checker.isArrayType(ty)) { + const arrayType = checker.getTypeArguments(ty)[0]; + if (isPromiseLike(checker, node, arrayType)) { + return true; + } + } + + if (checker.isTupleType(ty)) { + for (const tupleElementType of checker.getTypeArguments(ty)) { + if (isPromiseLike(checker, node, tupleElementType)) { + return true; + } + } + } + } + return false; +} + // Modified from tsutils.isThenable() to only consider thenables which can be // rejected/caught via a second parameter. Original source (MIT licensed): // // https://github.com/ajafff/tsutils/blob/49d0d31050b44b81e918eae4fbaf1dfe7b7286af/util/type.ts#L95-L125 -function isPromiseLike(checker: ts.TypeChecker, node: ts.Node): boolean { - const type = checker.getTypeAtLocation(node); +function isPromiseLike( + checker: ts.TypeChecker, + node: ts.Node, + type?: ts.Type, +): boolean { + type ??= checker.getTypeAtLocation(node); for (const ty of tsutils.unionTypeParts(checker.getApparentType(type))) { const then = ty.getProperty('then'); if (then === 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 c89e4316dd9..434c9735a46 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -222,7 +222,7 @@ async function test() { ` async function test() { class Thenable { - then(callback: () => {}): Thenable { + then(callback: () => void): Thenable { return new Thenable(); } } @@ -264,7 +264,7 @@ async function test() { ` async function test() { class CatchableThenable { - then(callback: () => {}, callback: () => {}): CatchableThenable { + then(callback: () => void, callback: () => void): CatchableThenable { return new CatchableThenable(); } } @@ -475,6 +475,35 @@ Promise.reject() .finally(() => {}); `, }, + { + code: ` +await Promise.all([Promise.resolve(), Promise.resolve()]); + `, + }, + { + code: ` +declare const promiseArray: Array>; +void promiseArray; + `, + }, + { + code: ` +[Promise.reject(), Promise.reject()].then(() => {}); + `, + }, + { + // Expressions aren't checked by this rule, so this just becomes an array + // of number | undefined, which is fine regardless of the ignoreVoid setting. + code: ` +[1, 2, void Promise.reject(), 3]; + `, + options: [{ ignoreVoid: false }], + }, + { + code: ` +['I', 'am', 'just', 'an', 'array']; + `, + }, ], invalid: [ @@ -997,7 +1026,7 @@ async function test() { code: ` async function test() { class CatchableThenable { - then(callback: () => {}, callback: () => {}): CatchableThenable { + then(callback: () => void, callback: () => void): CatchableThenable { return new CatchableThenable(); } } @@ -1651,5 +1680,131 @@ Promise.reject(new Error('message')).finally(() => {}); `, errors: [{ line: 2, messageId: 'floatingVoid' }], }, + { + code: ` +function _>>( + maybePromiseArray: S | undefined, +): void { + maybePromiseArray?.[0]; +} + `, + errors: [{ line: 5, messageId: 'floatingVoid' }], + }, + { + code: ` +[1, 2, 3].map(() => Promise.reject()); + `, + errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +declare const array: unknown[]; +array.map(() => Promise.reject()); + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +declare const promiseArray: Array>; +void promiseArray; + `, + options: [{ ignoreVoid: false }], + errors: [{ line: 3, messageId: 'floatingPromiseArray' }], + }, + { + code: ` +[1, 2, Promise.reject(), 3]; + `, + errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +[1, 2, Promise.reject().catch(() => {}), 3]; + `, + errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +const data = ['test']; +data.map(async () => { + await new Promise((_res, rej) => setTimeout(rej, 1000)); +}); + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +function _>>>( + maybePromiseArrayArray: S | undefined, +): void { + maybePromiseArrayArray?.[0]; +} + `, + errors: [{ line: 5, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +function f>>(a: T): void { + a; +} + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +declare const a: Array> | undefined; +a; + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +function f>>(a: T | undefined): void { + a; +} + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +[Promise.reject()] as const; + `, + errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +declare function cursed(): [Promise, Promise]; +cursed(); + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` +[ + 'Type Argument number ', + 1, + 'is not', + Promise.resolve(), + 'but it still is flagged', +] as const; + `, + errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` + declare const arrayOrPromiseTuple: + | Array + | [number, number, Promise, string]; + arrayOrPromiseTuple; + `, + errors: [{ line: 5, messageId: 'floatingPromiseArrayVoid' }], + }, + { + code: ` + declare const okArrayOrPromiseArray: Array | Array>; + okArrayOrPromiseArray; + `, + errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }], + }, ], });