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.

Sign up for GitHub

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

Jump to bottom

Better error recovery for when an arrow function is missing a curly brace. #28

Closed
wants to merge 2 commits into from
Closed

Better error recovery for when an arrow function is missing a curly brace. #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eac8a7f
Better error recovery for when an arrow function is missing a curly b…
DanielRosenwasser Jul 16, 2014
a9a38be
Addressed code review feedback.
DanielRosenwasser Jul 16, 2014
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
143 changes: 92 additions & 51 deletions src/compiler/parser.ts
View file
Open in desktop
Expand Up @@ -299,6 +299,12 @@ module ts {
Count // Number of parsing contexts
}

enum Tristate {
Copy link
Contributor

Choose a reason for hiding this comment

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

TristateBoolean, or just Troolean (jk, don't actually name it that).

Also, the order should probably be False, Maybe, True

Copy link
Member Author

Choose a reason for hiding this comment

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

So one might say that you're...trollean about trooleans?

😎

False,
True,
Unknown
}

function parsingContextErrors(context: ParsingContext): DiagnosticMessage {
switch (context) {
case ParsingContext.SourceElements: return Diagnostics.Declaration_or_statement_expected;
Expand Down Expand Up @@ -1262,6 +1268,11 @@ module ts {
}
}

function isExpressionStatement(): boolean {
// As per the grammar, neither '{' nor 'function' can start an expression statement.
return token !== SyntaxKind.OpenBraceToken && token !== SyntaxKind.FunctionKeyword && isExpression();
}

function parseExpression(noIn?: boolean): Expression {
var expr = parseAssignmentExpression(noIn);
while (parseOptional(SyntaxKind.CommaToken)) {
Expand Down Expand Up @@ -1405,33 +1416,43 @@ module ts {
function tryParseParenthesizedArrowFunctionExpression(): Expression {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is parseSignatureAndArrow called anymore? If not, you can get rid of it.

var pos = getNodePos();

// Note isParenthesizedArrowFunctionExpression returns true, false or undefined.
var triState = isParenthesizedArrowFunctionExpression();
if (triState !== false) {
// If we *definitely* had an arrow function expression, then just parse one out.
// Otherwise we *maybe* had an arrow function and we need to *try* to parse it out
// (which will ensure we rollback if we fail).
var sig = triState === true
? parseSignatureAndArrow()
: tryParse(parseSignatureAndArrow);

// If we got a signature, we're good to go. consume the rest of this arrow function.
if (sig) {

// 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);
}
// If not, we're probably better off bailing out and returning a bogus function expression.
else {
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just do tryParseSignatureAndArrow, folding the tryParse into the function itself

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

// Was not a parenthesized arrow function.
return undefined;
}

// Note: this function returns a tristate:
//
// true -> there is definitely a parenthesized arrow function here.
// false -> there is definitely *not* a parenthesized arrow function here.
// undefined -> there *might* be a parenthesized arrow function here. Speculatively
// look ahead to be sure.
function isParenthesizedArrowFunctionExpression(): boolean {
// True -> There is definitely a parenthesized arrow function here.
// False -> There is definitely *not* a parenthesized arrow function here.
// Unknown -> There *might* be a parenthesized arrow function here.
// Speculatively look ahead to be sure.
function isParenthesizedArrowFunctionExpression(): Tristate {
if (token === SyntaxKind.OpenParenToken || token === SyntaxKind.LessThanToken) {
return lookAhead(() => {
var first = token;
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the comment above this. It says you're returning undefined.

Expand All @@ -1442,39 +1463,38 @@ module ts {
// 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.
return true;
return Tristate.True;
}

if (!isIdentifier()) {
// 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 false;
return Tristate.False;
}

// This *could* be a parenthesized arrow function. Return 'undefined' to let
// the caller know.
return undefined;
// This *could* be a parenthesized arrow function.
// Return Unknown to let the caller know.
return Tristate.Unknown;
}
else {
Debug.assert(first === SyntaxKind.LessThanToken);

// If we have "<" not followed by an identifier, then this definitely is not
// an arrow function.
// If we have "<" not followed by an identifier,
// then this definitely is not an arrow function.
if (!isIdentifier()) {
return false;
return Tristate.False;
}

// This *could* be a parenthesized arrow function. Return 'undefined' to let
// the caller know.
return undefined;
// This *could* be a parenthesized arrow function.
return Tristate.Unknown;
}
});
}

// Definitely not a parenthesized arrow function.
return false;
return Tristate.False;
}

function parseSignatureAndArrow(): ParsedSignature {
Expand All @@ -1484,8 +1504,33 @@ module ts {
}

function parseArrowExpressionTail(pos: number, sig: ParsedSignature, noIn: boolean): FunctionExpression {
var body = token === SyntaxKind.OpenBraceToken ? parseBody() : parseAssignmentExpression(noIn);
return makeFunctionExpression(SyntaxKind.ArrowFunction, pos, undefined, sig, body);
var body: Node;

if (token === SyntaxKind.OpenBraceToken) {
body = parseBody(/* ignoreMissingOpenBrace */ false)
}
else if (isStatement(/* inErrorRecovery */ true) && !isExpressionStatement() && token !== SyntaxKind.FunctionKeyword) {
// Check if we got a plain statement (i.e. no expression-statements, no functions expressions/declarations)
//
// Here we try to recover from a potential error situation in the case where the
// user meant to supply a block. For example, if the user wrote:
//
// a =>
// var v = 0;
// }
//
// they may be missing an open brace. Check to see if that's the case so we can
// try to recover better. If we don't do this, then the next close curly we see may end
// up preemptively closing the containing construct.
//
// Note: even when 'ignoreMissingOpenBrace' is passed as true, parseBody will still error.
body = parseBody(/* ignoreMissingOpenBrace */ true);
}
else {
body = parseAssignmentExpression(noIn);
}

return makeFunctionExpression(SyntaxKind.ArrowFunction, pos, /* name */ undefined, sig, body);
}

function isAssignmentOperator(): boolean {
Expand Down Expand Up @@ -1761,7 +1806,7 @@ module ts {
node.name = parsePropertyName();
if (token === SyntaxKind.OpenParenToken || token === SyntaxKind.LessThanToken) {
var sig = parseSignature(SyntaxKind.CallSignature, SyntaxKind.ColonToken);
var body = parseBody();
var body = parseBody(/* ignoreMissingOpenBrace */ false);
node.initializer = makeFunctionExpression(SyntaxKind.FunctionExpression, node.pos, node.name, sig, body);
}
else {
Expand Down Expand Up @@ -1795,7 +1840,7 @@ module ts {
parseExpected(SyntaxKind.FunctionKeyword);
var name = isIdentifier() ? parseIdentifier() : undefined;
var sig = parseSignature(SyntaxKind.CallSignature, SyntaxKind.ColonToken);
var body = parseBody();
var body = parseBody(/* ignoreMissingOpenBrace */ false);
return makeFunctionExpression(SyntaxKind.FunctionExpression, pos, name, sig, body);
}

Expand All @@ -1822,9 +1867,9 @@ module ts {

// STATEMENTS

function parseBlock(): Block {
function parseBlock(ignoreMissingOpenBrace: boolean): Block {
var node = <Block>createNode(SyntaxKind.Block);
if (parseExpected(SyntaxKind.OpenBraceToken)) {
if (parseExpected(SyntaxKind.OpenBraceToken) || ignoreMissingOpenBrace) {
node.statements = parseList(ParsingContext.BlockStatements, parseStatement);
parseExpected(SyntaxKind.CloseBraceToken);
}
Expand All @@ -1834,8 +1879,8 @@ module ts {
return finishNode(node);
}

function parseBody(): Block {
var block = parseBlock();
function parseBody(ignoreMissingOpenBrace: boolean): Block {
var block = parseBlock(ignoreMissingOpenBrace);
block.kind = SyntaxKind.FunctionBlock;
return block;
}
Expand Down Expand Up @@ -2022,7 +2067,7 @@ module ts {
function parseTokenAndBlock(token: SyntaxKind, kind: SyntaxKind): Block {
var pos = getNodePos();
parseExpected(token);
var result = parseBlock();
var result = parseBlock(/* ignoreMissingOpenBrace */ false);
result.kind = kind;
result.pos = pos;
return result;
Expand All @@ -2037,7 +2082,7 @@ module ts {
var typeAnnotationColonLength = scanner.getTextPos() - typeAnnotationColonStart;
var typeAnnotation = parseTypeAnnotation();
parseExpected(SyntaxKind.CloseParenToken);
var result = <CatchBlock>parseBlock();
var result = <CatchBlock>parseBlock(/* ignoreMissingOpenBrace */ false);
result.kind = SyntaxKind.CatchBlock;
result.pos = pos;
result.variable = variable;
Expand Down Expand Up @@ -2114,14 +2159,10 @@ module ts {
}
}

function isStatementOrFunction(inErrorRecovery: boolean): boolean {
return token === SyntaxKind.FunctionKeyword || isStatement(inErrorRecovery);
}

function parseStatement(): Statement {
switch (token) {
case SyntaxKind.OpenBraceToken:
return parseBlock();
return parseBlock(/* ignoreMissingOpenBrace */ false);
case SyntaxKind.VarKeyword:
return parseVariableStatement();
case SyntaxKind.FunctionKeyword:
Expand Down Expand Up @@ -2168,7 +2209,7 @@ module ts {
var initialPosition = scanner.getTokenPos();
var errorCountBeforeBody = file.syntacticErrors.length;
if (token === SyntaxKind.OpenBraceToken) {
var body = parseBody();
var body = parseBody(/* ignoreMissingOpenBrace */ false);
if (body && inAmbientContext && file.syntacticErrors.length === errorCountBeforeBody) {
var diagnostic = isConstructor ? Diagnostics.A_constructor_implementation_cannot_be_declared_in_an_ambient_context : Diagnostics.A_function_implementation_cannot_be_declared_in_an_ambient_context;
grammarErrorAtPos(initialPosition, 1, diagnostic);
Expand Down Expand Up @@ -2340,7 +2381,7 @@ module ts {
node.typeParameters = sig.typeParameters;
node.parameters = sig.parameters;
node.type = sig.type;
node.body = parseBody();
node.body = parseBody(/* ignoreMissingOpenBrace */ false);
return finishNode(node);
}

Expand All @@ -2364,7 +2405,7 @@ module ts {
if (token === SyntaxKind.OpenBracketToken) {
return true;
}

// If we were able to get any potential identifier...
if (idToken !== undefined) {
// If we have a non-keyword identifier, or if we have an accessor, then it's safe to parse.
Expand Down
7 changes: 7 additions & 0 deletions tests/baselines/reference/arrowFunctionMissingCurlyWithSemicolon.errors.txt
View file
Open in desktop
@@ -0,0 +1,7 @@
==== tests/cases/compiler/arrowFunctionMissingCurlyWithSemicolon.ts (1 errors) ====
// Should error at semicolon.
var f = () => ;
~
!!! Expression expected.
var b = 1 * 2 * 3 * 4;
var square = (x: number) => x * x;
4 changes: 3 additions & 1 deletion tests/baselines/reference/emptyMemberAccess.errors.txt
View file
Open in desktop
@@ -1,9 +1,11 @@
==== tests/cases/compiler/emptyMemberAccess.ts (1 errors) ====
==== tests/cases/compiler/emptyMemberAccess.ts (2 errors) ====
function getObj() {

().toString();
~
!!! '=>' expected.
~~~~~~~~
!!! Cannot find name 'toString'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem great. Maybe we should return Tristate.False if we see empty parentheses without an arrow following. Then the normal expression parsing can give an error for empty parenthesized expression

Copy link
Member Author

Choose a reason for hiding this comment

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

Plan to resolve in #34.


}

4 changes: 3 additions & 1 deletion tests/baselines/reference/parserEmptyParenthesizedExpression1.errors.txt
View file
Open in desktop
@@ -1,6 +1,8 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEmptyParenthesizedExpression1.ts (1 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEmptyParenthesizedExpression1.ts (2 errors) ====
function getObj() {
().toString();
~
!!! '=>' expected.
~~~~~~~~
!!! Cannot find name 'toString'.
}
14 changes: 3 additions & 11 deletions tests/baselines/reference/parserMissingLambdaOpenBrace1.errors.txt
View file
Open in desktop
@@ -1,4 +1,4 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserMissingLambdaOpenBrace1.ts (9 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserMissingLambdaOpenBrace1.ts (5 errors) ====
class C {
where(filter: Iterator<T, boolean>): Query<T> {
~~~~~~~~~~~~~~~~~~~~
Expand All @@ -10,18 +10,10 @@
!!! Cannot find name 'fromDoWhile'.
var index = 0;
~~~
!!! Expression expected.
!!! '{' expected.
return this.doWhile((item, i) => filter(item, i) ? test(item, index++) : true);
~~~~~~~
!!! Property 'doWhile' does not exist on type 'C'.
~~~~
!!! Cannot find name 'test'.
});
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
}
}
~
!!! Declaration or statement expected.
}
4 changes: 3 additions & 1 deletion tests/baselines/reference/uncaughtCompilerError2.errors.txt
View file
Open in desktop
@@ -1,7 +1,9 @@
==== tests/cases/compiler/uncaughtCompilerError2.ts (1 errors) ====
==== tests/cases/compiler/uncaughtCompilerError2.ts (2 errors) ====
function getObj() {
().toString();
~
!!! '=>' expected.
~~~~~~~~
!!! Cannot find name 'toString'.
}

4 changes: 4 additions & 0 deletions tests/cases/compiler/arrowFunctionMissingCurlyWithSemicolon.ts
View file
Open in desktop
@@ -0,0 +1,4 @@
// Should error at semicolon.
var f = () => ;
var b = 1 * 2 * 3 * 4;
var square = (x: number) => x * x;