Fix parsing of 'file[]' and 'required[]' in old language versions #66769

RikkiGibson · 2023-02-09T01:38:18Z

CyrusNajmabadi · 2023-02-09T02:39:27Z

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


            if (!parsingStatementNotDeclaration)
            {
                var currentTokenKind = this.CurrentToken.Kind;
-                if (IsPossibleStartOfTypeDeclaration(currentTokenKind) ||
+                if ((IsPossibleStartOfTypeDeclaration(currentTokenKind) && currentTokenKind != SyntaxKind.OpenBracketToken) ||


what happens with other sorts of suffixes? like ? and *?

We do the right thing in those cases. I think the issue here is IsPossibleStartOfTypeDeclaration is the wrong helper to use here. That helper tells us if we might be at the start of an attribute list, or start of a modifier list, or at a type declaration keyword like class/struct.

I think what we actually want to know is, is this a non-contextual modifier or a type declaration keyword like class/struct/etc. Since there's no valid case in which this contextual keyword would be followed by an attribute list.

can you document this explicitly (and make sure we have tests here if not already present).

Done, thanks

RikkiGibson · 2023-02-09T20:44:22Z

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

@@ -325,6 +325,7 @@ internal CompilationUnitSyntax ParseCompilationUnitCore()
            }
        }

+        /// <summary>Are we possibly at the start of an attribute list, or at a modifier which is valid on a type, or on a keyword of a type declaration?</summary>
        private static bool IsPossibleStartOfTypeDeclaration(SyntaxKind kind)


Note that the usage of this method on line 1187 (ParseModifiers) really wants to be "is this a valid modifier for a type declaration".

It really makes one wish for reusable/composable patterns, versus being tempted to copy/paste this switch statement around.

The only fully valid usage of this method IMO is in IsPossibleNamespaceMemberDeclaration.

RikkiGibson · 2023-02-09T20:48:03Z

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

+        [Fact]
+        public void TestNewPartialArray()
+        {
+            UsingTree("new partial[1]");


This used to parse as a bad member declaration, as if it were new partial followed by an attribute list, which doesn't really make sense.

Might want to check the behavior of new file[1] and new required[1] at the top level as well.

RikkiGibson · 2023-02-09T23:10:05Z

As part of this issue I found (I think) that we regressed scenarios like new required(); as a top level statement (which I think worked in C# 9 but not 11).

…words

RikkiGibson · 2023-02-11T01:25:40Z

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

@@ -8062,7 +8068,7 @@ private bool IsPossibleNewExpression()

                // class, struct, enum, interface keywords, but also other modifiers that are not allowed after 
                // partial keyword but start class declaration, so we can assume the user just swapped them.
-                if (IsPossibleStartOfTypeDeclaration(PeekToken(2).Kind))
+                if (IsTypeModifierOrTypeKeyword(PeekToken(2).Kind))


2 things to note:

this helper is only called for ParseMemberDeclarationOrStatement. i.e., top-level declarations that could either be a member or a statement.

The intent here seems to be to ask: are we looking at new partial followed by something which indicates this is part of a member declaration. new partial [ isn't going to parse as a single member declaration. new partial MODIFIER is not valid, but we still try to keep going and parse as a single erroneous member declaration in such a case

This probably doesn't behave well when new partial is followed by a contextual keyword like new partial file .... That is still an error case at this point, though, so it just means our error recovery won't be the best. Eventually I'm sure we'll do the work to relax partial modifier ordering, at which point we might be able to refactor a bunch of this code, and maybe do something like "parse the modifiers and see if we had a partial modifier. if it's not the last modifier then do a LangVersion check".

333fred · 2023-02-13T18:40:33Z

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

@@ -1870,6 +1870,373 @@ public void RequiredModifierIncompleteMember_05()
            EOF();
        }

+        [Fact]
+        public void TypeNamedRequired_CSharp10()


There's no type named required in this test, unlike the file version :).

I'll add back 'class required' here, perhaps it's helpful to be clear about how it parses in each language version.

RikkiGibson · 2023-02-14T20:35:09Z

@dotnet/roslyn-compiler for a second review

RikkiGibson · 2023-02-15T16:28:33Z

Missed the window for 17.5, so will merge to 17.6 to start with.

cston · 2023-02-15T20:33:15Z

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

+        [Theory]
+        [InlineData("file")]
+        [InlineData("required")]
+        public void TopLevel_NewContextualKeywordArray(string keyword)


TopLevel_NewContextualKeywordArray

Consider adding [InlineData("async")] or a separate test if async is parsed differently than the other cases here.

cston · 2023-02-15T20:37:11Z

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

+
+            public unsafe class C
+            {
+                public file _file;


public

Consider testing the same class declaration but without the public modifier on the fields, with -langversion:10 and -langversion:11.

I feel this is adequately covered by TypeNamedRequired_CSharp10 and TypeNamedRequired_CSharp11

Fix parsing of 'file[]' and 'required[]' in old language versions

b785503

dotnet-issue-labeler bot added the Area-Compilers label Feb 9, 2023

CyrusNajmabadi reviewed Feb 9, 2023

View reviewed changes

WIP

c71e9d7

RikkiGibson commented Feb 9, 2023

View reviewed changes

Test nullable and partial array as global statement cases

874b885

RikkiGibson commented Feb 9, 2023

View reviewed changes

RikkiGibson added 2 commits February 9, 2023 12:52

More

70d5c6d

Fix test

01b73b6

RikkiGibson added 2 commits February 9, 2023 17:17

Fix parsing of more 'new-expression' combinations with contextual key…

39b91b3

…words

more

42c7450

RikkiGibson requested a review from CyrusNajmabadi February 11, 2023 01:25

RikkiGibson commented Feb 11, 2023

View reviewed changes

RikkiGibson marked this pull request as ready for review February 11, 2023 02:38

RikkiGibson requested a review from a team as a code owner February 11, 2023 02:38

333fred reviewed Feb 13, 2023

View reviewed changes

333fred approved these changes Feb 13, 2023

View reviewed changes

RikkiGibson requested a review from a team February 14, 2023 01:00

Add back 'class required'

a04f031

RikkiGibson changed the base branch from release/dev17.5 to main February 15, 2023 16:27

cston reviewed Feb 15, 2023

View reviewed changes

cston approved these changes Feb 15, 2023

View reviewed changes

RikkiGibson added 2 commits February 15, 2023 13:39

Add TopLevel_NewAsyncArray

fa4fdac

remove redundant tests

a6c8ab2

cston approved these changes Feb 15, 2023

View reviewed changes

RikkiGibson merged commit 54b7df9 into dotnet:main Feb 16, 2023

ghost added this to the Next milestone Feb 16, 2023

RikkiGibson deleted the file-parse-compat branch February 16, 2023 07:13

allisonchou mentioned this pull request Feb 17, 2023

[Automated] PRs inserted in VS build main-33417.173 #66943

Closed

RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing of 'file[]' and 'required[]' in old language versions #66769

Fix parsing of 'file[]' and 'required[]' in old language versions #66769

RikkiGibson commented Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

RikkiGibson Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

RikkiGibson Feb 9, 2023

RikkiGibson Feb 9, 2023 •

edited

RikkiGibson Feb 9, 2023

RikkiGibson Feb 9, 2023

RikkiGibson Feb 9, 2023

RikkiGibson commented Feb 9, 2023

RikkiGibson Feb 11, 2023 •

edited

333fred Feb 13, 2023

RikkiGibson Feb 14, 2023

RikkiGibson commented Feb 14, 2023

RikkiGibson commented Feb 15, 2023

cston Feb 15, 2023

cston Feb 15, 2023 •

edited

RikkiGibson Feb 15, 2023

Fix parsing of 'file[]' and 'required[]' in old language versions #66769

Fix parsing of 'file[]' and 'required[]' in old language versions #66769

Conversation

RikkiGibson commented Feb 9, 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

RikkiGibson Feb 9, 2023 • edited

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

RikkiGibson commented Feb 9, 2023

RikkiGibson Feb 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Feb 14, 2023

RikkiGibson commented Feb 15, 2023

Choose a reason for hiding this comment

cston Feb 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Feb 9, 2023 •

edited

RikkiGibson Feb 11, 2023 •

edited

cston Feb 15, 2023 •

edited