Introduce parser helper for parsing separated syntax lists. #66552

CyrusNajmabadi · 2023-01-26T03:30:07Z

No description provided.

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

CyrusNajmabadi · 2023-01-26T19:10:18Z

@RikkiGibson @333fred @dotnet/roslyn-compiler this is ready for review. Should be reviewed with whitespace off.

CyrusNajmabadi · 2023-01-26T19:10:55Z

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

-                p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleAttribute(),
-                p => p.CurrentToken.Kind == SyntaxKind.CloseBracketToken || p.IsTerminator(),
-                expected);
+            PostSkipAction skipBadAttributeListTokens(SeparatedSyntaxListBuilder<AttributeSyntax> list, SyntaxKind expected)


skip functions that were referenced from only single places moved to be local functinos to help keep the list-parsing/skipping logic close in proximity.

CyrusNajmabadi · 2023-01-26T19:11:54Z

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

+                static @this => @this.IsPossibleAttributeArgument(),
+                static @this => @this.ParseAttributeArgument(),
+                static (@this, openParen, argNodes, kind, _) => skipBadAttributeArgumentTokens(@this, openParen, argNodes, kind),
+                allowTrailingSeparator: false);


this is the simplest form of calling into the helper. You pass hte open token in (so errors can be attached to it if necessary), the token that finishes the list, the check if we're on a list element, the way to parse the list element, the function to determine skip/abort behavior, and if trailing separators are legal or not.

CyrusNajmabadi · 2023-01-26T19:12:21Z

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

+                    p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleTypeParameterConstraint(),
+                    p => p.CurrentToken.Kind == SyntaxKind.OpenBraceToken || p.IsCurrentTokenWhereOfConstraintClause() || p.IsTerminator(),
+                    expected);
+            }


moving the skip helpers inside the method that references it prevents drift where other helpers outside the method push tehse further away.

CyrusNajmabadi · 2023-01-26T19:15:10Z

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

+                static @this => @this.ParseExpressionCore(),
+                static (@this, openBrace, list, expectedKind, closeKind) => @this.SkipBadInitializerListTokens(openBrace, list, expectedKind),
+                allowTrailingSeparator: false,
+                trailingSeparatorError: ErrorCode.ERR_ExpressionExpected);


so this is something i think is worth bringing up in the PR. A couple of separated list parsing routines behave subtly different from teh rest. For example, this location reports a specialized error if there is a trailing comma that is unlike every other location that runs into that issue. I've preserved this to keep error messages identical. But, personally, i think it's pointless and we should just unify on the exact same error reporting strategy for all list parsing. specifically, just call into the element-parsing-function and have it report whatever error it normally would in this case.

Changing this behavior revealed just 7 tests taht change in the entire compiler. So i think it's fine to actually update things here, but i wanted to run it by compiler team.

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

CyrusNajmabadi · 2023-01-26T19:16:53Z

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

+                                    separator, MakeError(separator.FullWidth + this.CurrentToken.GetLeadingTriviaWidth(), this.CurrentToken.Width, trailingSeparatorError.Value));
+                                argNodes.AddSeparator(separator);
+                                break;
+                            }


this was one of the special cases.

CyrusNajmabadi · 2023-01-27T18:20:46Z

@RikkiGibson @333fred this is ready for review.

CyrusNajmabadi · 2023-01-27T18:22:51Z

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

+                return @this.SkipBadSeparatedListTokensWithExpectedKind(ref openBracket, list,
+                    static p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleAttribute(),
+                    static (p, closeKind) => p.CurrentToken.Kind == closeKind,
+                    expectedKind, closeKind);
            }


note: we could consider inlining this code into the use site (it's always just a call to @this.SkipBadSeparatedListTokensWithExpectedKind). But that might make the call to ParseCommaSeparatedSyntaxList a little too verbose/unclear. So i'm keeping hte error-recovery code separated out to keep the main code as clear as possible.

CyrusNajmabadi · 2023-01-27T18:24:01Z

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

-            return this.SkipBadSeparatedListTokensWithExpectedKind(ref tmp, list,
-                p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleAttribute(),
-                p => p.CurrentToken.Kind == SyntaxKind.CloseBracketToken || p.IsTerminator(),
-                expected);


of note, the p.IsTerminator() portion is removed. it's always required in all the calls to SkipBadSeparatedListTokensWithExpectedKind, so it just got moved directly into that helper instead.

@RikkiGibson A ton of the calls to SkipBadSeparatedListTokensWithExpectedKind are virtually identical in how they operate. Furthermore, they seem to use the same info that is already passed into ParseCommaSeparatedSyntaxList (like having logic that duplicates what is in isPossibleElement). Further work here could make it so that instead of having to pass in the error-recovery function, you get a default one that bahaves sensibly given all the data taht ParseCommaSeparatedSyntaxList already has.

I kept that out of scope here as i don't know this particular area super well and i wasn't quite sure if this could be done cleanly at the present.

CyrusNajmabadi · 2023-01-27T18:26:47Z

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

+                return this.SkipBadSeparatedListTokensWithExpectedKind(ref colon, list,
+                    static p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleAttribute(),
+                    static (p, _) => p.CurrentToken.Kind == SyntaxKind.OpenBraceToken || p.IsCurrentTokenWhereOfConstraintClause(),
+                    expected);


this is definitely inconsistent in that this guy doesn't pass along closeKind (but instead just inlines the value it knows that would be into its lambda). We should make these more consistent in the future.

Is there a comment indicates this or an issue that tracks doing this?

… into parserHelper

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

333fred · 2023-01-27T22:47:43Z

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

+                else if (skipBadTokens(this, ref openToken, nodes, SyntaxKind.IdentifierToken, closeTokenKind) == PostSkipAction.Continue)
+                {
+                    // Something we didn't recognize, try to skip tokens, reporting that we expected an identifier here.
+                    // While 'identifier' may not be completely accurate in terms of what the list needs, it's a


Worth parameterizing? It might be useful to say "expression" for some of these.

I think we should do it as a separate pass after this pr.

note: i'm interested in this. but i think it should be separte :)

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

CyrusNajmabadi · 2023-01-27T18:41:43Z

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

+                        or SyntaxKind.OpenParenToken
+                        or SyntaxKind.ColonColonToken
+                        or SyntaxKind.DotDotToken
+                        or SyntaxKind.OpenBraceToken);


IDE was unintentionally taking advantage of { ] being paired together in pattern parsing. Not intentional not desirable. So this got tweaked to the new parse this has where the { has a missing } not a converted ] => }.

CyrusNajmabadi · 2023-01-28T00:08:59Z

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

+                else if (skipBadTokens(this, ref openToken, nodes, SyntaxKind.IdentifierToken, closeTokenKind) == PostSkipAction.Continue)
+                {
+                    // Something we didn't recognize, try to skip tokens, reporting that we expected an identifier here.
+                    // While 'identifier' may not be completely accurate in terms of what the list needs, it's a


I think we should do it as a separate pass after this pr.

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

RikkiGibson · 2023-01-28T02:28:28Z

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

+                return this.SkipBadSeparatedListTokensWithExpectedKind(ref colon, list,
+                    static p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleAttribute(),
+                    static (p, _) => p.CurrentToken.Kind == SyntaxKind.OpenBraceToken || p.IsCurrentTokenWhereOfConstraintClause(),
+                    expected);


Is there a comment indicates this or an issue that tracks doing this?

RikkiGibson · 2023-01-28T02:30:26Z

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

+            // If we ever want this function to parse out separated lists with a different separator, we can
+            // parameterize this method on this value.
+            var separatorTokenKind = SyntaxKind.CommaToken;
+            var nodes = _pool.AllocateSeparated<TNode>();


You'd think we'd use a try/finally here. I know that often with pools of managed objects, if the renter loses the object it just gets GC'ed. Maybe we should scrutinize the places where try/finally is used and say, hey if an exception happens in parsing, we don't really care if some random object happens to not get returned to the pool, it's more important to us to e.g. improve inline-ability and so on.

yeah. we don't really care about exceptions here, it's not a normal use case. The reason we use try/finally is primarily because we have code that may early-exit as part of normal parsing. And it's much easier to just use try/finally than have to remember to return to pool on all exit paths.

As this only exists in a single place, it's fine to use this pattern.

RikkiGibson · 2023-01-28T02:33:27Z

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

        }

-        private ExpressionSyntax ParseSwitchExpression(ExpressionSyntax governingExpression, SyntaxToken switchKeyword)
+        private SwitchExpressionSyntax ParseSwitchExpression(ExpressionSyntax governingExpression, SyntaxToken switchKeyword)


I assume you made a deliberate decision to not revise parsing switch expression arms here. Do we have a note somewhere to follow up on it? Assuming it would be a benefit to unify switch expression arm parsing with the other comma-separated lists.

I didn't update code that wasn't using the existing pattern. We can certainly go back and attempt to do that if someone wants :) What i was trying to do was have a common helper for all the cases that had been copy/pasted and slowly changed over time :)

as an example, this code doesn't use the pattern of IsSwitchExpressionArm/ParseSwitchExpressionArm. So it's a non-trivial update to move from this form to teh form the rest have. Why this code doesn't follow the other pattern is something i don't have understanding of.

put another way ParseCommaSeparatedList is the helper for all separated-list-parsing that has the same breakdown of "IsXXX, ParseXXX, SkipBadXXX". If we have such code, it should move to the helper.

RikkiGibson · 2023-01-28T02:38:19Z

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

+        [Fact, WorkItem(53011, "https://github.com/dotnet/roslyn/issues/53011")]
+        public void InvalidPropertyPattern()
+        {
+            UsingExpression(@"new object() is { {}: 1 }", TestOptions.RegularWithPatternCombinators,


tbh, I didn't understand this parse. It makes it seem like { {} } is a valid pattern, it's just to do : 1, we should have inserted a comma in between. What am I missing?

the syntax model for this allows a sequence of pattern children, invalid forms of which are reported about if they are syntactically ok.

It makes it seem like { {} } is a valid pattern

during parsing it is. later on in binding it does this:

if (expr == null) { if (!hasErrors) diagnostics.Add(ErrorCode.ERR_PropertyPatternNameMissing, pattern.Location, pattern); memberType = CreateErrorType(); member = null; hasErrors = true; }

I'm not sure why teh parsing is so lenient. perhaps to better handle all sort of weird things that might happen as the user is typing.

CyrusNajmabadi · 2023-01-28T02:46:52Z

Is there a comment indicates this or an issue that tracks doing this?

I don't know what this is in reference to.

RikkiGibson · 2023-01-28T02:57:42Z

Bleh github is not showing the thread the comment is part of. Was response to:

this is definitely inconsistent in that this guy doesn't pass along closeKind (but instead just inlines the value it knows that would be into its lambda). We should make these more consistent in the future.

CyrusNajmabadi · 2023-01-28T02:59:16Z

this is definitely inconsistent in that this guy doesn't pass along closeKind (but instead just inlines the value it knows that would be into its lambda). We should make these more consistent in the future.

I don't have anything tracking this. TBH, i think tracking items are virtually useless. They just go into the void. :)

If someone is motivated and wants to continue improving this stuff, i'm all for it (and i might do it myself). But i don't have it as a goal to solve all the parser issues here. I just want to clean up some broken windows, and make things meaningfully better than befoer :)

CyrusNajmabadi added 9 commits January 25, 2023 18:44

Introduce helper

de62fd5

Use helper

fd039b1

In progress

c36bec9

In progress

5f9b61e

In progress

e194c41

In progress

d74310c

In progress

6b338a8

In progress

acaf6b6

In progress

32a960b

CyrusNajmabadi requested a review from a team as a code owner January 26, 2023 03:30

dotnet-issue-labeler bot added the Area-Compilers label Jan 26, 2023

CyrusNajmabadi added 6 commits January 25, 2023 19:34

Move skip methods to where they are used

e6688a9

Move skip methods to where they are used

cb959d4

Move skip methods to where they are used

f11bf01

Move skip methods to where they are used

85f9d19

Move skip methods to where they are used

e555ae1

move

909786c

alrz reviewed Jan 26, 2023

View reviewed changes

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

CyrusNajmabadi added 3 commits January 26, 2023 10:44

Restore error messages

fd6db09

Restore exact behavior

0a98280

Restore exact behavior

c9d5b1c

CyrusNajmabadi changed the title ~~WIP: Introduce parser helper for parsing separated syntax lists.~~ Introduce parser helper for parsing separated syntax lists. Jan 26, 2023

CyrusNajmabadi commented Jan 26, 2023

View reviewed changes

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

CyrusNajmabadi commented Jan 26, 2023

View reviewed changes

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

CyrusNajmabadi commented Jan 26, 2023

View reviewed changes

CyrusNajmabadi added 2 commits January 27, 2023 09:55

Add docs

b97d44e

Use helper for enum parsing

92681ab

CyrusNajmabadi commented Jan 27, 2023

View reviewed changes

CyrusNajmabadi and others added 2 commits January 27, 2023 10:30

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

38a376b

… into parserHelper

Remove

f54258e

RikkiGibson self-assigned this Jan 27, 2023

333fred reviewed Jan 27, 2023

View reviewed changes

CyrusNajmabadi commented Jan 28, 2023

View reviewed changes

CyrusNajmabadi added 2 commits January 27, 2023 17:28

Make all arguments required

4db910c

Remove comment

f66a43f

CyrusNajmabadi requested a review from 333fred January 28, 2023 01:30

333fred approved these changes Jan 28, 2023

View reviewed changes

RikkiGibson reviewed Jan 28, 2023

View reviewed changes

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

RikkiGibson reviewed Jan 28, 2023

View reviewed changes

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

CyrusNajmabadi added 2 commits January 27, 2023 18:20

Remove comment

46f3206

Move

626a82e

RikkiGibson approved these changes Jan 28, 2023

View reviewed changes

CyrusNajmabadi enabled auto-merge (squash) January 28, 2023 03:29

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

3af79d0

CyrusNajmabadi merged commit c555e80 into dotnet:main Jan 28, 2023

CyrusNajmabadi deleted the parserHelper branch January 28, 2023 21:22

ghost added this to the Next milestone Jan 28, 2023

Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce parser helper for parsing separated syntax lists. #66552

Introduce parser helper for parsing separated syntax lists. #66552

CyrusNajmabadi commented Jan 26, 2023

CyrusNajmabadi commented Jan 26, 2023

CyrusNajmabadi Jan 26, 2023

CyrusNajmabadi Jan 26, 2023

CyrusNajmabadi Jan 26, 2023 •

edited

Loading

CyrusNajmabadi Jan 26, 2023

CyrusNajmabadi Jan 26, 2023

CyrusNajmabadi commented Jan 27, 2023

CyrusNajmabadi Jan 27, 2023

CyrusNajmabadi Jan 27, 2023

CyrusNajmabadi Jan 27, 2023

CyrusNajmabadi Jan 27, 2023

RikkiGibson Jan 28, 2023

333fred Jan 27, 2023

CyrusNajmabadi Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

CyrusNajmabadi Jan 27, 2023

CyrusNajmabadi Jan 28, 2023

RikkiGibson Jan 28, 2023

RikkiGibson Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

RikkiGibson Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

RikkiGibson Jan 28, 2023

CyrusNajmabadi Jan 28, 2023

CyrusNajmabadi Jan 28, 2023 •

edited

Loading

CyrusNajmabadi commented Jan 28, 2023

RikkiGibson commented Jan 28, 2023

CyrusNajmabadi commented Jan 28, 2023 •

edited

Loading

Introduce parser helper for parsing separated syntax lists. #66552

Introduce parser helper for parsing separated syntax lists. #66552

Conversation

CyrusNajmabadi commented Jan 26, 2023

CyrusNajmabadi commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 27, 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

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

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 Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 28, 2023

RikkiGibson commented Jan 28, 2023

CyrusNajmabadi commented Jan 28, 2023 • edited Loading

CyrusNajmabadi Jan 26, 2023 •

edited

Loading

CyrusNajmabadi Jan 28, 2023 •

edited

Loading

CyrusNajmabadi commented Jan 28, 2023 •

edited

Loading