Improve consistency in our parser code. #65508

CyrusNajmabadi · 2022-11-18T20:10:15Z

Followup to #65480.

Should be reviewed with whitespace off.

Our parser code is heavily inconsistent from one method to the next. This makes understanding and maintenance more difficult, and makes it easier for bugs to creep in. New code doesn't have clear patterns to follow to keep things consistent. Note that this is not stylistic, but rather patterns that help ensure clarity and correctness.

Comments in-line in the major consistency efforts made in teh code.

CyrusNajmabadi · 2022-11-18T20:12:44Z

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

+            return _syntaxFactory.AttributeList(
+                openBracket,
+                attrLocation,
+                _pool.ToListAndFree(attributes),


instead of try/finally that we use so that we can just return things directly without having an intermediary cleanup step, we instead have a clean pattern we use most of hte time to just ask the pool to free the list once we are done adding to it.

CyrusNajmabadi · 2022-11-18T20:15:17Z

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

+                openBracket,
+                attrLocation,
+                _pool.ToListAndFree(attributes),
+                this.EatToken(SyntaxKind.CloseBracketToken));


as much as possible, parsing and creation of hte nodes tries to follow the design of a simple recursive descent parser. e.g. a construct like X -> A B C should be parsed as return new X(ParseA(), ParseB(), ParseC()). This makes the tree structure and recursive nature readily apparent, and gives no opportunity for confusing state tracking or the possibility for other complex code to run in between making htings more difficult to understand.

Note: there is more we can do here. For example, our list-parsing today always requires statement forms to get the list. This can be improved so even with complex syntactic constructs, we can model things as simple recursive descent for all portions of hte node. Leaving that for the future though as that will be a much larger change.

CyrusNajmabadi · 2022-11-18T20:15:59Z

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

-            return _syntaxFactory.Attribute(name, argList);
+            return _syntaxFactory.Attribute(
+                this.ParseQualifiedName(),
+                this.ParseAttributeArgumentList());


note: while there are many fixups like this in teh code, looking through, the majority of the existing constructs already follow the new form, so this is just brining things in line with a proven approach that has the benefits listed in teh prior comments.

CyrusNajmabadi · 2022-11-18T20:17:03Z

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

            }
+
+            return _syntaxFactory.BaseList(colon, _pool.ToListAndFree(list));


7 lines to 1 simple idiomatic line

CyrusNajmabadi · 2022-11-18T20:17:52Z

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

-                this.CurrentToken.Kind == SyntaxKind.ImplicitKeyword ||
-                this.CurrentToken.Kind == SyntaxKind.ExplicitKeyword ||
-                this.CurrentToken.Kind == SyntaxKind.OperatorKeyword;
+            return this.CurrentToken.Kind is SyntaxKind.ImplicitKeyword or SyntaxKind.ExplicitKeyword or SyntaxKind.OperatorKeyword;


patterns make kind checks much simpler (And faster to boot, though likely in a way that is utterly negligible). The readability here is much better though.

CyrusNajmabadi · 2022-11-18T20:19:18Z

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

@@ -4024,42 +3960,39 @@ private AccessorListSyntax ParseAccessorList(bool isEvent)
            {
                // parse property accessors
                var builder = _pool.Allocate<AccessorDeclarationSyntax>();
-                try


above we do the pattern of default(SyntaxList<AccessorDeclarationSyntax>) and then lazily creating the builder. i'm super sketical this is worth it. but i didn't want to change that as i didn't want to accidentally cause a regression. I'll do a perf analysis of such a change in a spearate change in the future.

CyrusNajmabadi · 2022-11-18T20:19:30Z

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

@@ -4270,7 +4203,7 @@ private enum PostSkipAction
                    nodes.Add(token);
                }

-                trailingTrivia = (nodes.Count > 0) ? nodes.ToListNode() : null;
+                trailingTrivia = nodes.ToListNode();


ToListNode() already does the Count check.

CyrusNajmabadi · 2022-11-18T20:21:11Z

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

            }
            finally
            {
                _termState = saveTerm;
-                _pool.Free(variables);


the saving/restoring of _termState is something i'd like a helper for. But it may involve lambda callbacks, so i didn't want to make it without a plan to test perf.

Why lambda callbacks? I imagine a disposable ref struct holding a ref to _termState.

the lambda callback would be for the code to execute inbetween the termState change/restoration.

Couldn't using block be used instead? Like the following in this particular case:

using (new Temp(ref _termState, _termState | TerminatorState.IsEndOfFieldDeclaration)) { var variables = _pool.AllocateSeparated<VariableDeclaratorSyntax>(); this.ParseVariableDeclarators(type, VariableFlags.Fixed, variables, parentKind); return _syntaxFactory.FieldDeclaration( attributes, modifiers.ToList(), _syntaxFactory.VariableDeclaration(type, _pool.ToListAndFree(variables)), this.EatToken(SyntaxKind.SemicolonToken)); }

yes, it could. but that means each case has to repeat teh same pattern. The idea would be to encapsulte the pattern in a helper that itself takes a callback for the work that is specific to that grammar production :)

CyrusNajmabadi · 2022-11-18T20:22:37Z

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

-            return _syntaxFactory.TupleElement(type, name);
+            return _syntaxFactory.TupleElement(
+                ParseType(),
+                IsTrueIdentifier() ? this.ParseIdentifierToken() : null);


use of ternaries allows us to model constructs like X -> A B? C giving the same nice construction/recursive-descent parity/clarity. We use this in many other places, so this is just making things consistent here.

CyrusNajmabadi · 2022-11-18T20:23:32Z

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

+                    semi2,
+                    _pool.ToListAndFree(incrementors),
+                    this.EatToken(SyntaxKind.CloseParenToken),
+                    ParseEmbeddedStatement());
            }
            finally
            {
                _termState = saveTerm;
                this.Release(ref resetPoint);


i want to make it possible to use a simple using statement for reset-points in the future. Needing a try/finally to simulate this feels silly.

CyrusNajmabadi · 2022-11-18T20:24:09Z

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

                    semicolon);
            }
            finally
            {
-                _pool.Free(variables);
                _pool.Free(mods);


currently, there's no helper for SyntaxTokenLists, but i want to add one to clean this up as well.

CyrusNajmabadi · 2022-11-18T20:26:15Z

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

@@ -12611,11 +12477,11 @@ private AnonymousObjectCreationExpressionSyntax ParseAnonymousTypeExpression()
            var openBrace = this.EatToken(SyntaxKind.OpenBraceToken);
            var expressions = _pool.AllocateSeparated<AnonymousObjectMemberDeclaratorSyntax>();
            this.ParseAnonymousTypeMemberInitializers(ref openBrace, ref expressions);
-            var closeBrace = this.EatToken(SyntaxKind.CloseBraceToken);
-            var result = _syntaxFactory.AnonymousObjectCreationExpression(@new, openBrace, expressions, closeBrace);
-            _pool.Free(expressions);


an example of needing to break things up over multiple statements to free things. Both this pattern and thr try/finally pattern go away with the simple ToListAndFree pattern that already existed.

CyrusNajmabadi · 2022-11-28T17:48:03Z

@jcouv this is ready for review.

jcouv

LGTM Thanks (iteration 6)

CyrusNajmabadi added 2 commits November 18, 2022 11:49

Improve parsing consistency

b66d822

Simplify list cleanup

f76a01f

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

CyrusNajmabadi commented Nov 18, 2022

View reviewed changes

Simplify

ae70ea2

CyrusNajmabadi commented Nov 18, 2022

View reviewed changes

Simplify

ec13090

CyrusNajmabadi commented Nov 18, 2022

View reviewed changes

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

4c64f68

CyrusNajmabadi marked this pull request as ready for review November 19, 2022 02:31

CyrusNajmabadi requested a review from a team as a code owner November 19, 2022 02:31

CyrusNajmabadi requested a review from jcouv November 19, 2022 02:32

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

351d492

CyrusNajmabadi requested a review from 333fred November 21, 2022 17:37

jcouv self-assigned this Nov 28, 2022

jcouv approved these changes Nov 29, 2022

View reviewed changes

CyrusNajmabadi requested a review from RikkiGibson November 29, 2022 00:26

333fred approved these changes Nov 29, 2022

View reviewed changes

CyrusNajmabadi merged commit d40add2 into dotnet:main Nov 29, 2022

CyrusNajmabadi deleted the parsingConsistency branch November 29, 2022 01:26

ghost added this to the Next milestone Nov 29, 2022

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

allisonchou mentioned this pull request Dec 2, 2022

[Automated] PRs inserted in VS build main-33201.295 #65718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve consistency in our parser code. #65508

Improve consistency in our parser code. #65508

CyrusNajmabadi commented Nov 18, 2022 •

edited

Loading

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

jjonescz Nov 29, 2022 •

edited

Loading

CyrusNajmabadi Nov 29, 2022

jjonescz Nov 29, 2022

CyrusNajmabadi Nov 29, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi Nov 18, 2022

CyrusNajmabadi commented Nov 28, 2022

jcouv left a comment

		}

		return _syntaxFactory.BaseList(colon, _pool.ToListAndFree(list));

Improve consistency in our parser code. #65508

Improve consistency in our parser code. #65508

Conversation

CyrusNajmabadi commented 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

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

jjonescz Nov 29, 2022 • 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

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 28, 2022

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 18, 2022 •

edited

Loading

jjonescz Nov 29, 2022 •

edited

Loading