Skip to content

Better error recovery when errant semicolon found in a class. #25

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -619,14 +619,14 @@ module ts {
}

// True if positioned at the start of a list element
function isListElement(kind: ParsingContext): boolean {
function isListElement(kind: ParsingContext, inErrorRecovery: boolean): boolean {
switch (kind) {
case ParsingContext.SourceElements:
case ParsingContext.ModuleElements:
return isSourceElement();
return isSourceElement(inErrorRecovery);
case ParsingContext.BlockStatements:
case ParsingContext.SwitchClauseStatements:
return isStatement();
return isStatement(inErrorRecovery);
case ParsingContext.SwitchClauses:
return token === SyntaxKind.CaseKeyword || token === SyntaxKind.DefaultKeyword;
case ParsingContext.TypeMembers:
Expand All @@ -650,6 +650,8 @@ module ts {
case ParsingContext.TypeArguments:
return isType();
}

Debug.fail("Non-exhaustive case in 'isListElement'.");
}

// True if positioned at a list terminator
Expand Down Expand Up @@ -717,10 +719,12 @@ module ts {
}

// True if positioned at element or terminator of the current list or any enclosing list
function isInParsingContext(): boolean {
function isInSomeParsingContext(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!! Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it wasn't easy, but I did it all for you Jason. 🎂

for (var kind = 0; kind < ParsingContext.Count; kind++) {
if (parsingContext & (1 << kind)) {
if (isListElement(kind) || isListTerminator(kind)) return true;
if (isListElement(kind, /* inErrorRecovery */ true) || isListTerminator(kind)) {
return true;
}
}
}

Expand All @@ -734,12 +738,12 @@ module ts {
var result = <NodeArray<T>>[];
result.pos = getNodePos();
while (!isListTerminator(kind)) {
if (isListElement(kind)) {
if (isListElement(kind, /* inErrorRecovery */ false)) {
result.push(parseElement());
}
else {
error(parsingContextErrors(kind));
if (isInParsingContext()) {
if (isInSomeParsingContext()) {
break;
}
nextToken();
Expand All @@ -761,7 +765,7 @@ module ts {
var errorCountBeforeParsingList = file.syntacticErrors.length;
var commaStart = -1; // Meaning the previous token was not a comma
while (true) {
if (isListElement(kind)) {
if (isListElement(kind, /* inErrorRecovery */ false)) {
result.push(parseElement());
commaStart = scanner.getTokenPos();
if (parseOptional(SyntaxKind.CommaToken)) {
Expand Down Expand Up @@ -791,7 +795,7 @@ module ts {
}
else {
error(parsingContextErrors(kind));
if (token !== SyntaxKind.CommaToken && isInParsingContext()) {
if (isInSomeParsingContext()) {
break;
}
nextToken();
Expand Down Expand Up @@ -2066,12 +2070,19 @@ module ts {
return finishNode(node);
}

function isStatement(): boolean {
function isStatement(inErrorRecovery: boolean): boolean {
switch (token) {
case SyntaxKind.SemicolonToken:
// If we're in error recovery, then we don't want to treat ';' as an empty statement.
// The problem is that ';' can show up in far too many contexts, and if we see one
// and assume it's a statement, then we may bail out innapropriately from whatever
// we're parsing. For example, if we have a semicolon in the middle of a class, then
// we really don't want to assume the class is over and we're on a statement in the
// outer module. We just want to consume and move on.
return !inErrorRecovery;
case SyntaxKind.OpenBraceToken:
case SyntaxKind.VarKeyword:
case SyntaxKind.FunctionKeyword:
case SyntaxKind.SemicolonToken:
case SyntaxKind.IfKeyword:
case SyntaxKind.DoKeyword:
case SyntaxKind.WhileKeyword:
Expand Down Expand Up @@ -2103,8 +2114,8 @@ module ts {
}
}

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

function parseStatement(): Statement {
Expand Down Expand Up @@ -2813,8 +2824,8 @@ module ts {
return result;
}

function isSourceElement(): boolean {
return isDeclaration() || isStatement();
function isSourceElement(inErrorRecovery: boolean): boolean {
return isDeclaration() || isStatement(inErrorRecovery);
}

function parseSourceElement() {
Expand Down
4 changes: 1 addition & 3 deletions tests/baselines/reference/ambientGetters.errors.txt
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
==== tests/cases/compiler/ambientGetters.ts (3 errors) ====
==== tests/cases/compiler/ambientGetters.ts (2 errors) ====

declare class A {
get length() : number;
~
!!! '{' expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad error because fixing it (by putting the braces) will just lead you to another error (no accessors in ambient context)

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 catch - this is definitely something worth looking into.

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 #139.

}
~
!!! Declaration or statement expected.

declare class B {
get length() { return 0; }
Expand Down
34 changes: 4 additions & 30 deletions tests/baselines/reference/es6ClassTest3.errors.txt
Original file line number Diff line number Diff line change
@@ -1,45 +1,19 @@
==== tests/cases/compiler/es6ClassTest3.ts (15 errors) ====
==== tests/cases/compiler/es6ClassTest3.ts (2 errors) ====
module M {
class Visibility {
public foo() { };
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
private bar() { };
~~~~~~~
!!! Declaration or statement expected.
~
!!! ';' expected.
~~~
!!! Cannot find name 'bar'.
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
private x: number;
~~~~~~~
!!! Declaration or statement expected.
~~~~~~
!!! Cannot find name 'number'.
public y: number;
~~~~~~
!!! Declaration or statement expected.
~~~~~~
!!! Cannot find name 'number'.
public z: number;
~~~~~~
!!! Declaration or statement expected.
~~~~~~
!!! Cannot find name 'number'.

constructor() {
~
!!! ';' expected.
~~~~~~~~~~~
!!! Cannot find name 'constructor'.
this.x = 1;
~~~~
!!! 'this' cannot be referenced in a module body.
this.y = 2;
~~~~
!!! 'this' cannot be referenced in a module body.
}
}
}
~
!!! Declaration or statement expected.
}
18 changes: 4 additions & 14 deletions tests/baselines/reference/exportDeclareClass1.errors.txt
Original file line number Diff line number Diff line change
@@ -1,26 +1,16 @@
==== tests/cases/compiler/exportDeclareClass1.ts (9 errors) ====
==== tests/cases/compiler/exportDeclareClass1.ts (4 errors) ====
export declare class eaC {
static tF() { };
~
!!! A function implementation cannot be declared in an ambient context.
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
static tsF(param:any) { };
~~~~~~
!!! Declaration or statement expected.
~
!!! ',' expected.
~
!!! ';' expected.
~~~
!!! Cannot find name 'tsF'.
~~~~~
!!! Cannot find name 'param'.
~~~
!!! Cannot find name 'any'.
!!! A function implementation cannot be declared in an ambient context.
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
};
~
!!! Declaration or statement expected.

export declare class eaC2 {
static tF();
Expand Down
14 changes: 4 additions & 10 deletions tests/baselines/reference/parser0_004152.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
==== tests/cases/conformance/parser/ecmascript5/Fuzz/parser0_004152.ts (38 errors) ====
==== tests/cases/conformance/parser/ecmascript5/Fuzz/parser0_004152.ts (34 errors) ====
export class Game {
~~~~~~~~~~~~~~~~~~~
private position = new DisplayPosition([), 3, 3, 3, 3, 3, 0, 3, 3, 3, 3, 3, 3, 0], NoMove, 0);
Expand Down Expand Up @@ -40,8 +40,7 @@
!!! ';' expected.
~
!!! Unexpected token. A constructor, method, accessor, or property was expected.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! Cannot compile external modules unless the '--module' flag is provided.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~
!!! Cannot find name 'DisplayPosition'.
~
Expand Down Expand Up @@ -69,14 +68,9 @@
~
!!! Duplicate identifier '0'.
private prevConfig: SeedCoords[][];
~~~~~~~
!!! Declaration or statement expected.
~
!!! Expression expected.
~
!!! Expression expected.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~
!!! Cannot find name 'SeedCoords'.
}
~
!!! Declaration or statement expected.
!!! Cannot compile external modules unless the '--module' flag is provided.
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEqualsGreaterThanAfterFunction2.ts (4 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserEqualsGreaterThanAfterFunction2.ts (5 errors) ====
function (a => b;
~
!!! Identifier expected.
~~
!!! ',' expected.
~
!!! ',' expected.

!!! ')' expected.
~~~~~~~~~~~~~~~~~
!!! Function implementation expected.
Loading