Move context-sensitive parsing to context-free parsing. #65480

CyrusNajmabadi · 2022-11-17T06:20:51Z

No description provided.

CyrusNajmabadi · 2022-11-17T06:22:14Z

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

@@ -8760,10 +8759,6 @@ private bool IsPossibleStatement(bool acceptAccessibilityMods)
                case SyntaxKind.IdentifierToken:
                    return IsTrueIdentifier();

-                case SyntaxKind.CatchKeyword:
-                case SyntaxKind.FinallyKeyword:
-                    return !_isInTry;


this was actually highly undesirable in the parser. it effectively made the handling of blocks, and 'catch/finally' tokens context-sensitive. While unlikely to cause bad issues, it means you could end up in some sort of wonky incremental scenario where something like a block might move inside/outside of a try, and get reused, despite the context not being the same (and thus meaning that non-incremental parsing would produce a different tree).

CyrusNajmabadi · 2022-11-17T06:23:04Z

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

@@ -4114,24 +4114,39 @@ public static int Main()
 ";
            // Extra Errors
            ParseAndValidate(test,
-    // (12,13): error CS1524: Expected catch or finally
-    //             }
-    Diagnostic(ErrorCode.ERR_ExpectedEndTry, "}"),


this went away because the catch already has syntax errors, so the later error is suppressed now.

CyrusNajmabadi · 2022-11-17T06:23:47Z

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

@@ -14038,15 +14025,13 @@ private new ResetPoint GetResetPoint()
            return new ResetPoint(
                base.GetResetPoint(),
                _termState,
-                _isInTry,


removing context we used to have save/restore (but didn't store within nodes (like we do with our other contextual flags)).

CyrusNajmabadi · 2022-11-17T06:24:14Z

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

@@ -32,7 +32,6 @@ internal partial class LanguageParser : SyntaxParser

        private int _recursionDepth;
        private TerminatorState _termState; // Resettable
-        private bool _isInTry; // Resettable
        private bool _checkedTopLevelStatementsFeatureAvailability; // Resettable


this flag is likely bad as well.

(it is, and it is removed in my sibling pr)

CyrusNajmabadi · 2022-11-17T07:32:26Z

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

@@ -32,7 +32,6 @@ internal partial class LanguageParser : SyntaxParser

        private int _recursionDepth;
        private TerminatorState _termState; // Resettable
-        private bool _isInTry; // Resettable


less contextual state (explained below why thisi s bad).

CyrusNajmabadi · 2022-11-17T07:33:11Z

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

@@ -14096,5 +14086,44 @@ private new struct ResetPoint
            node = this.AddTrailingSkippedSyntax(node, trailingTrash.Node);
            return node;
        }
+
+        private static bool ContainsErrorDiagnostic(GreenNode node)


copied out from the other PR. if this goes in first, i'll unify that one on his.

The current diff shows this being deleted from above and added here.

CyrusNajmabadi · 2022-11-18T04:36:59Z

Tagging @cston as well.

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

jcouv

Done with review pass (iteration 5). Much like the simplification! Minor comment only.

jcouv · 2022-11-18T18:58:11Z

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

@@ -8840,10 +8800,6 @@ private bool IsPossibleStatement(bool acceptAccessibilityMods)
                case SyntaxKind.IdentifierToken:
                    return IsTrueIdentifier();

-                case SyntaxKind.CatchKeyword:
-                case SyntaxKind.FinallyKeyword:
-                    return !_isInTry;


nit: If this is still called on those keywords, consider handling catch and finally explicitly and returning true for clarity
Otherwise, consider asserting/throwing #Closed

on the fence... this def can be called on those (this is IsPossibleStatement). but explicitly hitting them and stating that they are not statements (which is what we want so that higher up try-statements actually will consume error catches/finallys into them), seems inverted from how this method generally works. specifically that it says the cases that def are statements, not hte cases that are not.

I would be ok with having these there commented otu saying why we explicitly do not consider them the start of statements.

Let's leave as-is then

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

jcouv

LGTM Thanks (iteration 10)

CyrusNajmabadi · 2022-11-18T20:43:48Z

@cston ptal if available. Thanks!

333fred · 2022-11-18T22:04:32Z

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

@@ -14096,5 +14086,44 @@ private new struct ResetPoint
            node = this.AddTrailingSkippedSyntax(node, trailingTrash.Node);
            return node;
        }
+
+        private static bool ContainsErrorDiagnostic(GreenNode node)


The current diff shows this being deleted from above and added here.

CyrusNajmabadi · 2022-11-18T22:18:52Z

The current diff shows this being deleted from above and added here.

It was added in multiple different PRs. This just removes the dupe when they merged together :-)

jcouv · 2022-11-18T22:42:48Z

Please remember to squash compiler PRs

CyrusNajmabadi · 2022-11-18T23:05:17Z

Sorry! My bad. :-(

CyrusNajmabadi added 2 commits November 16, 2022 21:58

Move context-sensitive parsing to context-free

2261a87

restore

5148cf8

CyrusNajmabadi requested a review from a team as a code owner November 17, 2022 06:20

dotnet-issue-labeler bot added the Area-Compilers label Nov 17, 2022

CyrusNajmabadi commented Nov 17, 2022

View reviewed changes

CyrusNajmabadi requested a review from jcouv November 17, 2022 06:24

CyrusNajmabadi added 2 commits November 16, 2022 23:15

Reduce errors

f9e3c3f

Reduce errors

fd41e94

CyrusNajmabadi commented Nov 17, 2022

View reviewed changes

jcouv self-assigned this Nov 17, 2022

CyrusNajmabadi requested a review from 333fred November 18, 2022 18:01

CyrusNajmabadi added 4 commits November 18, 2022 10:02

Merge remote-tracking branch 'upstream/main' into tryParsing

c203ed7

Merge remote-tracking branch 'upstream/main' into tryParsing

bab636b

Remove dupe

c5cc14b

Rename local and add comment

5f82703

jcouv reviewed Nov 18, 2022

View reviewed changes

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

jcouv reviewed Nov 18, 2022

View reviewed changes

Add braces back

2102100

jcouv reviewed Nov 18, 2022

View reviewed changes

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

jcouv reviewed Nov 18, 2022

View reviewed changes

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

remove unnecessary helper

dc8e601

CyrusNajmabadi force-pushed the tryParsing branch from fcd80b6 to dc8e601 Compare November 18, 2022 19:03

CyrusNajmabadi requested a review from jcouv November 18, 2022 19:06

jcouv approved these changes Nov 18, 2022

View reviewed changes

CyrusNajmabadi mentioned this pull request Nov 18, 2022

Improve consistency in our parser code. #65508

Merged

333fred approved these changes Nov 18, 2022

View reviewed changes

CyrusNajmabadi merged commit ccf9a90 into dotnet:main Nov 18, 2022

CyrusNajmabadi deleted the tryParsing branch November 18, 2022 22:19

ghost added this to the Next milestone Nov 18, 2022

allisonchou mentioned this pull request Nov 24, 2022

[Automated] PRs inserted in VS build main-33123.395 #65599

Closed

allisonchou modified the milestones: Next, 17.5 P2 Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move context-sensitive parsing to context-free parsing. #65480

Move context-sensitive parsing to context-free parsing. #65480

CyrusNajmabadi commented Nov 17, 2022

CyrusNajmabadi Nov 17, 2022

CyrusNajmabadi Nov 17, 2022

CyrusNajmabadi Nov 17, 2022

CyrusNajmabadi Nov 17, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 17, 2022

CyrusNajmabadi Nov 17, 2022

333fred Nov 18, 2022

CyrusNajmabadi commented Nov 18, 2022

jcouv left a comment

jcouv Nov 18, 2022 •

edited

Loading

CyrusNajmabadi Nov 18, 2022

jcouv Nov 18, 2022

jcouv left a comment

CyrusNajmabadi commented Nov 18, 2022

333fred Nov 18, 2022

CyrusNajmabadi commented Nov 18, 2022

jcouv commented Nov 18, 2022

CyrusNajmabadi commented Nov 18, 2022

Move context-sensitive parsing to context-free parsing. #65480

Move context-sensitive parsing to context-free parsing. #65480

Conversation

CyrusNajmabadi commented Nov 17, 2022

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

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 18, 2022

jcouv left a comment

Choose a reason for hiding this comment

jcouv Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 18, 2022

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 18, 2022

jcouv commented Nov 18, 2022

CyrusNajmabadi commented Nov 18, 2022

jcouv Nov 18, 2022 •

edited

Loading