Move to more consistent patterns in the parser. #65724

CyrusNajmabadi · 2022-12-02T19:00:48Z

Definitely review with whitespace off.

CyrusNajmabadi · 2022-12-02T23:27:11Z

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

-                if (this.CurrentToken.Kind == SyntaxKind.OperatorKeyword)
-                {
-                    return null;
-                }


unnecessary. the very next check bails if we don't have an identifiertoken, so if we have an operatorkeyword, that would always bail.

CyrusNajmabadi · 2022-12-02T23:27:56Z

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


            if (result.IsMissing &&
                (this.CurrentToken.Kind != SyntaxKind.CommaToken && this.CurrentToken.Kind != SyntaxKind.GreaterThanToken) &&
-                ((nextTokenKind = this.PeekToken(1).Kind) == SyntaxKind.CommaToken || nextTokenKind == SyntaxKind.GreaterThanToken))
+                (this.PeekToken(1).Kind is SyntaxKind.CommaToken or SyntaxKind.GreaterThanToken))


this is so much cleaner. locals being assigned halfway through an expr to read later is not good :)

CyrusNajmabadi · 2022-12-02T23:28:01Z

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

@@ -7227,6 +7197,7 @@ private SyntaxToken EatNullableQualifierIfApplicable(ParseTypeMode mode)
            Debug.Assert(this.CurrentToken.Kind == SyntaxKind.QuestionToken);
            using var resetPoint = this.GetDisposableResetPoint(resetOnDispose: false);

+


Suggested change

CyrusNajmabadi · 2022-12-02T23:28:44Z

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

@@ -10945,29 +10911,31 @@ private ExpressionSyntax ParseTermWithoutPostfix(Precedence precedence)
                        return this.AddError(this.CreateMissingIdentifierName(), ErrorCode.ERR_InvalidExprTerm, this.CurrentToken.Text);
                    }
                case SyntaxKind.IdentifierToken:
-                    if (this.IsTrueIdentifier())
                    {


braces are to prevent some conflicts with pattern name clashes in a switch statement.

CyrusNajmabadi · 2022-12-05T17:06:17Z

@jjonescz @RikkiGibson @333fred ptal :)

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

Co-authored-by: Jan Jones <[email protected]>

CyrusNajmabadi · 2022-12-06T17:46:13Z

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

@@ -4774,7 +4693,7 @@ private void ParseParameterModifiers(SyntaxListBuilder modifiers, bool isFunctio
                this.ParseVariableDeclarators(type, flags: VariableFlags.LocalOrField, variables: variables, parentKind: parentKind);

                // Make 'scoped' part of the type when it is the last token in the modifiers list
-                if (modifiers.Count != 0 && modifiers[modifiers.Count - 1] is SyntaxToken { Kind: SyntaxKind.ScopedKeyword } scopedKeyword)
+                if (modifiers is [.., SyntaxToken { Kind: SyntaxKind.ScopedKeyword } scopedKeyword])


Yaay patterns.

333fred

Done review pass.

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

333fred · 2022-12-06T19:27:46Z

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

@@ -7111,8 +6995,11 @@ bool canBeNullableType()
                            if (mode == ParseTypeMode.DefinitePattern)
                                return true; // Permit nullable type parsing and report while binding for a better error message
                            if (mode == ParseTypeMode.NewExpression && type.Kind == SyntaxKind.TupleType &&
-                                this.PeekToken(1).Kind != SyntaxKind.OpenParenToken && this.PeekToken(1).Kind != SyntaxKind.OpenBraceToken)
+                                this.PeekToken(1).Kind is not SyntaxKind.OpenParenToken and not SyntaxKind.OpenBraceToken)
+                            {


Most of the file you've removed these, but here you added them?

so thsi is a requirement of .net style/formatting. you can only have a single-line body without braces if the condition is also single-line. i can remove, but i figured while i was here i should address it.

333fred · 2022-12-06T20:10:32Z

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

-            }
-
-            return _syntaxFactory.JoinClause(@join, type, name, @in, inExpression, @on, leftExpression, @equals, rightExpression, joinInto);
+            return _syntaxFactory.JoinClause(


I would prefer these using named parameters please, reading this is much more difficult than the original version with local names.

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

CyrusNajmabadi · 2022-12-08T16:53:25Z

hey @RikkiGibson could i get eyes on this?

RikkiGibson · 2022-12-08T20:46:00Z

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

-                        token = this.AddError(token, ErrorCode.ERR_MemberNeedsType);
-                        var voidType = _syntaxFactory.PredefinedType(token);
+                        var voidType = _syntaxFactory.PredefinedType(
+                            this.AddError(SyntaxFactory.MissingToken(SyntaxKind.VoidKeyword), ErrorCode.ERR_MemberNeedsType));


I am wondering if a syntactically simpler helper for creating a missing token with a certain error on it would be desirable here and in many other places. For example: this.CreateMissingToken(SyntaxKind.VoidKeyword, ErrorCode.MemberNeedsType)

that would make a ton of sense, and i would def be in support of helpers for common patterns like this.

RikkiGibson · 2022-12-08T20:58:17Z

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

@@ -6270,7 +6167,7 @@ private bool IsOpenName()
                        separator = ConvertToMissingWithTrailingTrivia(separator, SyntaxKind.DotToken);
                    }

-                    if (isEvent && this.CurrentToken.Kind != SyntaxKind.OpenBraceToken && this.CurrentToken.Kind != SyntaxKind.SemicolonToken)
+                    if (isEvent && this.CurrentToken.Kind is not SyntaxKind.OpenBraceToken and not SyntaxKind.SemicolonToken)


I wouldn't mind seeing these De Morgan'd like this.CurrentToken.Kind is not (SyntaxKind.OpenBraceToken or SyntaxKind.SemicolonToken)

i was on the fence. Parenthesized patterns don't quite feel natural to me just yet. And i thought this was nice for preserving the same exact type of check we previously had. e.g. it was && and != before, so makign that and and not seemed the least contentious.

RikkiGibson · 2022-12-08T21:08:31Z

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

-                            this.CurrentToken.Kind == SyntaxKind.OpenBracketToken ||   // array type
-                            this.CurrentToken.Kind == SyntaxKind.OpenBraceToken;   // object initializer
+                            this.CurrentToken.Kind is SyntaxKind.OpenParenToken or   // ctor parameters
+                                                      SyntaxKind.OpenBracketToken or   // array type


I personally would not do this (inserting extra spaces in order to make certain tokens align across lines).

In fact, this is something the format analyzer forbids in other cases. e.g.

roslyn/src/Workspaces/Core/Portable/Editing/DeclarationModifiers.cs

Lines 192 to 216 in d73d31c

[Flags]

private enum Modifiers

{

#pragma warning disable format

None = 0,

Static = 1 << 0,

Abstract = 1 << 1,

New = 1 << 2,

Unsafe = 1 << 3,

ReadOnly = 1 << 4,

Virtual = 1 << 5,

Override = 1 << 6,

Sealed = 1 << 7,

Const = 1 << 8,

WithEvents = 1 << 9,

Partial = 1 << 10,

Async = 1 << 11,

WriteOnly = 1 << 12,

Ref = 1 << 13,

Volatile = 1 << 14,

Extern = 1 << 15,

Required = 1 << 16,

File = 1 << 17,

#pragma warning restore format

}

I would much rather see us simply insert a line break + indent before the token where we desire the alignment.

return this.CurrentToken.Kind is SyntaxKind.OpenParenToken or SyntaxKind.OpenBracketToken or SyntaxKind.OpenBraceToken;

I prefer even more to put the operator token at the start of the line. SyntaxKind will not strictly align in this case (if we used an and or not operator in some places, for example), but the visual pop of seeing the smaller operator token at the start of the line seems better for skimmability, in my opinion.

return this.CurrentToken.Kind is SyntaxKind.OpenParenToken or SyntaxKind.OpenBracketToken or SyntaxKind.OpenBraceToken;

i can do that. i checked with IDE at one point if we had a view on where was best for operators, and currently there's no consistency from the team. so we'ved decided to not be opinionated there.

RikkiGibson · 2022-12-08T21:20:03Z

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


                    case SyntaxKind.ExclamationToken:
                        expr = _syntaxFactory.PostfixUnaryExpression(SyntaxKind.SuppressNullableWarningExpression, expr, this.EatToken());
-                        break;
+                        continue;


It looks like using continue here instead of break is just to improve clarity? We used to jump to the end of the switch statement and then hit the top of the next loop iteration, but now we just hit the top of the next loop iteration?

correct. IMO 'continue' is better for clarity. 'break' both has the issue of being confusing (am i breaking the loop? or am i in a switch?), and needing the reader to then need to see what happens at the end of hte loop. 'continue' perfect describes what is happening, and doesn't require forward reading.

RikkiGibson · 2022-12-08T21:25:12Z

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

-                        this.EatTokenAsKind(SyntaxKind.IdentifierToken) :
-                        this.ParseIdentifierToken();
+                    var identifier = (this.CurrentToken.Kind != SyntaxKind.IdentifierToken && this.PeekToken(1).Kind == SyntaxKind.EqualsGreaterThanToken)
+                        ? this.EatTokenAsKind(SyntaxKind.IdentifierToken)


I personally find it strange that we like to put the ?: tokens at the start of the line, but we like to put the is/or tokens at the end of the line.

Jah. This is the opinionated IDE style we have. We have a formatting rule for this as well, but it (like most things) is turned off in teh compiler layer :)

CyrusNajmabadi added 4 commits December 2, 2022 09:52

Improve parser consistency

8a8953d

Simplify more

5fa3332

Improve parser consistency

a71d81d

Improve parser consistency

7d5a43c

dotnet-issue-labeler bot added the Area-Compilers label Dec 2, 2022

CyrusNajmabadi added 2 commits December 2, 2022 11:08

simplify further

598abba

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

3f9fd20

CyrusNajmabadi commented Dec 2, 2022

View reviewed changes

CyrusNajmabadi requested review from jjonescz and RikkiGibson December 2, 2022 23:29

CyrusNajmabadi marked this pull request as ready for review December 2, 2022 23:29

CyrusNajmabadi requested a review from a team as a code owner December 2, 2022 23:29

CyrusNajmabadi added 3 commits December 2, 2022 15:52

Lint

02eb3ed

Simplify

b3a64a3

Simplify

305c6fb

CyrusNajmabadi requested a review from 333fred December 6, 2022 00:02

jjonescz reviewed Dec 6, 2022

View reviewed changes

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

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

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

CyrusNajmabadi and others added 3 commits December 6, 2022 07:03

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

3c185a9

Co-authored-by: Jan Jones <[email protected]>

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

072a5cc

Simplify more

7948b95

CyrusNajmabadi requested a review from jjonescz December 6, 2022 16:57

CyrusNajmabadi added 5 commits December 6, 2022 08:58

Use pattern

2771844

Simplify

847a141

Simplify

081dc80

Simplify

cd2fc20

Simplify

22b93e6

CyrusNajmabadi added 5 commits December 6, 2022 09:37

Simplify more

46419ac

Simplify more

66212d0

Simplify more

c79c2d4

Simplify more

5eca08a

Simplify more

64fd4de

CyrusNajmabadi commented Dec 6, 2022

View reviewed changes

CyrusNajmabadi added 3 commits December 6, 2022 09:48

Simplify more

2a0aa58

Simplify more

e65403f

Simplify more

aef2339

333fred reviewed Dec 6, 2022

View reviewed changes

CyrusNajmabadi added 2 commits December 6, 2022 12:59

named arg

a29a571

Named args

45affd5

CyrusNajmabadi requested a review from 333fred December 6, 2022 21:01

333fred approved these changes Dec 6, 2022

View reviewed changes

CyrusNajmabadi requested a review from jcouv December 6, 2022 23:34

jjonescz reviewed Dec 7, 2022

View reviewed changes

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

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

CyrusNajmabadi added 2 commits December 7, 2022 09:12

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

3bcb6bd

Named args and cleanup

12d8b40

CyrusNajmabadi requested a review from jjonescz December 7, 2022 17:24

CyrusNajmabadi added 2 commits December 7, 2022 09:25

Cleanup

668c130

Cleanup

3546b99

RikkiGibson self-assigned this Dec 8, 2022

RikkiGibson approved these changes Dec 8, 2022

View reviewed changes

CyrusNajmabadi merged commit dabbd10 into dotnet:main Dec 8, 2022

CyrusNajmabadi deleted the parserCleanup branch December 8, 2022 21:54

ghost added this to the Next milestone Dec 8, 2022

allisonchou mentioned this pull request Dec 15, 2022

[Automated] PRs inserted in VS build main-33215.136 #66029

Closed

Cosifne modified the milestones: Next, 17.5 P3 Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to more consistent patterns in the parser. #65724

Move to more consistent patterns in the parser. #65724

CyrusNajmabadi commented Dec 2, 2022 •

edited

Loading

CyrusNajmabadi Dec 2, 2022

CyrusNajmabadi Dec 2, 2022

CyrusNajmabadi Dec 2, 2022

CyrusNajmabadi Dec 2, 2022 •

edited

Loading

CyrusNajmabadi commented Dec 5, 2022

CyrusNajmabadi Dec 6, 2022

333fred left a comment

333fred Dec 6, 2022

CyrusNajmabadi Dec 6, 2022

333fred Dec 6, 2022

CyrusNajmabadi Dec 6, 2022

CyrusNajmabadi Dec 6, 2022

CyrusNajmabadi commented Dec 8, 2022

RikkiGibson Dec 8, 2022

CyrusNajmabadi Dec 8, 2022

RikkiGibson Dec 8, 2022

CyrusNajmabadi Dec 8, 2022

RikkiGibson Dec 8, 2022

CyrusNajmabadi Dec 8, 2022

RikkiGibson Dec 8, 2022

CyrusNajmabadi Dec 8, 2022

RikkiGibson Dec 8, 2022

CyrusNajmabadi Dec 8, 2022

		@@ -7227,6 +7197,7 @@ private SyntaxToken EatNullableQualifierIfApplicable(ParseTypeMode mode)
		Debug.Assert(this.CurrentToken.Kind == SyntaxKind.QuestionToken);
		using var resetPoint = this.GetDisposableResetPoint(resetOnDispose: false);

	[Flags]
	private enum Modifiers
	{
	#pragma warning disable format
	None = 0,
	Static = 1 << 0,
	Abstract = 1 << 1,
	New = 1 << 2,
	Unsafe = 1 << 3,
	ReadOnly = 1 << 4,
	Virtual = 1 << 5,
	Override = 1 << 6,
	Sealed = 1 << 7,
	Const = 1 << 8,
	WithEvents = 1 << 9,
	Partial = 1 << 10,
	Async = 1 << 11,
	WriteOnly = 1 << 12,
	Ref = 1 << 13,
	Volatile = 1 << 14,
	Extern = 1 << 15,
	Required = 1 << 16,
	File = 1 << 17,
	#pragma warning restore format
	}

Move to more consistent patterns in the parser. #65724

Move to more consistent patterns in the parser. #65724

Conversation

CyrusNajmabadi commented Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 5, 2022

Choose a reason for hiding this comment

333fred 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

CyrusNajmabadi commented Dec 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 2, 2022 •

edited

Loading

CyrusNajmabadi Dec 2, 2022 •

edited

Loading