Simplify the way that async function returns are type-checked so that…

… the error messages become more clear.

The newly introduced check models the legal return types from async code as a union type (which they essentially are) so that a subtype test is sufficient to verify them. There is a risk that synthesizing union types like these could lead to very verbose error messages, but it's thought that the error case being clarified (see bug) is more common.

This CL also tightens the requirements on async function to return no kind of union, including unions with `null` or `undefined`. This was required to make the return type validation correct, but also deemed more accurate. Nullable unions were initially supported as a compromise but on consideration seemed not particularly valuable.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219873146

src/com/google/javascript/jscomp/Promises.java

@@ -16,6 +16,7 @@
 
 package com.google.javascript.jscomp;
 
+
 import com.google.javascript.rhino.jstype.JSType;
 import com.google.javascript.rhino.jstype.JSTypeNative;
 import com.google.javascript.rhino.jstype.JSTypeRegistry;
@@ -96,4 +97,36 @@ static final JSType getResolvedType(JSTypeRegistry registry, JSType type) {
 
     return type;
   }
+
+  /**
+   * Synthesizes a type representing the legal types of a return expression within async code
+   * (i.e.`Promise` callbacks, async functions).
+   *
+   * <p>The return type will generally be a union but may not be, for example:
+   *
+   * <ul>
+   *   <li>`!Promise<number>` => `number|!IThenable<number>`
+   *   <li>`number` => `number|!IThenable<number>`
+   *   <li>`?` => `?`
+   *   <li>`*` => `*`
+   *   <li>`number|!Foo` => `number|!Foo|!IThenable<number|!Foo>`
+   *   <li>`!Foo|!IThenable<!Foo>` => `Foo|!IThenable<!Foo>`
+   *   <li>`!Promise<!IThenable<!Foo>>` => `!Foo|!IThenable<!Foo>`
+   * </ul>
+   *
+   * Note that this method may create an incorrect (but not really dangerous) type when supplied
+   * with types that are nonsensical in an async context, for example:
+   *
+   * <ul>
+   *   <li>`number|!IThenable<string>` => `number|string|!IThenable<number|string>`
+   *   <li>`?IThenable<!Foo>` => `null|!Foo|!Thenable<?Foo>`
+   * </ul>
+   */
+  static final JSType createAsyncReturnableType(JSTypeRegistry registry, JSType maybeThenable) {
+    JSType parameterType = getResolvedType(registry, maybeThenable);
+    return registry.createUnionType(
+        parameterType,
+        registry.createTemplatizedType(
+            registry.getNativeObjectType(JSTypeNative.I_THENABLE_TYPE), parameterType));
+  }
 }

src/com/google/javascript/jscomp/TypeCheck.java

@@ -2135,7 +2135,7 @@ private void visitFunction(NodeTraversal t, Node n) {
     } else if (n.isAsyncFunction()) {
       // An async function must return a Promise or supertype of Promise
       JSType returnType = functionType.getReturnType();
-      validator.expectValidAsyncReturnType(t, n, returnType.restrictByNotNullOrUndefined());
+      validator.expectValidAsyncReturnType(t, n, returnType);
     }
   }
 
@@ -2481,14 +2481,11 @@ private void visitImplicitReturnExpression(NodeTraversal t, Node exprNode) {
         expectedReturnType = getNativeType(VOID_TYPE);
       } else if (enclosingFunction.isAsyncFunction()) {
         // Unwrap the async function's declared return type.
-        expectedReturnType = Promises.getTemplateTypeOfThenable(typeRegistry, expectedReturnType);
+        expectedReturnType = Promises.createAsyncReturnableType(typeRegistry, expectedReturnType);
       }
 
       // Fetch the returned value's type
       JSType actualReturnType = getJSType(exprNode);
-      if (enclosingFunction.isAsyncFunction()) {
-        actualReturnType = Promises.getResolvedType(typeRegistry, actualReturnType);
-      }
 
       validator.expectCanAssignTo(t, exprNode, actualReturnType, expectedReturnType,
           "inconsistent return type");
@@ -2526,9 +2523,10 @@ private void visitReturn(NodeTraversal t, Node n) {
         // e.g. if returnType is "Generator<string>", make it just "string".
         returnType = getTemplateTypeOfGenerator(returnType);
       } else if (enclosingFunction.isAsyncFunction()) {
-        // Unwrap the template variable from a async function's declared return type.
-        // e.g. if returnType is "!Promise<string>" or "!IThenable<string>", make it just "string".
-        returnType = Promises.getTemplateTypeOfThenable(typeRegistry, returnType);
+        // e.g. `!Promise<string>` => `string|!IThenable<string>`
+        // We transform the expected return type rather than the actual return type so that the
+        // extual return type is always reported to the user. This was felt to be clearer.
+        returnType = Promises.createAsyncReturnableType(typeRegistry, returnType);
       } else if (returnType.isVoidType() && functionType.isConstructor()) {
         // Allow constructors to use empty returns for flow control.
         if (!n.hasChildren()) {
@@ -2547,11 +2545,6 @@ private void visitReturn(NodeTraversal t, Node n) {
         valueNode = n;
       } else {
         actualReturnType = getJSType(valueNode);
-        if (enclosingFunction.isAsyncFunction()) {
-          // We want to treat `return Promise.resolve(1);` as if it were `return 1;` inside an async
-          // function.
-          actualReturnType = Promises.getResolvedType(typeRegistry, actualReturnType);
-        }
       }
 
       // verifying

test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java

@@ -213,12 +213,12 @@ public void testAsyncArrowWithCorrectBlocklessReturn() {
   public void testAsyncArrowWithIncorrectBlocklessReturn() {
     testTypes(
         lines(
-            "function takesPromiseProvider(/** function(): ?Promise<string> */ getPromise) {}",
+            "function takesPromiseProvider(/** function(): !Promise<string> */ getPromise) {}",
             "takesPromiseProvider(async () => 1);"),
         lines(
             "inconsistent return type", // preserve newline
             "found   : number",
-            "required: string"));
+            "required: (IThenable<string>|string)"));
   }
 
   @Test
@@ -4093,22 +4093,20 @@ public void testAsyncReturnsPromise2() {
         lines(
             "inconsistent return type", // preserve newline
             "found   : number",
-            "required: string"));
+            "required: (IThenable<string>|string)"));
   }
 
   @Test
-  public void testAsyncCanReturnNullablePromise() {
-    // TODO(lharker): don't allow async functions to return null.
+  public void testAsyncFunctionCannotReturnNullablePromise() {
     testTypesWithCommonExterns(
         lines(
             "/** @return {?Promise<string>} */",
             "async function getAString() {",
-            "  return 1;",
+            "  return '';",
             "}"),
         lines(
-            "inconsistent return type", // preserve newline
-            "found   : number",
-            "required: string"));
+            "The return type of an async function must be a non-union supertype of Promise",
+            "found: (Promise<string>|null)"));
   }
 
   @Test
@@ -4170,7 +4168,7 @@ public void testAsyncCanReturnIThenable1() {
         lines(
             "inconsistent return type", //
             "found   : number",
-            "required: string"));
+            "required: (IThenable<string>|string)"));
   }
 
   @Test
@@ -4185,8 +4183,8 @@ public void testAsyncReturnStatementIsResolved() {
             "}"),
         lines(
             "inconsistent return type", // preserve newline
-            "found   : number",
-            "required: string"));
+            "found   : IThenable<number>",
+            "required: (IThenable<string>|string)"));
   }
 
   @Test