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

Improved lookahead for arrow functions. #203

Merged
merged 5 commits into from Jul 25, 2014
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 59 additions & 24 deletions src/compiler/parser.ts
Expand Up @@ -1445,64 +1445,87 @@ module ts {
function tryParseParenthesizedArrowFunctionExpression(): Expression {
var pos = getNodePos();

// Indicates whether we are certain that we should parse an arrow expression.
var triState = isParenthesizedArrowFunctionExpression();

// It is not a parenthesized arrow function.
if (triState === Tristate.False) {
return undefined;
}

// If we're certain that we have an arrow function expression, then just parse one out.
if (triState === Tristate.True) {
var sig = parseSignature(SyntaxKind.CallSignature, SyntaxKind.ColonToken);

// If we have an arrow, then try to parse the body.
if (parseExpected(SyntaxKind.EqualsGreaterThanToken)) {
return parseArrowExpressionTail(pos, sig, /*noIn:*/ false);
// Even if not, try to parse if we have an opening brace, just in case we're in an error state.
if (parseExpected(SyntaxKind.EqualsGreaterThanToken) || token === SyntaxKind.OpenBraceToken) {
return parseArrowExpressionTail(pos, sig, /* noIn: */ false);
}
// If not, we're probably better off bailing out and returning a bogus function expression.
else {
// If not, we're probably better off bailing out and returning a bogus function expression.
return makeFunctionExpression(SyntaxKind.ArrowFunction, pos, /* name */ undefined, sig, createMissingNode());
}
}

// Otherwise, *maybe* we had an arrow function and we need to *try* to parse it out
// (which will ensure we rollback if we fail).
var sig = tryParse(parseSignatureAndArrow);
// *Maybe* we had an arrow function and we need to try to parse it out,
// rolling back and trying other parses if we fail.
var sig = tryParse(parseSignatureIfArrowOrBraceFollows);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the tryParse here. Instead, just call tryParseSignatureIfArrowOrBraceFollows. Internally, that function should use tryParse for the signature portion.

if (sig === undefined) {
return undefined;
}
else {
parseExpected(SyntaxKind.EqualsGreaterThanToken);
return parseArrowExpressionTail(pos, sig, /*noIn:*/ false);
}
}

// True -> There is definitely a parenthesized arrow function here.
// False -> There is definitely *not* a parenthesized arrow function here.
// True -> We definitely expect a parenthesized arrow function here.
// False -> There *cannot* be a parenthesized arrow function here.
// Unknown -> There *might* be a parenthesized arrow function here.
// Speculatively look ahead to be sure.
// Speculatively look ahead to be sure, and rollback if not.
function isParenthesizedArrowFunctionExpression(): Tristate {
if (token === SyntaxKind.OpenParenToken || token === SyntaxKind.LessThanToken) {
return lookAhead(() => {
var first = token;
nextToken();
var second = nextToken();

if (first === SyntaxKind.OpenParenToken) {
if (token === SyntaxKind.CloseParenToken || token === SyntaxKind.DotDotDotToken) {
// Simple cases. if we see () or (... then presume that presume
// that this must be an arrow function. Note, this may be too aggressive
// for the "()" case. It's not uncommon for this to appear while editing
// code. We should look to see if there's actually a => before proceeding.
if (second === SyntaxKind.CloseParenToken) {
// Simple cases: "() =>", "(): ", and "() {".
// This is an arrow function with no parameters.
// The last one is not actually an arrow function,
// but this is probably what the user intended.
var third = nextToken();
switch (third) {
case SyntaxKind.EqualsGreaterThanToken:
case SyntaxKind.ColonToken:
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens in the following case?

var a = true ? () : false;

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, but that's not a valid expression anyway. Cyrus and I discussed this offline, and it doesn't seem like you'd get such a big win there.

Still, if you'd like to file a bug for it, I'd be more than happy to take it on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok

case SyntaxKind.OpenBraceToken:
return Tristate.True;
default:
return Tristate.False;
}
}

// Simple case: "(..."
// This is an arrow function with a rest parameter.
if (second === SyntaxKind.DotDotDotToken) {
return Tristate.True;
}

// If we had "(" followed by something that's not an identifier,
// then this definitely doesn't look like a lambda.
// Note: we could be a little more lenient and allow
// "(public" or "(private". These would not ever actually be allowed,
// but we could provide a good error message instead of bailing out.
if (!isIdentifier()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did tihs comment move outside of the block that it is relevant to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Felt like it would be more consistent since all the other comment blocks reside outside the checks. I'm feeling against it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, with different phrasing, I think it might be fine.

// We had "(" not followed by an identifier. This definitely doesn't
// look like a lambda. Note: we could be a little more lenient and allow
// (public or (private. These would not ever actually be allowed,
// but we could provide a good error message instead of bailing out.
return Tristate.False;
}

// If we have something like "(a:", then we must have a
// type-annotated parameter in an arrow function expression.
if (nextToken() === SyntaxKind.ColonToken) {
return Tristate.True;
}

// This *could* be a parenthesized arrow function.
// Return Unknown to let the caller know.
return Tristate.Unknown;
Expand All @@ -1526,10 +1549,22 @@ module ts {
return Tristate.False;
}

function parseSignatureAndArrow(): ParsedSignature {
function parseSignatureIfArrowOrBraceFollows(): ParsedSignature {
var sig = parseSignature(SyntaxKind.CallSignature, SyntaxKind.ColonToken);
parseExpected(SyntaxKind.EqualsGreaterThanToken);
return sig;

// Parsing a signature isn't enough.
// Parenthesized arrow signatures often look like other valid expressions.
// For instance:
// - "(x = 10)" is an assignment expression parsed as a signature with a default parameter value.
// - "(x,y)" is a comma expression parsed as a signature with two parameters.
// - "a ? (b): c" will have "(b):" parsed as a signature with a return type annotation.
//
// So we need just a bit of lookahead to ensure that it can only be a signature.
if (token === SyntaxKind.EqualsGreaterThanToken || token === SyntaxKind.OpenBraceToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good. could you add a comment here why having a colon isn't enogh. i.e. the ambiguity with:

a ? (b) : ...
thanks!

return sig;
}

return undefined;
}

function parseArrowExpressionTail(pos: number, sig: ParsedSignature, noIn: boolean): FunctionExpression {
Expand Down
115 changes: 115 additions & 0 deletions tests/baselines/reference/arrowFunctionsMissingTokens.errors.txt
@@ -0,0 +1,115 @@
==== tests/cases/compiler/arrowFunctionsMissingTokens.ts (24 errors) ====

module missingArrowsWithCurly {
var a = () { };
~
!!! '=>' expected.

var b = (): void { }
~
!!! '=>' expected.

var c = (x) { };
~
!!! '=>' expected.

var d = (x: number, y: string) { };
~
!!! '=>' expected.

var e = (x: number, y: string): void { };
~
!!! '=>' expected.
}

module missingCurliesWithArrow {
module withStatement {
var a = () => var k = 10;};
~~~
!!! '{' expected.

var b = (): void => var k = 10;}
~~~
!!! '{' expected.

var c = (x) => var k = 10;};
~~~
!!! '{' expected.

var d = (x: number, y: string) => var k = 10;};
~~~
!!! '{' expected.

var e = (x: number, y: string): void => var k = 10;};
~~~
!!! '{' expected.

var f = () => var k = 10;}
~~~
!!! '{' expected.
}

module withoutStatement {
var a = () => };
~
!!! Expression expected.

var b = (): void => }
~
!!! Expression expected.

var c = (x) => };
~
!!! Expression expected.

var d = (x: number, y: string) => };
~
!!! Expression expected.

var e = (x: number, y: string): void => };
~
!!! Expression expected.

var f = () => }
~
!!! Expression expected.
}
~
!!! Declaration or statement expected.
}
~
!!! Declaration or statement expected.

module ceci_nEst_pas_une_arrow_function {
var a = ();
~
!!! Expression expected.

var b = (): void;
~
!!! '=>' expected.

var c = (x);
~
!!! Cannot find name 'x'.

var d = (x: number, y: string);
~
!!! '=>' expected.

var e = (x: number, y: string): void;
~
!!! '=>' expected.
}

module okay {
var a = () => { };

var b = (): void => { }

var c = (x) => { };

var d = (x: number, y: string) => { };

var e = (x: number, y: string): void => { };
}
8 changes: 3 additions & 5 deletions tests/baselines/reference/emptyMemberAccess.errors.txt
@@ -1,11 +1,9 @@
==== tests/cases/compiler/emptyMemberAccess.ts (2 errors) ====
==== tests/cases/compiler/emptyMemberAccess.ts (1 errors) ====
function getObj() {

().toString();
~
!!! '=>' expected.
~~~~~~~~
!!! Cannot find name 'toString'.
~
!!! Expression expected.

}

4 changes: 2 additions & 2 deletions tests/baselines/reference/es6ClassTest9.errors.txt
Expand Up @@ -2,8 +2,8 @@
declare class foo();
~
!!! '{' expected.
~
!!! '=>' expected.
~
!!! Expression expected.
function foo() {}
~~~
!!! Duplicate identifier 'foo'.
Expand Down
22 changes: 4 additions & 18 deletions tests/baselines/reference/fatarrowfunctionsErrors.errors.txt
@@ -1,4 +1,4 @@
==== tests/cases/compiler/fatarrowfunctionsErrors.ts (25 errors) ====
==== tests/cases/compiler/fatarrowfunctionsErrors.ts (18 errors) ====
foo((...Far:any[])=>{return 0;})
~~~
!!! Cannot find name 'foo'.
Expand Down Expand Up @@ -39,25 +39,11 @@
~
!!! '=>' expected.
var x2 = (a:number) :void {};
~
!!! ')' expected.
~
!!! ',' expected.
~
!!! Variable declaration expected.
~~~~
!!! Variable declaration expected.
~
!!! Cannot find name 'a'.
~
!!! '=>' expected.
var x3 = (a:number) {};
~
!!! ')' expected.
~
!!! ',' expected.
~
!!! Variable declaration expected.
~
!!! Cannot find name 'a'.
!!! '=>' expected.
var x4= (...a: any[]) { };
~
!!! '=>' expected.
6 changes: 2 additions & 4 deletions tests/baselines/reference/invalidTryStatements2.errors.txt
@@ -1,4 +1,4 @@
==== tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts (5 errors) ====
==== tests/cases/conformance/statements/tryStatements/invalidTryStatements2.ts (4 errors) ====
function fn() {
try {
} catch { // syntax error, missing '(x)'
Expand All @@ -10,9 +10,7 @@
~~~~~
!!! Statement expected.
~
!!! ';' expected.
~
!!! Cannot find name 'x'.
!!! '=>' expected.
Copy link
Member Author

Choose a reason for hiding this comment

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


finally{ } // error missing try
~~~~~~~
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/parser566700.errors.txt
@@ -1,4 +1,4 @@
==== tests/cases/conformance/parser/ecmascript5/RegressionTests/parser566700.ts (1 errors) ====
var v = ()({});
~
!!! '=>' expected.
~
!!! Expression expected.
@@ -1,8 +1,6 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEmptyParenthesizedExpression1.ts (2 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEmptyParenthesizedExpression1.ts (1 errors) ====
function getObj() {
().toString();
~
!!! '=>' expected.
~~~~~~~~
!!! Cannot find name 'toString'.
~
!!! Expression expected.
}
@@ -1,4 +1,4 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/Expressions/parserErrorRecovery_Expression1.ts (1 errors) ====
var v = ()({});
~
!!! '=>' expected.
~
!!! Expression expected.
@@ -1,10 +1,6 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/ParameterLists/parserErrorRecovery_ParameterList5.ts (4 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/ParameterLists/parserErrorRecovery_ParameterList5.ts (2 errors) ====
(a:number => { }
~
!!! ')' expected.
~~
!!! ';' expected.
~
!!! Cannot find name 'a'.
~~~~~~
!!! Cannot find name 'number'.
!!! ',' expected.
~
!!! ')' expected.