Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

return-undefined: various bug fixes #3298

Merged
merged 2 commits into from
Oct 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/language/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I like the type guard. do we still need the other non-type-guard function signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it there to avoid breaking custom rules that might use this exported API.

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);
Expand Down
35 changes: 21 additions & 14 deletions src/rules/returnUndefinedRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
16 changes: 13 additions & 3 deletions test/rules/return-undefined/test.ts.lint
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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;`.