return-undefined: various bug fixes (#3298)

palantir · Oct 20, 2017 · cc3d19d · cc3d19d
1 parent f1e0de2
commit cc3d19d
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 19 deletions.
diff --git a/src/language/utils.ts b/src/language/utils.ts
@@ -93,11 +93,13 @@ export function someAncestor(node: ts.Node, predicate: (n: ts.Node) => boolean):
     return predicate(node) || (node.parent !== undefined && someAncestor(node.parent, predicate));
 }
 
-export function ancestorWhere<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => boolean): ts.Node | undefined {
+export function ancestorWhere<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => n is T): T | undefined;
+export function ancestorWhere(node: ts.Node, predicate: (n: ts.Node) => boolean): ts.Node | undefined;
+export function ancestorWhere<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => n is T): T | undefined {
     let cur: ts.Node | undefined = node;
     do {
         if (predicate(cur)) {
-            return cur as T;
+            return cur;
         }
         cur = cur.parent;
     } while (cur !== undefined);

diff --git a/src/rules/returnUndefinedRule.ts b/src/rules/returnUndefinedRule.ts
@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { isIdentifier, isReturnStatement, isUnionType } from "tsutils";
+import { isIdentifier, isReturnStatement, isTypeReference, isUnionType } from "tsutils";
 import * as ts from "typescript";
 
 import * as Lint from "../index";
@@ -58,7 +58,7 @@ function walk(ctx: Lint.WalkContext<void>, checker: ts.TypeChecker) {
             return;
         }
 
-        const functionReturningFrom = Lint.ancestorWhere(node, isFunctionLike) as FunctionLike | undefined;
+        const functionReturningFrom = Lint.ancestorWhere(node, isFunctionLike);
         if (functionReturningFrom === undefined) {
             // Return outside of function is invalid
             return;
@@ -108,24 +108,32 @@ function getReturnKind(node: FunctionLike, checker: ts.TypeChecker): ReturnKind
             return ReturnKind.Value;
     }
 
-    const contextual = isFunctionExpressionLike(node) ? tryGetReturnType(checker.getContextualType(node), checker) : undefined;
+    const contextual = isFunctionExpressionLike(node) && node.type === undefined
+        ? tryGetReturnType(checker.getContextualType(node), checker)
+        : undefined;
     const returnType = contextual !== undefined ? contextual : tryGetReturnType(checker.getTypeAtLocation(node), checker);
 
-    if (returnType === undefined) {
+    if (returnType === undefined || Lint.isTypeFlagSet(returnType, ts.TypeFlags.Any)) {
         return undefined;
-    } else if (isEffectivelyVoid(returnType)) {
+    }
+    if ((Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword) ? isEffectivelyVoidPromise : isEffectivelyVoid)(returnType)) {
         return ReturnKind.Void;
-    } else if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword)) {
-        // Would need access to `checker.getPromisedTypeOfPromise` to do this properly.
-        // Assume that the return type is the global Promise (since this is an async function) and get its type argument.
-        const typeArguments = (returnType as ts.GenericType).typeArguments;
-        if (typeArguments !== undefined && typeArguments.length === 1) {
-            return isEffectivelyVoid(typeArguments[0]) ? ReturnKind.Void : ReturnKind.Value;
-        }
     }
     return ReturnKind.Value;
 }
 
+/** True for `void`, `undefined`, Promise<void>, or `void | undefined | Promise<void>`. */
+function isEffectivelyVoidPromise(type: ts.Type): boolean {
+    // Would need access to `checker.getPromisedTypeOfPromise` to do this properly.
+    // Assume that the return type is the global Promise (since this is an async function) and get its type argument.
+
+    // tslint:disable-next-line no-bitwise
+    return Lint.isTypeFlagSet(type, ts.TypeFlags.Void | ts.TypeFlags.Undefined) ||
+        isUnionType(type) && type.types.every(isEffectivelyVoidPromise) ||
+        isTypeReference(type) && type.typeArguments !== undefined && type.typeArguments.length === 1 &&
+            isEffectivelyVoidPromise(type.typeArguments[0]);
+}
+
 /** True for `void`, `undefined`, or `void | undefined`. */
 function isEffectivelyVoid(type: ts.Type): boolean {
     // tslint:disable-next-line no-bitwise
@@ -143,8 +151,7 @@ function tryGetReturnType(fnType: ts.Type | undefined, checker: ts.TypeChecker):
         return undefined;
     }
 
-    const ret = checker.getReturnTypeOfSignature(sigs[0]);
-    return Lint.isTypeFlagSet(ret, ts.TypeFlags.Any) ? undefined : ret;
+    return checker.getReturnTypeOfSignature(sigs[0]);
 }
 
 function isFunctionLike(node: ts.Node): node is FunctionLike {

diff --git a/test/rules/return-undefined/test.ts.lint b/test/rules/return-undefined/test.ts.lint
@@ -36,17 +36,19 @@ badContextualType(() => {
          ~~~~~~~ [UNDEFINED]
 });
 
-// 'any' doesn't provide a contextual type, so assumes meant to return 'void'.
+// Allow anything in callback taking 'any'.
 function takesAnyCb(cb: () => any): void;
 takesAnyCb(() => { return; });
 takesAnyCb(() => { return undefined; });
-                   ~~~~~~~~~~~~~~~~~ [VOID]
 takesAnyCb(() => {
     if (1 === 2) return;
-                 ~~~~~~~ [UNDEFINED]
     else return 1;
 });
 takesAnyCb(() => {
+    if (1 === 2) return;
+    else return undefined;
+});
+takesAnyCb((): void => {
     if (1 === 2) return;
     else return undefined;
          ~~~~~~~~~~~~~~~~~ [VOID]
@@ -68,5 +70,13 @@ declare function f<T>(action: () => T | undefined): void;
 f(() => { return undefined; });
           ~~~~~~~~~~~~~~~~~ [VOID]
 
+declare function test(cb: () => Promise<void> | void): void;
+
+test(async () => {
+    if (1 === 2) return;
+    else return undefined;
+         ~~~~~~~~~~~~~~~~~ [VOID]
+});
+
 [VOID]: `void` function should use `return;`, not `return undefined;`.
 [UNDEFINED]: Value-returning function should use `return undefined;`, not just `return;`.