Complain when a non-void function lacks a return expresson. #147

DanielRosenwasser · 2014-07-18T22:27:08Z

In effect this fixes #62

Also
- Changes the error message for get accessors lacking return expressions.
- Actually checks for return expressions instead of return statements for get-accessors.
- Removes fancy quotes.
- Corrects errors in the compiler caught by the new check.
- Simplified checkAndAggregateReturnTypes by extracting it out to visitReturnStatements.

DanielRosenwasser · 2014-07-18T22:31:51Z

tests/baselines/reference/ambientGetters.errors.txt


    declare class A {
        get length() : number;
                             ~
 !!! '{' expected.
+                       ~~~~~~
+!!! A function whose declared type is neither 'void' nor 'any' must have a 'return' expression or consist of a single 'throw' statement.


This seems like an unfortunate consequence of #139.

RyanCavanaugh · 2014-07-18T23:55:44Z

Baselines 🆗

JsonFreeman · 2014-07-19T00:41:07Z

src/compiler/checker.ts

-                if (!isInAmbientContext(node) && node.body && !(checkGetterContainsSingleThrowStatement(node) || checkGetterReturnsValue(node))) {
-                    error(node.name, Diagnostics.Getters_must_return_a_value);
+                if (!isInAmbientContext(node) && node.body && !(bodyContainsReturnExpressions(<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);


Did we used to support a getter with a single throw statement?

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; } }

Can you add an explanation why this is error? Per TypeScript spec (and ECMA 262) return statement that lacks expression returns the value undefined

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

Well yes, but doesn't the following also implicitly return undefined?

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.

Adding 'Breaking Change' label to the original issue then

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.

JsonFreeman · 2014-07-19T00:49:53Z

Looks great!

vladima · 2014-07-19T05:31:28Z

tests/baselines/reference/parserReturnStatement4.errors.txt

@@ -1,4 +1,6 @@
-==== tests/cases/conformance/parser/ecmascript5/Statements/ReturnStatements/parserReturnStatement4.ts (1 errors) ====
+==== tests/cases/conformance/parser/ecmascript5/Statements/ReturnStatements/parserReturnStatement4.ts (2 errors) ====
    var v = { get foo() { return } };


This is legal JavaScript code and should not be an error. Return statement without expression returns undefined

CyrusNajmabadi · 2014-07-21T02:40:18Z

src/compiler/checker.ts

+        function checkAndAggregateReturnExpressionTypes(body: Block, contextualType?: Type, contextualMapper?: TypeMapper): Type[] {
+            var aggregatedTypes: Type[] = [];
+
+            visitReturnStatements(body, (returnStatement) => {


no need for parens around the parameter.

In effect this fixes #62. Also - Changes the error message for get accessors lacking return expressions. - Actually checks for return expressions instead of return statements for get-accessors. - Removes fancy quotes. - Corrects errors in the compiler caught by the new check. - Simplified `checkAndAggregateReturnTypes` by extracting it out to `visitReturnStatements`.

…statements.

mhegazy · 2014-07-22T20:46:15Z

👍

Complain when a non-void function lacks a return expresson.

CyrusNajmabadi · 2014-07-22T23:58:14Z

src/compiler/checker.ts

+        function checkAndAggregateReturnExpressionTypes(body: Block, contextualType?: Type, contextualMapper?: TypeMapper): Type[] {
+            var aggregatedTypes: Type[] = [];
+
+            forEachReturnStatement(body, (returnStatement) => {


no parens around (returnStatement)

Fixed in 0e10fc7.

As per feedback in pull request #147.

DanielRosenwasser changed the title ~~Complain when a non-void/non-any function lacks a return expresson.~~ Complain when a non-void function lacks a return expresson. Jul 18, 2014

DanielRosenwasser reviewed Jul 18, 2014
View reviewed changes

JsonFreeman reviewed Jul 19, 2014
View reviewed changes

vladima reviewed Jul 19, 2014
View reviewed changes

DanielRosenwasser mentioned this pull request Jul 20, 2014

[Errors] No error for function with return type annotation that lacks a return statement #62

Closed

CyrusNajmabadi reviewed Jul 21, 2014
View reviewed changes

DanielRosenwasser added 5 commits July 21, 2014 16:42

Removed TODO relating to the task I just fixed.

b7343c2

Minor renaming.

69018e6

Changed check on annotated functions/getters to only look for return …

1728f7c

…statements.

Changed error message for annotated functions lacking return statements.

0f4e887

DanielRosenwasser added a commit that referenced this pull request Jul 22, 2014

Merge pull request #147 from Microsoft/noReturnExpression

c8fc26a

Complain when a non-void function lacks a return expresson.

DanielRosenwasser merged commit c8fc26a into master Jul 22, 2014

DanielRosenwasser deleted the noReturnExpression branch July 22, 2014 20:46

CyrusNajmabadi reviewed Jul 22, 2014
View reviewed changes

DanielRosenwasser added a commit that referenced this pull request Jul 23, 2014

Removed parentheses around arrow function parameters.

0e10fc7

As per feedback in pull request #147.

microsoft locked and limited conversation to collaborators Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complain when a non-void function lacks a return expresson. #147

Complain when a non-void function lacks a return expresson. #147

DanielRosenwasser commented Jul 18, 2014

DanielRosenwasser Jul 18, 2014

RyanCavanaugh commented Jul 18, 2014

JsonFreeman Jul 19, 2014

DanielRosenwasser Jul 19, 2014

vladima Jul 19, 2014

vladima Jul 19, 2014

DanielRosenwasser Jul 19, 2014

RyanCavanaugh Jul 20, 2014

DanielRosenwasser Jul 20, 2014

JsonFreeman commented Jul 19, 2014

vladima Jul 19, 2014

CyrusNajmabadi Jul 21, 2014

mhegazy commented Jul 22, 2014

CyrusNajmabadi Jul 22, 2014

DanielRosenwasser Jul 23, 2014

Complain when a non-void function lacks a return expresson. #147

Complain when a non-void function lacks a return expresson. #147

Conversation

DanielRosenwasser commented Jul 18, 2014

Choose a reason for hiding this comment

RyanCavanaugh commented Jul 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsonFreeman commented Jul 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Jul 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment