From 3527630186cf9abc3decb4d5cfb9f30c6ad46ffa Mon Sep 17 00:00:00 2001 From: Eric Ferreira Date: Wed, 11 Jan 2023 19:45:05 -0500 Subject: [PATCH] [promise-function-async] Only allow unions in explicit return types When we return a union containing a promise from a function implicitly, it's often a mistake. This commit makes it so if the return type is explicit, any `Promise` in the return type (whether it's part of a union or not) will flag the function. If it is intentional, make the return type explicit. Fixes #6329 --- .../docs/rules/promise-function-async.md | 18 +++++++++++++- .../src/rules/promise-function-async.ts | 4 +++- .../rules/promise-function-async.test.ts | 24 +++++++++++++++++++ ...lTypesByName.ts => containsTypesByName.ts} | 22 +++++++++++------ packages/type-utils/src/index.ts | 2 +- 5 files changed, 60 insertions(+), 10 deletions(-) rename packages/type-utils/src/{containsAllTypesByName.ts => containsTypesByName.ts} (52%) diff --git a/packages/eslint-plugin/docs/rules/promise-function-async.md b/packages/eslint-plugin/docs/rules/promise-function-async.md index e697217dc5d9..4ca59ac341e0 100644 --- a/packages/eslint-plugin/docs/rules/promise-function-async.md +++ b/packages/eslint-plugin/docs/rules/promise-function-async.md @@ -11,10 +11,12 @@ Ensures that each function is only capable of: - returning a rejected promise, or - throwing an Error object. -In contrast, non-`async` `Promise` - returning functions are technically capable of either. +In contrast, non-`async`, `Promise`-returning functions are technically capable of either. Code that handles the results of those functions will often need to handle both cases, which can get complex. This rule's practice removes a requirement for creating code to handle both cases. +> When functions return unions of `Promise` and non-`Promise` types implicitly, it is usually a mistake—this rule flags those cases. If it is intentional, make the return type explicitly to allow the rule to pass. + ## Examples Examples of code for this rule @@ -29,6 +31,10 @@ const arrowFunctionReturnsPromise = () => Promise.resolve('value'); function functionReturnsPromise() { return Promise.resolve('value'); } + +function functionReturnsUnionWithPromiseImplicitly(p: boolean) { + return p ? 'value' : Promise.resolve('value'); +} ``` ### ✅ Correct @@ -39,4 +45,14 @@ const arrowFunctionReturnsPromise = async () => Promise.resolve('value'); async function functionReturnsPromise() { return Promise.resolve('value'); } + +function functionReturnsUnionWithPromiseExplicitly( + p: boolean, +): string | Promise { + return p ? 'value' : Promise.resolve('value'); +} + +async function functionReturnsUnionWithPromiseImplicitly(p: boolean) { + return p ? 'value' : Promise.resolve('value'); +} ``` diff --git a/packages/eslint-plugin/src/rules/promise-function-async.ts b/packages/eslint-plugin/src/rules/promise-function-async.ts index 4387bc52c9b2..e8b9f44739b8 100644 --- a/packages/eslint-plugin/src/rules/promise-function-async.ts +++ b/packages/eslint-plugin/src/rules/promise-function-async.ts @@ -111,10 +111,12 @@ export default util.createRule({ const returnType = checker.getReturnTypeOfSignature(signatures[0]); if ( - !util.containsAllTypesByName( + !util.containsTypesByName( returnType, allowAny!, allAllowedPromiseNames, + // Only if a return type is explicitly declared must it match all + Boolean(node.returnType), ) ) { // Return type is not a promise diff --git a/packages/eslint-plugin/tests/rules/promise-function-async.test.ts b/packages/eslint-plugin/tests/rules/promise-function-async.test.ts index ccf99b6afcbb..4ae8c4922c85 100644 --- a/packages/eslint-plugin/tests/rules/promise-function-async.test.ts +++ b/packages/eslint-plugin/tests/rules/promise-function-async.test.ts @@ -165,6 +165,13 @@ abstract class Test { } `, }, + ` +function promiseInUnionWithExplicitReturnType( + p: boolean, +): Promise | number { + return p ? Promise.resolve(5) : 5; +} + `, ], invalid: [ { @@ -752,5 +759,22 @@ const foo = { }, ], }, + { + code: ` +function promiseInUnionWithoutExplicitReturnType(p: boolean) { + return p ? Promise.resolve(5) : 5; +} + `, + errors: [ + { + messageId, + }, + ], + output: ` +async function promiseInUnionWithoutExplicitReturnType(p: boolean) { + return p ? Promise.resolve(5) : 5; +} + `, + }, ], }); diff --git a/packages/type-utils/src/containsAllTypesByName.ts b/packages/type-utils/src/containsTypesByName.ts similarity index 52% rename from packages/type-utils/src/containsAllTypesByName.ts rename to packages/type-utils/src/containsTypesByName.ts index 07aa20dac048..2e57984ac283 100644 --- a/packages/type-utils/src/containsAllTypesByName.ts +++ b/packages/type-utils/src/containsTypesByName.ts @@ -5,13 +5,16 @@ import { isTypeFlagSet } from './typeFlagUtils'; /** * @param type Type being checked by name. + * @param allowAny Whether to consider `any` and `unknown` to match. * @param allowedNames Symbol names checking on the type. - * @returns Whether the type is, extends, or contains all of the allowed names. + * @param mustMatchAll Whether all parts have to match, as opposed to any parts matching. + * @returns Whether the type is, extends, or contains the allowed names (or all matches the allowed names, if mustMatchAll is true). */ -export function containsAllTypesByName( +export function containsTypesByName( type: ts.Type, allowAny: boolean, allowedNames: Set, + mustMatchAll: boolean, ): boolean { if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { return !allowAny; @@ -26,16 +29,21 @@ export function containsAllTypesByName( return true; } + const predicate = (t: ts.Type): boolean => + containsTypesByName(t, allowAny, allowedNames, mustMatchAll); + if (isUnionOrIntersectionType(type)) { - return type.types.every(t => - containsAllTypesByName(t, allowAny, allowedNames), - ); + return mustMatchAll + ? type.types.every(predicate) + : type.types.some(predicate); } const bases = type.getBaseTypes(); + return ( typeof bases !== 'undefined' && - bases.length > 0 && - bases.every(t => containsAllTypesByName(t, allowAny, allowedNames)) + (mustMatchAll + ? bases.length > 0 && bases.every(predicate) + : bases.some(predicate)) ); } diff --git a/packages/type-utils/src/index.ts b/packages/type-utils/src/index.ts index dde032e1770c..a350d5c8658e 100644 --- a/packages/type-utils/src/index.ts +++ b/packages/type-utils/src/index.ts @@ -1,4 +1,4 @@ -export * from './containsAllTypesByName'; +export * from './containsTypesByName'; export * from './getConstrainedTypeAtLocation'; export * from './getContextualType'; export * from './getDeclaration';