feat(eslint-plugin): [promise-function-async] check for promises in implicit return types (#6330)

* [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

* Refrain from renaming the type-util, instead add an optional fourth param.

Author: ericbf

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,15 @@ const arrowFunctionReturnsPromise = async () => Promise.resolve('value');
 async function functionReturnsPromise() {
   return Promise.resolve('value');
 }
+
+// An explicit return type that is not Promise means this function cannot be made async, so it is ignored by the rule
+function functionReturnsUnionWithPromiseExplicitly(
+  p: boolean,
+): string | Promise<string> {
+  return p ? 'value' : Promise.resolve('value');
+}
+
+async function functionReturnsUnionWithPromiseImplicitly(p: boolean) {
+  return p ? 'value' : Promise.resolve('value');
+}
 ```

packages/eslint-plugin/src/rules/promise-function-async.ts

@@ -115,6 +115,8 @@ export default util.createRule<Options, MessageIds>({
           returnType,
           allowAny!,
           allAllowedPromiseNames,
+          // If no return type is explicitly set, we check if any parts of the return type match a Promise (instead of requiring all to match).
+          node.returnType == null,
         )
       ) {
         // Return type is not a promise

packages/eslint-plugin/tests/rules/promise-function-async.test.ts

@@ -165,6 +165,23 @@ abstract class Test {
 }
       `,
     },
+    `
+function promiseInUnionWithExplicitReturnType(
+  p: boolean,
+): Promise<number> | number {
+  return p ? Promise.resolve(5) : 5;
+}
+    `,
+    `
+function explicitReturnWithPromiseInUnion(): Promise<number> | number {
+  return 5;
+}
+    `,
+    `
+async function asyncFunctionReturningUnion(p: boolean) {
+  return p ? Promise.resolve(5) : 5;
+}
+    `,
   ],
   invalid: [
     {
@@ -752,5 +769,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;
+}
+      `,
+    },
   ],
 });

packages/type-utils/src/containsAllTypesByName.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 matchAnyInstead Whether to instead just check if any parts match, rather than all parts.
+ * @returns Whether the type is, extends, or contains the allowed names (or all matches the allowed names, if mustMatchAll is true).
  */
 export function containsAllTypesByName(
   type: ts.Type,
   allowAny: boolean,
   allowedNames: Set<string>,
+  matchAnyInstead = false,
 ): 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 =>
+    containsAllTypesByName(t, allowAny, allowedNames, matchAnyInstead);
+
   if (isUnionOrIntersectionType(type)) {
-    return type.types.every(t =>
-      containsAllTypesByName(t, allowAny, allowedNames),
-    );
+    return matchAnyInstead
+      ? type.types.some(predicate)
+      : type.types.every(predicate);
   }
 
   const bases = type.getBaseTypes();
+
   return (
     typeof bases !== 'undefined' &&
-    bases.length > 0 &&
-    bases.every(t => containsAllTypesByName(t, allowAny, allowedNames))
+    (matchAnyInstead
+      ? bases.some(predicate)
+      : bases.length > 0 && bases.every(predicate))
   );
 }