Initial support for collection literal parsing. #66417

CyrusNajmabadi · 2023-01-13T21:48:01Z

Followup to #66412

Relates to test plan #66418

CyrusNajmabadi · 2023-01-13T21:51:53Z

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

-            if (isPossibleAttributeOrModifier)
+            var isPossibleModifier = IsAdditionalLocalFunctionModifier(tk)
+                && (tk is not (SyntaxKind.AsyncKeyword or SyntaxKind.ScopedKeyword)
+                || ShouldContextualKeywordBeTreatedAsModifier(parsingStatementNotDeclaration: true));


we don't need to check for attributes here since attributes have already been parsed out before this.

CyrusNajmabadi · 2023-01-13T21:55:02Z

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

@@ -527,7 +527,7 @@ private enum NamespaceParts
                                // incomplete members must be processed before we add any nodes to the body:
                                ReduceIncompleteMembers(ref pendingIncompleteMembers, ref openBraceOrSemicolon, ref body, ref initialBadNodes);

-                                var attribute = this.ParseAttributeDeclaration();
+                                var attribute = this.ParseAttributeDeclaration(inExpressionContext: parentKind == SyntaxKind.CompilationUnit);


[ at the top level could start an attribute, or could start a collection literal. However, [ inside a namespace always starts an attribute. Good test cases @jnm2 :)

while not legal, the parser should also accept:

[IAmAnAttribute(1, 2, 3)] [on, a, collection, literal].DoWork();

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

CyrusNajmabadi · 2023-01-13T21:56:48Z

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

@@ -2776,7 +2793,7 @@ private MemberDeclarationSyntax ParseMemberDeclarationCore(SyntaxKind parentKind

            try
            {
-                var attributes = this.ParseAttributeDeclarations();
+                var attributes = this.ParseAttributeDeclarations(inExpressionContext: false);


all the places taht pass in false should have tests taht show we still think tehy are an attribute, even if the user types . after them.

CyrusNajmabadi · 2023-01-13T21:57:58Z

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

+
+                // PROTOTYPE: Def back compat concern here.  What if the user has `x ? y?[0] : z` this would be legal,
+                // but will now change to `y?[0] : z` being a ternary.  Have to decide if this is acceptable break.
+                return this.CurrentToken.Kind != SyntaxKind.ColonToken;


some interesting compat cases. we may have to be strict and require the user to write x ? ([1, 2, 3]) : ([4, 5, 6]). But that seems Really unfortunate. So i'm currently allowing the nicer form of x ? [1, 2, 3] : [4, 5, 6], but it may cause a compat break that LDM will have to approve.

CyrusNajmabadi · 2023-01-13T21:58:33Z

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

+
+            if (this.CurrentToken.Kind != SyntaxKind.CloseBracketToken)
+            {
+tryAgain:


this form is how we always do separated syntax list parsing. I personally dont' like repeating the entire full form. but it's what we do everywhere wlse, so this is consistent.

Is this referring to the duplicated IsPossibleCollectionCreationExpression() || this.CurrentToken.Kind == SyntaxKind.CommaToken check, before and within the loop? It looks like there's also a duplicated SkipBadCollectionCreationExpressionTokens() check within the loop and following the loop.

If it's simpler to combine the duplicated checks in each case, within a while (true) { ... } for instance, I'd prefer it.

I would very much like to keep this form in sync with all the other ParseSeparatedList algorithms we have in the parser. if we do want to change them, doign them all at once would be my preference.

I also really hate this code, and fwiw we are already not consistent on separated syntax list parsing algorithms. ParseFunctionPointerType, for example, uses a much simpler form of this.

Funny, I was just noting in #65256 (comment) that switch expression arm parsing does it its own way too :)

I would like for goto tryAgain; to goto the git history and for us to do something uniformly better here. But that seems like work better done in the base branch.

I think a very suitable approach would be to have a basic ParseSeparatedList helper which takes in a few callbacks (And some boolean options, like "allowTrailingSeparator"). Using your approach of passing the parser instance along rikki, we could ensure this doesn't cause allocs.

But i was worried about potential perf regressions of doing that, so i wanted to leave to an alternate PR that could be tested in isolation.

CyrusNajmabadi · 2023-01-13T21:59:38Z

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

+
+            var expression = this.ParseExpression();
+            if (dotDotToken != null)
+                return _syntaxFactory.SpreadElement(dotDotToken, expression);


interesting case woudl be .. without a following expression. .. ..goo. goo: (without a following expression). And things like [x ? 1 : 2 : value] (which i believe the spec will not allow... but i think will be allowed here).

also: [ : value] (Without a key).

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

…jmabadi/roslyn into collectionLiteralParsing

CyrusNajmabadi · 2023-01-30T21:18:05Z

@jcouv could you take a look?

jcouv · 2023-01-31T17:31:30Z

namespace Microsoft.CodeAnalysis.CSharp.UnitTests

nit: consider file-scoped namespace

In reply to: 1410787932

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/CollectionLiteralParsingTests.cs:9 in 3db96f8. [](commit_id = 3db96f8, deletion_comment = False)

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

CyrusNajmabadi · 2023-01-31T18:25:31Z

nit: consider file-scoped namespace

Done.

jcouv · 2023-01-31T18:28:08Z

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

@@ -10103,6 +10150,7 @@ private static Precedence GetPrecedence(SyntaxKind op)
                case SyntaxKind.ArrayCreationExpression:
                case SyntaxKind.BaseExpression:
                case SyntaxKind.CharacterLiteralExpression:
+                case SyntaxKind.CollectionCreationExpression:


nit: please mention the precedence (and the resolution on any upcoming ambiguity decision) in the spec. Rex/Bill likely to ask question on that when they get to standardization. #Closed

I can def add more. But it's already in the spec with where this construct is in the grammar. (can't hurt to have more info though of course) :)

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

jcouv · 2023-01-31T18:37:32Z

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

+            {
+                return @this.SkipBadSeparatedListTokensWithExpectedKind(ref openBracket, list,
+                    static p => p.CurrentToken.Kind != SyntaxKind.CommaToken && !p.IsPossibleCollectionElement(),
+                    static (p, closeKind) => p.CurrentToken.Kind == closeKind,


The shadowing of closeKind is confusing. Let's use different identifiers.
I see that we did that previously in SkipBadInitializerListTokens :-/ Still wouldn't recommend it. #Closed

this shadowing is extremely intentional (and done with all callback lambdas). It's precisely to indicate taht it's the same value, just passed through the lambda as an arg so that you always have hte same gy. Renaming is actually more confusing here as now there's the potential semantic confusion of "oh wait... is this closeKind2 somehow different from the containing closeKind1, and what does that mean?"

As with teh above, if we are to change this, i woulid prefer it be in a separate PR that does that for all the code like this (there are at least a dozen places to fixup if we do this).

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

jcouv · 2023-01-31T18:50:12Z

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

+            if (SyntaxFacts.IsReservedKeyword(this.CurrentToken.Kind) && this.PeekToken(1).Kind == SyntaxKind.ColonToken)
+            {
+                var keyword = this.EatTokenWithPrejudice(SyntaxKind.IdentifierToken);
+                var identifier = ConvertToIdentifier(keyword).WithDiagnosticsGreen(keyword.GetDiagnostics());


.WithDiagnosticsGreen(keyword.GetDiagnostics())

Is the WithDiagnosticsGreen portion necessary? #Closed

Yes. ConvertToIdentifier doesn't preserve diagnostics.

Can there be diagnostics? Other places that use ConvertToIdentifier don't do this

I don't think diagnostics can occur. But if they could, then the WithDiagnosticsGreen call should be inside ConvertToIdentifier (like we do in ConvertToKeyword), not the responsibility of the caller of ConvertToIdentifier.

Can there be diagnostics?

Yes. EatTokenWithPrejudice adds one.

Other places that use ConvertToIdentifier don't do this

They likely aren't using EatTokenWithPrejudice :)

But if they could, then the WithDiagnosticsGreen call should be inside ConvertToIdentifier

I can do that and see what happens.

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

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

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

jcouv

Done with review pass (iteration 65)

jcouv

LGTM Thanks! (iteration 66)
I'll summon the Gandalf in me to remind you that this shall be squashed 🧙

CyrusNajmabadi · 2023-01-31T22:43:08Z

:)

jcouv · 2023-01-31T22:51:55Z

Thanks! :-)

cston · 2023-01-31T22:59:43Z

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

@@ -15,6 +15,7 @@

 namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax
 {
+    using Microsoft.CodeAnalysis.CSharp.Symbols;


Microsoft.CodeAnalysis.CSharp.Symbols

Is this used?

nope. sorry :( can remove in followup.

CyrusNajmabadi added 4 commits January 13, 2023 13:13

Do not parse out an attribute that will actually be a collection literal

8c4380d

No need to check for attributes here. They have already been parsed out

f9a3716

Initial parsing

90cfe9d

Add handling for certain cases

24cd2f7

CyrusNajmabadi requested review from jnm2 and cston January 13, 2023 21:48

CyrusNajmabadi requested review from a team as code owners January 13, 2023 21:48

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

Rename method

1a7b04d

CyrusNajmabadi commented Jan 13, 2023

View reviewed changes

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

CyrusNajmabadi commented Jan 13, 2023

View reviewed changes

jnm2 reviewed Jan 15, 2023

View reviewed changes

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

jnm2 and others added 5 commits January 15, 2023 14:14

Start a parser test file with a few initial tests

4347ee4

Support parsing -> and ?.

fb616a7

Adding parser tests

4ac2f65

Merge branch 'collectionLiteralSyntax' into collectionLiteralParsing

8896025

Merge branch 'collectionLiteralParsing' of https://github.com/CyrusNa…

e5d6049

…jmabadi/roslyn into collectionLiteralParsing

CyrusNajmabadi requested a review from a team as a code owner January 15, 2023 20:00

jnm2 and others added 6 commits January 15, 2023 15:00

Adding parser tests

1a05f97

Improve error recovery

89926d8

Merge branch 'collectionLiteralParsing' of https://github.com/CyrusNa…

4d5d02d

…jmabadi/roslyn into collectionLiteralParsing

Improve error recovery

56eebcb

Merge branch 'collectionLiteralSyntax' into collectionLiteralParsing

7d69f00

Fix parsing

60fe9b5

CyrusNajmabadi enabled auto-merge (squash) January 30, 2023 19:29

CyrusNajmabadi disabled auto-merge January 30, 2023 19:30

RikkiGibson self-assigned this Jan 31, 2023

jcouv reviewed Jan 31, 2023

View reviewed changes

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

CyrusNajmabadi added 2 commits January 31, 2023 10:24

Add incremental parsing tests

f847b9c

use file scoped namespaces

a1347b1

jcouv reviewed Jan 31, 2023

View reviewed changes

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

Simplify

7d18098

jcouv reviewed Jan 31, 2023

View reviewed changes

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

jcouv reviewed Jan 31, 2023

View reviewed changes

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

jcouv reviewed Jan 31, 2023

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs Show resolved Hide resolved

Add syntax test

d9a1965

jcouv reviewed Jan 31, 2023

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LocalFunctionParsingTests.cs Show resolved Hide resolved

jcouv reviewed Jan 31, 2023

View reviewed changes

Copy diagnostics when converting token

e340181

CyrusNajmabadi requested a review from jcouv January 31, 2023 22:14

jcouv approved these changes Jan 31, 2023

View reviewed changes

CyrusNajmabadi enabled auto-merge (squash) January 31, 2023 22:42

cston reviewed Jan 31, 2023

View reviewed changes

cston approved these changes Jan 31, 2023

View reviewed changes

CyrusNajmabadi merged commit 598cbe4 into dotnet:features/CollectionLiterals Jan 31, 2023

CyrusNajmabadi deleted the collectionLiteralParsing branch February 1, 2023 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for collection literal parsing. #66417

Initial support for collection literal parsing. #66417

CyrusNajmabadi commented Jan 13, 2023 •

edited by jcouv

Loading

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 13, 2023

cston Jan 17, 2023 •

edited

Loading

CyrusNajmabadi Jan 17, 2023

333fred Jan 26, 2023

RikkiGibson Jan 26, 2023

CyrusNajmabadi Jan 26, 2023

CyrusNajmabadi Jan 13, 2023

CyrusNajmabadi Jan 15, 2023

CyrusNajmabadi commented Jan 30, 2023

jcouv commented Jan 31, 2023 •

edited

Loading

CyrusNajmabadi commented Jan 31, 2023

jcouv Jan 31, 2023 •

edited

Loading

CyrusNajmabadi Jan 31, 2023

jcouv Jan 31, 2023 •

edited

Loading

CyrusNajmabadi Jan 31, 2023

jcouv Jan 31, 2023 •

edited

Loading

CyrusNajmabadi Jan 31, 2023

jcouv Jan 31, 2023

jcouv Jan 31, 2023

CyrusNajmabadi Jan 31, 2023

jcouv left a comment

jcouv left a comment

CyrusNajmabadi commented Jan 31, 2023

jcouv commented Jan 31, 2023

cston Jan 31, 2023

CyrusNajmabadi Jan 31, 2023

Initial support for collection literal parsing. #66417

Initial support for collection literal parsing. #66417

Conversation

CyrusNajmabadi commented Jan 13, 2023 • edited by jcouv 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

cston Jan 17, 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 Jan 30, 2023

jcouv commented Jan 31, 2023 • edited Loading

CyrusNajmabadi commented Jan 31, 2023

jcouv Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 31, 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

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 31, 2023

jcouv commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 13, 2023 •

edited by jcouv

Loading

cston Jan 17, 2023 •

edited

Loading

jcouv commented Jan 31, 2023 •

edited

Loading

jcouv Jan 31, 2023 •

edited

Loading

jcouv Jan 31, 2023 •

edited

Loading

jcouv Jan 31, 2023 •

edited

Loading