Skip to content
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

If the expression after the return statement is on a new line, the generated code is incorrect #2704

Closed
victorhurdugaci opened this issue Apr 10, 2015 · 5 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@victorhurdugaci
Copy link

Typescript:

    private isIdentifierCharacter(char: string): boolean {
        var charCode = char.charCodeAt(0);
        return
            ((charCode >= 'a'.charCodeAt(0) && charCode <= 'z'.charCodeAt(0)) ||
            charCode == '_'.charCodeAt(0));
    }

Generated js:

 FormulaScanner.prototype.isIdentifierCharacter = function (char) {
        var charCode = char.charCodeAt(0);
        return;
        ((charCode >= 'a'.charCodeAt(0) && charCode <= 'z'.charCodeAt(0)) || charCode == '_'.charCodeAt(0));
    };

Notice the ; after return in the generated code.

@CyrusNajmabadi CyrusNajmabadi added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Apr 10, 2015
@CyrusNajmabadi
Copy link
Contributor

This is by design due to how Automatic-Semicolon-Insertion (ASI) works for JavaScript. When you write

return
            ((charCode >= 'a'.charCodeAt(0) && charCode <= 'z'.charCodeAt(0)) ||
            charCode == '_'.charCodeAt(0));

It is syntactically equivalent to

return;
            ((charCode >= 'a'.charCodeAt(0) && charCode <= 'z'.charCodeAt(0)) ||
            charCode == '_'.charCodeAt(0));

This is covered in the ECMAScript spec here:

ReturnStatement :
    return [no LineTerminator here] Expression ;

...

The practical effect of these restricted productions is as follows:
...
When a continue, break, return, or throw token is encountered and a LineTerminator is encountered before the next token, a semicolon is automatically inserted after the continue, break, return, or throw token.

@victorhurdugaci
Copy link
Author

Ugh.. that's not fun. My 2c on how this could be improved, 3 alternatives:

  1. Don't be aligned with the spec and just insert the semicolon where it is mostly expected
  2. Give an "unreachable statement" warning because the code after return is not reachable.
  3. Give an error because the method is not returning boolean (in this case).

@CyrusNajmabadi
Copy link
Contributor

  1. Typescript is a superset of ECMAScript. We can't not be aligned with them :)
  2. We're considering the unreachable warning. @vladima has a PR for that here: Simple control flow analysis  #1287
  3. Empty return statements are legal (and common) in JavaScript. They are equivalent to return undefined. And, as undefined is an acceptable value for any type, we have to accept this. To not do this would be a massive breaking change.

That said, these would be an appropriate area for a Linter to help out with.

@IanYates
Copy link

IanYates commented May 6, 2015

@CyrusNajmabadi - I'd be keen for a linter to allow this to flag a warning, or even an error (as I believe it is in C#). I was just bitten by this today in code I wrote for a team member.

In my code, I now see via the intellisense popup on my method that the inferred return type was void. If I had put the expected return type in the method signature I would've had this at least flagged... Something to keep in mind :) (a VS 2013/2015 extension that put the inferred return type in grey next to the method name would be fantastic!)

Your points 1 & 3 are quite valid, and I take advantage of 3 frequently. I just didn't mean to take advantage this time unknowingly...

@basarat
Copy link
Contributor

basarat commented Sep 23, 2015

There is already a linter rule for this : https://github.com/palantir/tslint/blob/master/src/rules/noUnreachableRule.ts

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

5 participants