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
Changes from 2 commits
5b6bb5b
5fc2792
57d7cf5
b76c13c
b0c59e7
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 |
---|---|---|
|
@@ -1445,6 +1445,7 @@ 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. | ||
|
@@ -1457,49 +1458,67 @@ module ts { | |
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); | ||
var sig = tryParse(parseSignatureIfArrowOrBraceFollows); | ||
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: | ||
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. What happens in the following case? var a = true ? () : false; 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. 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. 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 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()) { | ||
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. Why did tihs comment move outside of the block that it is relevant to? 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. Felt like it would be more consistent since all the other comment blocks reside outside the checks. I'm feeling against it now. 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. 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; | ||
} | ||
|
||
|
@@ -1526,10 +1545,12 @@ module ts { | |
return Tristate.False; | ||
} | ||
|
||
function parseSignatureAndArrow(): ParsedSignature { | ||
function parseSignatureIfArrowOrBraceFollows(): ParsedSignature { | ||
var sig = parseSignature(SyntaxKind.CallSignature, SyntaxKind.ColonToken); | ||
parseExpected(SyntaxKind.EqualsGreaterThanToken); | ||
return sig; | ||
if (token === SyntaxKind.EqualsGreaterThanToken || token === SyntaxKind.OpenBraceToken) { | ||
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. looks good. could you add a comment here why having a colon isn't enogh. i.e. the ambiguity with: a ? (b) : ... |
||
return sig; | ||
} | ||
return undefined; | ||
} | ||
|
||
function parseArrowExpressionTail(pos: number, sig: ParsedSignature, noIn: boolean): FunctionExpression { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
==== tests/cases/compiler/arrowFunctionsMissingTokens.ts (31 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. | ||
~ | ||
!!! ',' expected. | ||
~ | ||
!!! Cannot find name 'x'. | ||
|
||
var e = (x: number, y: string): void; | ||
~ | ||
!!! ')' expected. | ||
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. still not good handling for this case. i think you need to explicitly look for id and treat that as an arrow function. |
||
~ | ||
!!! ',' expected. | ||
~ | ||
!!! Variable declaration expected. | ||
~~~~ | ||
!!! Variable declaration expected. | ||
~ | ||
!!! Expression expected. | ||
~ | ||
!!! Cannot find name 'x'. | ||
} | ||
|
||
module okay { | ||
var a = () => { }; | ||
|
||
var b = (): void => { } | ||
|
||
var c = (x) => { }; | ||
|
||
var d = (x: number, y: string) => { }; | ||
|
||
var e = (x: number, y: string): void => { }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)' | ||
|
@@ -10,9 +10,7 @@ | |
~~~~~ | ||
!!! Statement expected. | ||
~ | ||
!!! ';' expected. | ||
~ | ||
!!! Cannot find name 'x'. | ||
!!! '=>' expected. | ||
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. |
||
|
||
finally{ } // error missing try | ||
~~~~~~~ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
==== tests/cases/conformance/parser/ecmascript5/RegressionTests/parser566700.ts (1 errors) ==== | ||
var v = ()({}); | ||
~ | ||
!!! '=>' expected. | ||
~ | ||
!!! Expression expected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/Expressions/parserErrorRecovery_Expression1.ts (1 errors) ==== | ||
var v = ()({}); | ||
~ | ||
!!! '=>' expected. | ||
~ | ||
!!! Expression expected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,7 @@ | ||
==== tests/cases/compiler/uncaughtCompilerError2.ts (2 errors) ==== | ||
==== tests/cases/compiler/uncaughtCompilerError2.ts (1 errors) ==== | ||
function getObj() { | ||
().toString(); | ||
~ | ||
!!! '=>' expected. | ||
~~~~~~~~ | ||
!!! Cannot find name 'toString'. | ||
~ | ||
!!! Expression expected. | ||
} | ||
|
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.
You don't need the tryParse here. Instead, just call tryParseSignatureIfArrowOrBraceFollows. Internally, that function should use tryParse for the signature portion.