-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Complain when a non-void function lacks a return expresson. #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d33127a
b7343c2
69018e6
1728f7c
0f4e887
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3932,12 +3932,10 @@ module ts { | |
} | ||
|
||
// Aggregate the types of expressions within all the return statements. | ||
var types: Type[] = []; | ||
checkAndAggregateReturnExpressionTypes(func.body); | ||
var types = checkAndAggregateReturnExpressionTypes(<Block>func.body, contextualType, contextualMapper); | ||
|
||
// Try to return the best common type if we have any return expressions. | ||
if (types.length) { | ||
|
||
if (types.length > 0) { | ||
var commonType = getBestCommonType(types, /*contextualType:*/ undefined, /*candidatesOnly:*/ true); | ||
if (!commonType) { | ||
error(func, Diagnostics.No_best_common_type_exists_among_return_expressions); | ||
|
@@ -3963,16 +3961,18 @@ module ts { | |
} | ||
|
||
return voidType; | ||
} | ||
|
||
// WARNING: This has the same semantics as the forEach family of functions, | ||
// in that traversal terminates in the event that 'visitor' supplies a truthy value. | ||
function forEachReturnStatement<T>(body: Block, visitor: (stmt: ReturnStatement) => T): T { | ||
|
||
return traverse(body); | ||
|
||
function checkAndAggregateReturnExpressionTypes(node: Node) { | ||
function traverse(node: Node): T { | ||
switch (node.kind) { | ||
case SyntaxKind.ReturnStatement: | ||
var expr = (<ReturnStatement>node).expression; | ||
if (expr) { | ||
var type = checkAndMarkExpression(expr, contextualType, contextualMapper); | ||
if (!contains(types, type)) types.push(type); | ||
} | ||
break; | ||
return visitor(node); | ||
case SyntaxKind.Block: | ||
case SyntaxKind.FunctionBlock: | ||
case SyntaxKind.IfStatement: | ||
|
@@ -3989,15 +3989,77 @@ module ts { | |
case SyntaxKind.TryBlock: | ||
case SyntaxKind.CatchBlock: | ||
case SyntaxKind.FinallyBlock: | ||
forEachChild(node, checkAndAggregateReturnExpressionTypes); | ||
break; | ||
return forEachChild(node, traverse); | ||
} | ||
} | ||
} | ||
|
||
/// Returns a set of types relating to every return expression relating to a function block. | ||
function checkAndAggregateReturnExpressionTypes(body: Block, contextualType?: Type, contextualMapper?: TypeMapper): Type[] { | ||
var aggregatedTypes: Type[] = []; | ||
|
||
forEachReturnStatement(body, (returnStatement) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no parens around (returnStatement) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0e10fc7. |
||
var expr = returnStatement.expression; | ||
if (expr) { | ||
var type = checkAndMarkExpression(expr, contextualType, contextualMapper); | ||
if (!contains(aggregatedTypes, type)) { | ||
aggregatedTypes.push(type); | ||
} | ||
} | ||
}); | ||
|
||
return aggregatedTypes; | ||
} | ||
|
||
function bodyContainsAReturnStatement(funcBody: Block) { | ||
return forEachReturnStatement(funcBody, (returnStatement) => { | ||
return true; | ||
}); | ||
} | ||
|
||
function bodyContainsSingleThrowStatement(body: Block) { | ||
return (body.statements.length === 1) && (body.statements[0].kind === SyntaxKind.ThrowStatement) | ||
} | ||
|
||
// TypeScript Specification 1.0 (6.3) - July 2014 | ||
// An explicitly typed function whose return type isn't the Void or the Any type | ||
// must have at least one return statement somewhere in its body. | ||
// An exception to this rule is if the function implementation consists of a single 'throw' statement. | ||
function checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(func: FunctionDeclaration, returnType: Type): void { | ||
// Functions that return 'void' or 'any' don't need any return expressions. | ||
if (returnType === voidType || returnType === anyType) { | ||
return; | ||
} | ||
|
||
// If all we have is a function signature, or an arrow function with an expression body, then there is nothing to check. | ||
if (!func.body || func.body.kind !== SyntaxKind.FunctionBlock) { | ||
return; | ||
} | ||
|
||
var bodyBlock = <Block>func.body; | ||
|
||
// Ensure the body has at least one return expression. | ||
if (bodyContainsAReturnStatement(bodyBlock)) { | ||
return; | ||
} | ||
|
||
// If there are no return expressions, then we need to check if | ||
// the function body consists solely of a throw statement; | ||
// this is to make an exception for unimplemented functions. | ||
if (bodyContainsSingleThrowStatement(bodyBlock)) { | ||
return; | ||
} | ||
|
||
// This function does not conform to the specification. | ||
error(func.type, Diagnostics.A_function_whose_declared_type_is_neither_void_nor_any_must_return_a_value_or_consist_of_a_single_throw_statement); | ||
} | ||
|
||
function checkFunctionExpression(node: FunctionExpression, contextualType?: Type, contextualMapper?: TypeMapper): Type { | ||
// The identityMapper object is used to indicate that function expressions are wildcards | ||
if (contextualMapper === identityMapper) return anyFunctionType; | ||
if (contextualMapper === identityMapper) { | ||
return anyFunctionType; | ||
} | ||
|
||
var type = getTypeOfSymbol(node.symbol); | ||
var links = getNodeLinks(node); | ||
|
||
|
@@ -4015,6 +4077,9 @@ module ts { | |
signature.resolvedReturnType = returnType; | ||
} | ||
} | ||
else { | ||
checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type)); | ||
} | ||
} | ||
checkSignatureDeclaration(node); | ||
if (node.body.kind === SyntaxKind.FunctionBlock) { | ||
|
@@ -4587,28 +4652,9 @@ module ts { | |
} | ||
|
||
function checkAccessorDeclaration(node: AccessorDeclaration) { | ||
function checkGetterContainsSingleThrowStatement(node: AccessorDeclaration): boolean { | ||
var block = <Block>node.body; | ||
return block.statements.length === 1 && block.statements[0].kind === SyntaxKind.ThrowStatement; | ||
} | ||
|
||
function checkGetterReturnsValue(n: Node): boolean { | ||
switch (n.kind) { | ||
case SyntaxKind.ReturnStatement: | ||
return true; | ||
// do not dive into function-like things - return statements there don't count | ||
case SyntaxKind.FunctionExpression: | ||
case SyntaxKind.FunctionDeclaration: | ||
case SyntaxKind.ArrowFunction: | ||
case SyntaxKind.ObjectLiteral: | ||
return false; | ||
default: | ||
return forEachChild(n, checkGetterReturnsValue); | ||
} | ||
} | ||
if (node.kind === SyntaxKind.GetAccessor) { | ||
if (!isInAmbientContext(node) && node.body && !(checkGetterContainsSingleThrowStatement(node) || checkGetterReturnsValue(node))) { | ||
error(node.name, Diagnostics.Getters_must_return_a_value); | ||
if (!isInAmbientContext(node) && node.body && !(bodyContainsAReturnStatement(<Block>node.body) || bodyContainsSingleThrowStatement(<Block>node.body))) { | ||
error(node.name, Diagnostics.A_get_accessor_must_return_a_value_or_consist_of_a_single_throw_statement); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we used to support a getter with a single throw statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes; however, we erroneously permitted the following in both the old compiler and the new one prior to this commit. class C {
get foo() {
return;
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add an explanation why this is error? Per TypeScript spec (and ECMA 262) return statement that lacks expression returns the value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, This is crazy world of JavaScript. Also restricting this is a potential breaking change. I'd say that these issues should be traced by some sort of linter tool There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well yes, but doesn't the following also implicitly return class C {
get foo() {
}
} Additionally, the following should be permissible by the same reasoning, meaning that the entire check should be removed. function f(): string {
return;
} I think that the rules for the check should be the same between get accessors and functions with type annotations. If not, we may need to open this up to discussion, because then maybe we don't need the check. Edit: Sorry @vladima, didn't see your last response, and I decided to remove my answer and think the matter over a bit more before posting this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding 'Breaking Change' label to the original issue then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have opened up #162 to prevent this issue from blocking the fix. I'll simply amend this pull request so that it does not cause a breaking change. |
||
} | ||
} | ||
|
||
|
@@ -4837,8 +4883,6 @@ module ts { | |
} | ||
} | ||
} | ||
|
||
// TODO: Check at least one return statement in non-void/any function (except single throw) | ||
} | ||
|
||
function checkFunctionDeclaration(node: FunctionDeclaration) { | ||
|
@@ -4850,7 +4894,11 @@ module ts { | |
if (node === firstDeclaration) { | ||
checkFunctionOrConstructorSymbol(symbol); | ||
} | ||
|
||
checkSourceElement(node.body); | ||
if (node.type) { | ||
checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type)); | ||
} | ||
|
||
// If there is no body and no explicit return type, then report an error. | ||
if (program.getCompilerOptions().noImplicitAny && !node.body && !node.type) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
==== tests/cases/compiler/conflictingTypeAnnotatedVar.ts (2 errors) ==== | ||
==== tests/cases/compiler/conflictingTypeAnnotatedVar.ts (4 errors) ==== | ||
var foo: string; | ||
function foo(): number { } | ||
~~~ | ||
!!! Duplicate identifier 'foo'. | ||
~~~~~~ | ||
!!! A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement. | ||
function foo(): number { } | ||
~~~ | ||
!!! Duplicate identifier 'foo'. | ||
!!! Duplicate identifier 'foo'. | ||
~~~~~~ | ||
!!! A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
==== tests/cases/compiler/errorOnContextuallyTypedReturnType.ts (1 errors) ==== | ||
==== tests/cases/compiler/errorOnContextuallyTypedReturnType.ts (2 errors) ==== | ||
var n1: () => boolean = function () { }; // expect an error here | ||
~~ | ||
!!! Type '() => void' is not assignable to type '() => boolean': | ||
!!! Type 'void' is not assignable to type 'boolean'. | ||
var n2: () => boolean = function ():boolean { }; // expect an error here | ||
~~~~~~~ | ||
!!! A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
==== tests/cases/compiler/functionWithThrowButNoReturn1.ts (1 errors) ==== | ||
function fn(): number { | ||
~~~~~~ | ||
!!! A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement. | ||
throw new Error('NYI'); | ||
var t; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would rather enumerate the cases where we don't go in. But that's just me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've gone back and forth. It's kind of a matter of whitelisting vs blacklisting, so I went with whitelisting because it was feasible.