Skip to content

Fix parser regression for bad related diagnostic on missing matching brackets #44158

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

Merged
merged 8 commits into from
Mar 16, 2022
Merged

Fix parser regression for bad related diagnostic on missing matching brackets #44158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3cb457a
Revert "Revert #43460 and #40884 (#44175)"
jessetrinity May 27, 2021
e38a0ce
fix missing opening brace match error
jessetrinity May 27, 2021
fa19d47
refactor parseExpectedMatchingBrackets
jessetrinity Jun 2, 2021
c26543f
use getNodePos
jessetrinity Jun 2, 2021
0490253
accept baselines
jessetrinity Jun 2, 2021
4e3ebb2
Merge branch 'main' into fixParserBracketMatching
sandersn Mar 15, 2022
f02bd72
delete mistakenly added files
sandersn Mar 15, 2022
18a4df4
Revert getNodePos addition
sandersn Mar 15, 2022
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
2 changes: 1 addition & 1 deletion src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"category": "Error",
"code": 1006
},
"The parser expected to find a '}' to match the '{' token here.": {
"The parser expected to find a '{1}' to match the '{0}' token here.": {
"category": "Error",
"code": 1007
},
Expand Down
94 changes: 50 additions & 44 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1434,24 +1434,27 @@ namespace ts {
return inContext(NodeFlags.AwaitContext);
}

function parseErrorAtCurrentToken(message: DiagnosticMessage, arg0?: any): void {
parseErrorAt(scanner.getTokenPos(), scanner.getTextPos(), message, arg0);
function parseErrorAtCurrentToken(message: DiagnosticMessage, arg0?: any): DiagnosticWithDetachedLocation | undefined {
return parseErrorAt(scanner.getTokenPos(), scanner.getTextPos(), message, arg0);
}

function parseErrorAtPosition(start: number, length: number, message: DiagnosticMessage, arg0?: any): void {
function parseErrorAtPosition(start: number, length: number, message: DiagnosticMessage, arg0?: any): DiagnosticWithDetachedLocation | undefined {
// Don't report another error if it would just be at the same position as the last error.
const lastError = lastOrUndefined(parseDiagnostics);
let result: DiagnosticWithDetachedLocation | undefined;
if (!lastError || start !== lastError.start) {
parseDiagnostics.push(createDetachedDiagnostic(fileName, start, length, message, arg0));
result = createDetachedDiagnostic(fileName, start, length, message, arg0);
parseDiagnostics.push(result);
}

// Mark that we've encountered an error. We'll set an appropriate bit on the next
// node we finish so that it can't be reused incrementally.
parseErrorBeforeNextFinishedNode = true;
return result;
}

function parseErrorAt(start: number, end: number, message: DiagnosticMessage, arg0?: any): void {
parseErrorAtPosition(start, end - start, message, arg0);
function parseErrorAt(start: number, end: number, message: DiagnosticMessage, arg0?: any): DiagnosticWithDetachedLocation | undefined {
return parseErrorAtPosition(start, end - start, message, arg0);
}

function parseErrorAtRange(range: TextRange, message: DiagnosticMessage, arg0?: any): void {
Expand Down Expand Up @@ -1779,6 +1782,23 @@ namespace ts {
return false;
}

function parseExpectedMatchingBrackets(openKind: SyntaxKind, closeKind: SyntaxKind, openParsed: boolean, openPosition: number) {
if (token() === closeKind) {
nextToken();
return;
}
const lastError = parseErrorAtCurrentToken(Diagnostics._0_expected, tokenToString(closeKind));
if (!openParsed) {
return;
}
if (lastError) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, openPosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, tokenToString(openKind), tokenToString(closeKind))
);
}
}

function parseOptional(t: SyntaxKind): boolean {
if (token() === t) {
nextToken();
Expand Down Expand Up @@ -3739,7 +3759,7 @@ namespace ts {
if (lastError && lastError.code === Diagnostics._0_expected.code) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_to_match_the_token_here)
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, "{", "}")
);
}
}
Expand Down Expand Up @@ -5772,10 +5792,11 @@ namespace ts {

function parseArrayLiteralExpression(): ArrayLiteralExpression {
const pos = getNodePos();
parseExpected(SyntaxKind.OpenBracketToken);
const openBracketPosition = scanner.getTokenPos();
const openBracketParsed = parseExpected(SyntaxKind.OpenBracketToken);
const multiLine = scanner.hasPrecedingLineBreak();
const elements = parseDelimitedList(ParsingContext.ArrayLiteralMembers, parseArgumentOrArrayLiteralElement);
parseExpected(SyntaxKind.CloseBracketToken);
parseExpectedMatchingBrackets(SyntaxKind.OpenBracketToken, SyntaxKind.CloseBracketToken, openBracketParsed, openBracketPosition);
return finishNode(factory.createArrayLiteralExpression(elements, multiLine), pos);
}

Expand Down Expand Up @@ -5841,18 +5862,10 @@ namespace ts {
function parseObjectLiteralExpression(): ObjectLiteralExpression {
const pos = getNodePos();
const openBracePosition = scanner.getTokenPos();
parseExpected(SyntaxKind.OpenBraceToken);
const openBraceParsed = parseExpected(SyntaxKind.OpenBraceToken);
const multiLine = scanner.hasPrecedingLineBreak();
const properties = parseDelimitedList(ParsingContext.ObjectLiteralMembers, parseObjectLiteralElement, /*considerSemicolonAsDelimiter*/ true);
if (!parseExpected(SyntaxKind.CloseBraceToken)) {
const lastError = lastOrUndefined(parseDiagnostics);
if (lastError && lastError.code === Diagnostics._0_expected.code) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_to_match_the_token_here)
);
}
}
parseExpectedMatchingBrackets(SyntaxKind.OpenBraceToken, SyntaxKind.CloseBraceToken, openBraceParsed, openBracePosition);
Comment on lines 5864 to +5868
Copy link
Member

Choose a reason for hiding this comment

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

Having to repeat openBracePosition/openBraceParsed every time is still a bit noisy. You could eliminate it using a closure, but I don't think it's worth the performance cost. Still, uh, here's what it would look like:

const parseCloseBrace = parseExpectedMatchingToken(SyntaxKind.OpenBraceToken, SyntaxKind.CloseBraceToken)
const multiline = ...
const properties = ...
parseCloseBrace()

And then the first few lines of parseExpectedMatchingBrackets look like this:

        function parseExpectedMatchingBrackets(openKind: SyntaxKind, closeKind: SyntaxKind) {
            const openPosition = getNodePos();
            const openParsed = parseExpected(openKind);
            return () => {
                if (token() === closeKind) { // same as before, but inside a closure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, that's better. Why does it have significantly worse performance though?

Copy link
Member

Choose a reason for hiding this comment

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

You have to create a closure each time, which closes over openKind, closeKind, openPosition, openParsed. The naive way to compile this is to put them onto the heap, so the GC now has 4 numbers and a function to collect.

However, those are all numbers or booleans, and v8 might emit fast code. I have no idea; so it's probably worth testing if you like the change enough -- the existing perf suite certainly has enough brackets to stress it.

return finishNode(factory.createObjectLiteralExpression(properties, multiLine), pos);
}

Expand Down Expand Up @@ -5916,18 +5929,11 @@ namespace ts {
const pos = getNodePos();
const hasJSDoc = hasPrecedingJSDocComment();
const openBracePosition = scanner.getTokenPos();
if (parseExpected(SyntaxKind.OpenBraceToken, diagnosticMessage) || ignoreMissingOpenBrace) {
const openBraceParsed = parseExpected(SyntaxKind.OpenBraceToken, diagnosticMessage);
if (openBraceParsed || ignoreMissingOpenBrace) {
const multiLine = scanner.hasPrecedingLineBreak();
const statements = parseList(ParsingContext.BlockStatements, parseStatement);
if (!parseExpected(SyntaxKind.CloseBraceToken)) {
const lastError = lastOrUndefined(parseDiagnostics);
if (lastError && lastError.code === Diagnostics._0_expected.code) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_to_match_the_token_here)
);
}
}
parseExpectedMatchingBrackets(SyntaxKind.OpenBraceToken, SyntaxKind.CloseBraceToken, openBraceParsed, openBracePosition);
const result = withJSDoc(finishNode(factory.createBlock(statements, multiLine), pos), hasJSDoc);
if (token() === SyntaxKind.EqualsToken) {
parseErrorAtCurrentToken(Diagnostics.Declaration_or_statement_expected_This_follows_a_block_of_statements_so_if_you_intended_to_write_a_destructuring_assignment_you_might_need_to_wrap_the_the_whole_assignment_in_parentheses);
Expand Down Expand Up @@ -5983,9 +5989,10 @@ namespace ts {
const pos = getNodePos();
const hasJSDoc = hasPrecedingJSDocComment();
parseExpected(SyntaxKind.IfKeyword);
parseExpected(SyntaxKind.OpenParenToken);
const openParenPosition = scanner.getTokenPos();
const openParenParsed = parseExpected(SyntaxKind.OpenParenToken);
const expression = allowInAnd(parseExpression);
parseExpected(SyntaxKind.CloseParenToken);
parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenParsed, openParenPosition);
const thenStatement = parseStatement();
const elseStatement = parseOptional(SyntaxKind.ElseKeyword) ? parseStatement() : undefined;
return withJSDoc(finishNode(factory.createIfStatement(expression, thenStatement, elseStatement), pos), hasJSDoc);
Expand All @@ -5997,9 +6004,10 @@ namespace ts {
parseExpected(SyntaxKind.DoKeyword);
const statement = parseStatement();
parseExpected(SyntaxKind.WhileKeyword);
parseExpected(SyntaxKind.OpenParenToken);
const openParenPosition = scanner.getTokenPos();
const openParenParsed = parseExpected(SyntaxKind.OpenParenToken);
const expression = allowInAnd(parseExpression);
parseExpected(SyntaxKind.CloseParenToken);
parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenParsed, openParenPosition);

// From: https://mail.mozilla.org/pipermail/es-discuss/2011-August/016188.html
// 157 min --- All allen at wirfs-brock.com CONF --- "do{;}while(false)false" prohibited in
Expand All @@ -6013,9 +6021,10 @@ namespace ts {
const pos = getNodePos();
const hasJSDoc = hasPrecedingJSDocComment();
parseExpected(SyntaxKind.WhileKeyword);
parseExpected(SyntaxKind.OpenParenToken);
const openParenPosition = scanner.getTokenPos();
const openParenParsed = parseExpected(SyntaxKind.OpenParenToken);
const expression = allowInAnd(parseExpression);
parseExpected(SyntaxKind.CloseParenToken);
parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenParsed, openParenPosition);
const statement = parseStatement();
return withJSDoc(finishNode(factory.createWhileStatement(expression, statement), pos), hasJSDoc);
}
Expand Down Expand Up @@ -6091,9 +6100,10 @@ namespace ts {
const pos = getNodePos();
const hasJSDoc = hasPrecedingJSDocComment();
parseExpected(SyntaxKind.WithKeyword);
parseExpected(SyntaxKind.OpenParenToken);
const openParenPosition = scanner.getTokenPos();
const openParenParsed = parseExpected(SyntaxKind.OpenParenToken);
const expression = allowInAnd(parseExpression);
parseExpected(SyntaxKind.CloseParenToken);
parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenParsed, openParenPosition);
const statement = doInsideOfContext(NodeFlags.InWithStatement, parseStatement);
return withJSDoc(finishNode(factory.createWithStatement(expression, statement), pos), hasJSDoc);
}
Expand Down Expand Up @@ -7398,7 +7408,7 @@ namespace ts {
if (lastError && lastError.code === Diagnostics._0_expected.code) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_to_match_the_token_here)
createDetachedDiagnostic(fileName, openBracePosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, "{", "}")
);
}
}
Expand Down Expand Up @@ -8453,13 +8463,9 @@ namespace ts {
hasChildren = true;
if (child.kind === SyntaxKind.JSDocTypeTag) {
if (childTypeTag) {
parseErrorAtCurrentToken(Diagnostics.A_JSDoc_typedef_comment_may_not_contain_multiple_type_tags);
const lastError = lastOrUndefined(parseDiagnostics);
const lastError = parseErrorAtCurrentToken(Diagnostics.A_JSDoc_typedef_comment_may_not_contain_multiple_type_tags);
if (lastError) {
addRelatedInfo(
lastError,
createDetachedDiagnostic(fileName, 0, 0, Diagnostics.The_tag_was_first_specified_here)
);
addRelatedInfo(lastError, createDetachedDiagnostic(fileName, 0, 0, Diagnostics.The_tag_was_first_specified_here));
}
break;
}
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/constructorWithIncompleteTypeAnnotation.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts(261,1): error TS
if (retValue != 0 ^= {
~~
!!! error TS1005: ')' expected.
!!! related TS1007 tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts:22:20: The parser expected to find a ')' to match the '(' token here.
~


Expand Down
3 changes: 1 addition & 2 deletions tests/baselines/reference/errorRecoveryWithDotFollowedByNamespaceKeyword.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,4 @@ tests/cases/compiler/errorRecoveryWithDotFollowedByNamespaceKeyword.ts(9,2): err
}

!!! error TS1005: '}' expected.
!!! related TS1007 tests/cases/compiler/errorRecoveryWithDotFollowedByNamespaceKeyword.ts:3:19: The parser expected to find a '}' to match the '{' token here.
!!! related TS1007 tests/cases/compiler/errorRecoveryWithDotFollowedByNamespaceKeyword.ts:2:20: The parser expected to find a '}' to match the '{' token here.
!!! related TS1007 tests/cases/compiler/errorRecoveryWithDotFollowedByNamespaceKeyword.ts:3:19: The parser expected to find a '}' to match the '{' token here.
3 changes: 1 addition & 2 deletions tests/baselines/reference/jsonParserRecovery/JSX.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,4 @@ JSX(15,10): error TS1005: '}' expected.
</div>
)

!!! error TS1005: '}' expected.
!!! related TS1007 JSX:4:9: The parser expected to find a '}' to match the '{' token here.
!!! error TS1005: '}' expected.
3 changes: 1 addition & 2 deletions tests/baselines/reference/jsonParserRecovery/TypeScript_code.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,4 @@ TypeScript code(1,22): error TS1005: '}' expected.
~~~~
!!! error TS1012: Unexpected token.

!!! error TS1005: '}' expected.
!!! related TS1007 TypeScript code:1:18: The parser expected to find a '}' to match the '{' token here.
!!! error TS1005: '}' expected.
3 changes: 1 addition & 2 deletions tests/baselines/reference/jsonParserRecovery/trailing_identifier.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,4 @@ trailing identifier(1,8): error TS1005: '}' expected.
~~~~
!!! error TS1012: Unexpected token.

!!! error TS1005: '}' expected.
!!! related TS1007 trailing identifier:1:4: The parser expected to find a '}' to match the '{' token here.
!!! error TS1005: '}' expected.
8 changes: 8 additions & 0 deletions tests/baselines/reference/missingCloseBracketInArray.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
tests/cases/compiler/missingCloseBracketInArray.ts(1,48): error TS1005: ']' expected.


==== tests/cases/compiler/missingCloseBracketInArray.ts (1 errors) ====
var alphas:string[] = alphas = ["1","2","3","4"

!!! error TS1005: ']' expected.
!!! related TS1007 tests/cases/compiler/missingCloseBracketInArray.ts:1:32: The parser expected to find a ']' to match the '[' token here.
5 changes: 5 additions & 0 deletions tests/baselines/reference/missingCloseBracketInArray.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//// [missingCloseBracketInArray.ts]
var alphas:string[] = alphas = ["1","2","3","4"

//// [missingCloseBracketInArray.js]
var alphas = alphas = ["1", "2", "3", "4"];
5 changes: 5 additions & 0 deletions tests/baselines/reference/missingCloseBracketInArray.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
=== tests/cases/compiler/missingCloseBracketInArray.ts ===
var alphas:string[] = alphas = ["1","2","3","4"
>alphas : Symbol(alphas, Decl(missingCloseBracketInArray.ts, 0, 3))
>alphas : Symbol(alphas, Decl(missingCloseBracketInArray.ts, 0, 3))

11 changes: 11 additions & 0 deletions tests/baselines/reference/missingCloseBracketInArray.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/compiler/missingCloseBracketInArray.ts ===
var alphas:string[] = alphas = ["1","2","3","4"
>alphas : string[]
>alphas = ["1","2","3","4" : string[]
>alphas : string[]
>["1","2","3","4" : string[]
>"1" : "1"
>"2" : "2"
>"3" : "3"
>"4" : "4"

32 changes: 32 additions & 0 deletions tests/baselines/reference/missingCloseParenStatements.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
tests/cases/compiler/missingCloseParenStatements.ts(2,26): error TS1005: ')' expected.
tests/cases/compiler/missingCloseParenStatements.ts(4,5): error TS1005: ')' expected.
tests/cases/compiler/missingCloseParenStatements.ts(8,39): error TS1005: ')' expected.
tests/cases/compiler/missingCloseParenStatements.ts(11,35): error TS1005: ')' expected.


==== tests/cases/compiler/missingCloseParenStatements.ts (4 errors) ====
var a1, a2, a3 = 0;
if ( a1 && (a2 + a3 > 0) {
~
!!! error TS1005: ')' expected.
!!! related TS1007 tests/cases/compiler/missingCloseParenStatements.ts:2:4: The parser expected to find a ')' to match the '(' token here.
while( (a2 > 0) && a1
{
~
!!! error TS1005: ')' expected.
!!! related TS1007 tests/cases/compiler/missingCloseParenStatements.ts:3:10: The parser expected to find a ')' to match the '(' token here.
do {
var i = i + 1;
a1 = a1 + i;
with ((a2 + a3 > 0) && a1 {
~
!!! error TS1005: ')' expected.
!!! related TS1007 tests/cases/compiler/missingCloseParenStatements.ts:8:18: The parser expected to find a ')' to match the '(' token here.
console.log(x);
}
} while (i < 5 && (a1 > 5);
~
!!! error TS1005: ')' expected.
!!! related TS1007 tests/cases/compiler/missingCloseParenStatements.ts:11:17: The parser expected to find a ')' to match the '(' token here.
}
}
28 changes: 28 additions & 0 deletions tests/baselines/reference/missingCloseParenStatements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//// [missingCloseParenStatements.ts]
var a1, a2, a3 = 0;
if ( a1 && (a2 + a3 > 0) {
while( (a2 > 0) && a1
{
do {
var i = i + 1;
a1 = a1 + i;
with ((a2 + a3 > 0) && a1 {
console.log(x);
}
} while (i < 5 && (a1 > 5);
}
}

//// [missingCloseParenStatements.js]
var a1, a2, a3 = 0;
if (a1 && (a2 + a3 > 0)) {
while ((a2 > 0) && a1) {
do {
var i = i + 1;
a1 = a1 + i;
with ((a2 + a3 > 0) && a1) {
console.log(x);
}
} while (i < 5 && (a1 > 5));
}
}
Loading