Improve error recovery when encountering errant close-paren tokens. #69482

CyrusNajmabadi · 2023-08-12T02:46:35Z

Our recursive parsing system was errantly adding close-paren tokens into the set of tokens used to determine when to bail out of a scope. This is not an appropriate use of that system as it should only look for tokens known to absolutely terminate a construct, or tokens known to absolutely start another sibling or parent construct.

Fixed this to no longer be part of the error recovery system, and instead just treat the close-paren specially for the 3 parsing constructs that cared about it.

Note: the majority of this PR is deletions. THe addition of a large test though makes it seem otherwise.

CyrusNajmabadi · 2023-08-12T02:49:11Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -8406,7 +8412,7 @@ private FixedStatementSyntax ParseFixedStatement(SyntaxList<AttributeListSyntax>

            var saveTerm = _termState;
            _termState |= TerminatorState.IsEndOfFixedStatement;
-            var decl = ParseVariableDeclaration();


renamed this to make it clear which caller contexts it is expected to be used in. The old name was highly misleading and could make people think they should they should use this in any circumstance they want to parse a variable-declaration (like in a field for example).

importantly, all of these are the constructs that do want to stop parsing the variable declaration once they hit an errant close paren, as their construct themselves want to consume the close paren into themselves.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

CyrusNajmabadi · 2023-08-12T02:52:55Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                else if (stopOnCloseParen && this.CurrentToken.Kind == SyntaxKind.CloseParenToken)
+                {
+                    break;
+                }


this is half of the fix. Callers who are parsing a list of declarators say how close parens should be treated.

CyrusNajmabadi · 2023-08-12T02:53:14Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -9784,7 +9800,6 @@ private bool IsEndOfDeclarationClause()
            switch (this.CurrentToken.Kind)
            {
                case SyntaxKind.SemicolonToken:
-                case SyntaxKind.CloseParenToken:


this is the other half. we don't mix 'close paren' into the 'bail out set' for error recovery.

CyrusNajmabadi · 2023-08-12T02:53:52Z

src/Compilers/CSharp/Test/Syntax/Parsing/AnonymousFunctionParsingTests.cs

@@ -2034,33 +2034,21 @@ public void StaticLambdaFunctionPointer()
                Diagnostic(ErrorCode.ERR_SemicolonExpected, "{").WithLocation(1, 58));


view with whitespace off.

…/roslyn into closeParenParsing

CyrusNajmabadi · 2023-08-12T02:54:47Z

src/Compilers/CSharp/Test/Syntax/Parsing/AnonymousFunctionParsingTests.cs

-                // (1,32): error CS1002: ; expected
-                // delegate*<void> ptr = &static () => { };
-                Diagnostic(ErrorCode.ERR_SemicolonExpected, ")").WithLocation(1, 32));
+                Diagnostic(ErrorCode.ERR_SyntaxError, "static").WithArguments(",").WithLocation(1, 24));


virtually all cases got better. with many fewer error recovery errors than before.

CyrusNajmabadi · 2023-08-12T02:55:13Z

src/Compilers/CSharp/Test/Syntax/Parsing/AnonymousFunctionParsingTests.cs

@@ -2103,7 +2091,7 @@ void verify()
                            }
                        }
                    }
-                    M(SyntaxKind.SemicolonToken);
+                    N(SyntaxKind.SemicolonToken);


Lots less missing tokens as error recovery now does a better job not preemptively bailing out.

CyrusNajmabadi · 2023-08-12T16:24:40Z

@dotnet/roslyn-compiler ptal. This helps a lot of situations where a simple errant close paren can cause huge cascading failures in parsing.

CyrusNajmabadi · 2023-08-12T16:24:50Z

@RikkiGibson @cston ptal.

CyrusNajmabadi · 2023-08-12T16:39:03Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -4528,22 +4528,11 @@ private void ParseParameterModifiers(SyntaxListBuilder modifiers, bool isFunctio

            var type = this.ParseType();

-            var saveTerm = _termState;
-            _termState |= TerminatorState.IsEndOfFieldDeclaration;


moved this into the single ParseFieldDeclarationVariableDeclarators helper.

CyrusNajmabadi · 2023-08-12T16:39:28Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-            var variables = _pool.AllocateSeparated<VariableDeclaratorSyntax>();
-            try
-            {
-                this.ParseVariableDeclarators(type, VariableFlags.Fixed, variables, parentKind);


renamed ParseVariableDeclarators to ParseFieldDeclarationVariableDeclarators since that's the only case it is used for. this allowed a lot of simplification.

CyrusNajmabadi · 2023-08-12T16:41:40Z

src/Compilers/CSharp/Test/Syntax/Parsing/CollectionExpressionParsingTests.cs

-            // (1,18): error CS1022: Type or namespace definition, or end-of-file expected
-            // [return: A, B].C();
-            Diagnostic(ErrorCode.ERR_EOFExpected, ")").WithLocation(1, 18));
+            Diagnostic(ErrorCode.ERR_SyntaxError, "]").WithArguments(",").WithLocation(1, 14));


we recover much faster in practically all cases now. no cases get worse. a small handful stay around the same.

CyrusNajmabadi · 2023-08-12T16:42:16Z

src/Compilers/CSharp/Test/Syntax/Parsing/CollectionExpressionParsingTests.cs

-            N(SyntaxKind.GlobalStatement);
-            {
-                N(SyntaxKind.EmptyStatement);
-                {
                    N(SyntaxKind.SemicolonToken);


nothing like thinking you're missing a semicolon, only to be immediatley followed by an unmatched semicolon that makes its own statement.

it's nice to see missing tokens "restored" and a cleaner parse.

CyrusNajmabadi · 2023-08-12T16:43:42Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

-                            {
-                                N(SyntaxKind.IdentifierToken, "M");
-                            }
-                        }


old code basically tried to guess what was going on after the ), getting is quite wrong. new paths basically go into error recovery and state that we can't handle things until we get to the next semicolon.

this also means a lot less broken constructs and a lot less error messages in these bogus cases.

CyrusNajmabadi · 2023-08-14T17:12:50Z

@RikkiGibson @cston ptal.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIsPatternExpression.cs

RikkiGibson · 2023-08-14T22:36:11Z

src/Compilers/CSharp/Test/Syntax/Parsing/CollectionExpressionParsingTests.cs

-            N(SyntaxKind.GlobalStatement);
-            {
-                N(SyntaxKind.EmptyStatement);
-                {
                    N(SyntaxKind.SemicolonToken);


it's nice to see missing tokens "restored" and a cleaner parse.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

Co-authored-by: Rikki Gibson <[email protected]>

CyrusNajmabadi · 2023-08-16T14:54:09Z

@jjonescz @cston ptal :)

cston · 2023-08-16T18:36:03Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                _syntaxFactory.VariableDeclaration(type, _pool.ToListAndFree(variables)),
+                _syntaxFactory.VariableDeclaration(
+                    type,
+                    this.ParseFieldDeclarationVariableDeclarators(type, VariableFlags.Const, parentKind)),


ParseFieldDeclarationVariableDeclarators

It looks like ParseConstantFieldDeclaration() is now indirectly saving and restoring _termState when parsing variable declarators. What is the impact of that change?

honestly, it's likely better there so that parsing a constant field is the same as parsing a normal field. TBH, it's kinda strange that we even have this method instead of just consuming 'const' in the caller and calling into normal field decl parsing.

The only real difference here is that ; is mixed into the terminator tokens for this construct. but that's totally fine and desirable for constant fields.

CyrusNajmabadi and others added 4 commits August 11, 2023 18:04

Fix error recovery with close parentheses

889c141

Merge branch 'main' into closeParenParsing

ecfeca7

Updat etests

86f7461

IMprove error recovery

8e04b38

CyrusNajmabadi requested a review from a team as a code owner August 12, 2023 02:46

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 12, 2023

CyrusNajmabadi commented Aug 12, 2023

View reviewed changes

Add comment

c7fb88a

CyrusNajmabadi commented Aug 12, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

f4a7933

CyrusNajmabadi commented Aug 12, 2023

View reviewed changes

CyrusNajmabadi added 2 commits August 11, 2023 19:54

Cleanup

a2eb4e6

Merge branch 'closeParenParsing' of https://github.com/CyrusNajmabadi…

d9f2288

…/roslyn into closeParenParsing

CyrusNajmabadi commented Aug 12, 2023

View reviewed changes

CyrusNajmabadi added 4 commits August 12, 2023 09:13

Fix test

b005cac

Fix test

b0476bb

Fix test

ced5866

Fix test

618237c

CyrusNajmabadi requested a review from a team as a code owner August 12, 2023 16:23

CyrusNajmabadi mentioned this pull request Aug 12, 2023

Report less errors on bad closing parentheses inside a local function #69479

Closed

CyrusNajmabadi added 3 commits August 12, 2023 09:27

Add comments

4d0643e

Simplify

d447f34

Simplify

a183b14

CyrusNajmabadi commented Aug 12, 2023

View reviewed changes

CyrusNajmabadi added 4 commits August 12, 2023 09:52

Docs

c0e464e

Multiple new lines

8f0c497

Fix test

2fbbfde

Remove

494edb9

RikkiGibson self-assigned this Aug 14, 2023

RikkiGibson approved these changes Aug 14, 2023

View reviewed changes

Rename methods

624f4ff

RikkiGibson reviewed Aug 14, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

d1d98bd

Co-authored-by: Rikki Gibson <[email protected]>

RikkiGibson approved these changes Aug 15, 2023

View reviewed changes

cston reviewed Aug 16, 2023

View reviewed changes

cston approved these changes Aug 16, 2023

View reviewed changes

CyrusNajmabadi merged commit 2b3c173 into dotnet:main Aug 16, 2023
27 checks passed

ghost added this to the Next milestone Aug 16, 2023

CyrusNajmabadi deleted the closeParenParsing branch August 16, 2023 19:08

Rekkonnect mentioned this pull request Aug 17, 2023

Poor error recovery after missing > #24642

Closed

allisonchou mentioned this pull request Aug 22, 2023

[Automated] PRs inserted in VS build main-34021.281 #69652

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error recovery when encountering errant close-paren tokens. #69482

Improve error recovery when encountering errant close-paren tokens. #69482

CyrusNajmabadi commented Aug 12, 2023 •

edited

Loading

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi commented Aug 12, 2023

CyrusNajmabadi commented Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

RikkiGibson Aug 14, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi Aug 12, 2023

CyrusNajmabadi commented Aug 14, 2023

RikkiGibson Aug 14, 2023

CyrusNajmabadi commented Aug 16, 2023

cston Aug 16, 2023

CyrusNajmabadi Aug 16, 2023

		@@ -2034,33 +2034,21 @@ public void StaticLambdaFunctionPointer()
		Diagnostic(ErrorCode.ERR_SemicolonExpected, "{").WithLocation(1, 58));

Improve error recovery when encountering errant close-paren tokens. #69482

Improve error recovery when encountering errant close-paren tokens. #69482

Conversation

CyrusNajmabadi commented Aug 12, 2023 • edited Loading

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

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 12, 2023

CyrusNajmabadi commented Aug 12, 2023

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

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 14, 2023

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 12, 2023 •

edited

Loading