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

jessetrinity · 2021-05-19T04:01:40Z

The parser expects the keywords if, do, while, and with to be followed by pairs of braces({, }) or parentheses ((,)).

It was also attempting to unconditionally add a related diagnostic for missing closing brackets to the diagnostics for missing opening brackets. When the opening bracket did not exist, this typically meant that diagnostic span was being added beyond the EOF.

Here we avoid associating the missing closing bracket diagnostic with the opening bracket diagnostic if we fail to parse the former. The errors resulting from the regression broke smart indentation on the keywords, thus the indentation tests in this change. We don't have the same concern about parseExpectedMatchingBrackets with array and object literals since their parsings are triggered by their opening braces.

Regressed by #40884

jessetrinity · 2021-05-19T04:18:33Z

I'll admit up front that I don't fully understand the chain of events that led from the parser error to smart indentation breaking. While debugging I was seeing Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed); in parser.ts throw when the getIndentation command ran, derailing it along with a handful of other language service command my editor was firing at the time. I assume that the bad diagnostic dirtied up something somewhere else in the parser and getting us into a bad state .-.

DanielRosenwasser · 2021-05-19T19:08:11Z

While debugging I was seeing Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed); in parser.ts throw when the getIndentation command ran, derailing it along with a handful of other language service command my editor was firing at the time. I assume that the bad diagnostic dirtied up something somewhere else in the parser and getting us into a bad state .-.

!!!

I would strongly urge a revert on the commit for 4.3.

sheetalkamat · 2021-05-19T20:29:34Z

src/compiler/parser.ts

            const expression = allowInAnd(parseExpression);
-            parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenPosition);
+            openParenExists ? parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenPosition)


Can you make these if else statements instead.. this makes it very hard to read

sheetalkamat · 2021-05-19T20:34:58Z

src/compiler/parser.ts

            const expression = allowInAnd(parseExpression);
-            parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenPosition);
+            openParenExists ? parseExpectedMatchingBrackets(SyntaxKind.OpenParenToken, SyntaxKind.CloseParenToken, openParenPosition)
+            : parseExpected(SyntaxKind.CloseParenToken);


This is needed in parseObjectLiteralExpression?

I think instead of changing this in all use cases, why not modify parseExpectedMatchingBrackets to take in result of openingBracket so all cases are always covered and we dont need to review all use cases...

I don't think it should be needed for object literal expression (or arrays) since we only try to parse them if an opening bracket has already been parsed (or scanned, not sure what the correct terminology is there).

sandersn

I bet the easiest way to move forward with this PR is to create a new one that starts with a revert of your previous revert PR, then add back a commit with a fix.

src/compiler/parser.ts

This reverts commit 5770434.

sandersn

2 suggestions plus 1 wishful-thinking suggestion

src/compiler/parser.ts

sandersn · 2021-05-28T13:27:50Z

src/compiler/parser.ts

@@ -1552,6 +1555,23 @@ namespace ts {
            return false;
        }

+        function parseExpectedMatchingBrackets(openKind: SyntaxKind, closeKind: SyntaxKind,  openParsed: boolean, openPosition: number) {
+            if (!openParsed) {


If openParsed is false, parseExpected is equivalent to the code before if (lastError).
if openParsed is true, the code before if (lastError) should run.

Equivalent code runs either way, so I'd move the openParsed check after const lastError = ...

If there was an elegant way to change parseExpected to return its error, I'd recommend replacing if (token() = ... through const lastError = ... with const lastError = parseExpected(.... But it's used all over the place with the current true | false return type.

sandersn · 2021-05-28T13:45:05Z

src/compiler/parser.ts

            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);


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

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

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.

sandersn · 2021-06-02T23:29:48Z

tests/baselines/reference/constructorWithIncompleteTypeAnnotation.errors.txt

@@ -121,7 +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.
+!!! related TS1007 tests/cases/compiler/constructorWithIncompleteTypeAnnotation.ts:22:19: The parser expected to find a ')' to match the '(' token here.


Uh-oh, I didn't expect any baseline changes. I counted the spaces and 19 is correct counting from 0; 20 is correct counting from 1. I'm not sure which we do for chars, but I see lines counting from 1 in tests/baselines/reference/jsonParserRecovery/TypeScript_code.errors.txt

I think this change is wrong. Sorry for steering you wrong. =(

jessetrinity requested review from orta and sandersn May 19, 2021 04:01

typescript-bot assigned jessetrinity May 19, 2021

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 19, 2021

jessetrinity changed the title ~~Fix parser regression for bad related diagnostic for matching brackets~~ Fix parser regression for bad related diagnostic on missing matching brackets May 19, 2021

sheetalkamat requested changes May 19, 2021

View reviewed changes

jessetrinity mentioned this pull request May 19, 2021

Revert #43460 and #40884 #44175

Merged

sandersn requested changes May 20, 2021

View reviewed changes

src/compiler/parser.ts Show resolved Hide resolved

src/compiler/parser.ts Outdated Show resolved Hide resolved

jessetrinity added 2 commits May 27, 2021 12:59

Revert "Revert microsoft#43460 and microsoft#40884 (microsoft#44175)"

3cb457a

This reverts commit 5770434.

fix missing opening brace match error

e38a0ce

jessetrinity force-pushed the fixParserBracketMatching branch from 70e8a60 to e38a0ce Compare May 27, 2021 20:16

sandersn requested changes May 28, 2021

View reviewed changes

sandersn added this to Not started in PR Backlog May 28, 2021

sandersn moved this from Not started to Waiting on author in PR Backlog May 28, 2021

refactor parseExpectedMatchingBrackets

fa19d47

jessetrinity force-pushed the fixParserBracketMatching branch from 780102b to fa19d47 Compare June 2, 2021 21:32

jessetrinity added 2 commits June 2, 2021 16:06

use getNodePos

c26543f

accept baselines

0490253

sandersn reviewed Jun 2, 2021

View reviewed changes

sandersn assigned sandersn and unassigned jessetrinity Feb 24, 2022

sandersn added 3 commits March 15, 2022 11:49

Merge branch 'main' into fixParserBracketMatching

4e3ebb2

delete mistakenly added files

f02bd72

Revert getNodePos addition

18a4df4

sandersn approved these changes Mar 16, 2022

View reviewed changes

sandersn merged commit 111ca92 into microsoft:main Mar 16, 2022

PR Backlog automation moved this from Waiting on author to Done Mar 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jessetrinity commented May 19, 2021 •

edited

jessetrinity commented May 19, 2021 •

edited

DanielRosenwasser commented May 19, 2021

sheetalkamat May 19, 2021

sheetalkamat May 19, 2021

jessetrinity May 27, 2021

sandersn left a comment

sandersn left a comment

sandersn May 28, 2021

sandersn May 28, 2021

jessetrinity Jun 2, 2021

sandersn Jun 2, 2021

sandersn Jun 2, 2021

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

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

Conversation

jessetrinity commented May 19, 2021 • edited

jessetrinity commented May 19, 2021 • edited

DanielRosenwasser commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessetrinity commented May 19, 2021 •

edited

jessetrinity commented May 19, 2021 •

edited