File modifier parsing #60823

RikkiGibson · 2022-04-18T22:39:30Z

Related to #60819

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs

src/Compilers/CSharp/Portable/Errors/MessageID.cs

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

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

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

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

cston · 2022-04-21T21:26:41Z

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

+                """);
+            N(SyntaxKind.CompilationUnit);
+            {
+                N(SyntaxKind.IncompleteMember);


IncompleteMember

Is this expected?

It seems perfectly valid to have a file partial ref struct. I'll have to fix it to ensure we get a correct parse here.

I was wrong. partial ref struct is not allowed. You have to say ref partial struct.

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

…dd tests.

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

333fred

Done review pass (commit 10). I did not look in depth at tests.

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs

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

src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs

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

…yntax

CyrusNajmabadi · 2022-04-28T16:54:50Z

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

@@ -1243,8 +1245,18 @@ private void ParseModifiers(SyntaxListBuilder tokens, bool forAccessors)
                            break;
                        }

+                    case DeclarationModifiers.File:
+                        if (!IsFeatureEnabled(MessageID.IDS_FeatureFileTypes) && !ShouldContextualKeywordBeTreatedAsModifier(parsingStatementNotDeclaration: false))


i don't love hte feature flag check actually controlling behavior. i think we should just parse this out if ShouldContextualKeywordBeTreatedAsModifier returns true and then report an issue later if the feature is not available. right?

That's fair. This is also the cause of a breaking change in the new language version. This is also what we are doing for 'required' modifier. See related discussion.

This ends up biasing our error recovery involving 'file' to: you meant to declare a member that uses this modifier, versus "you must have been using it as an identifier". Compare the _CSharp10 versions of tests to their CSharpNext or unsuffixed counterparts.

CyrusNajmabadi · 2022-04-28T16:55:53Z

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

        {
-            Debug.Assert(this.CurrentToken.ContextualKind == SyntaxKind.AsyncKeyword);
+            Debug.Assert(this.CurrentToken.ContextualKind != this.CurrentToken.Kind);


consider explicitly stating which contextual keywords this is expected to undertand. i think it's nicer if the reader can know "ok, this is just for async/file/etc." as opposed to "oh... what agbout all the other contextual keywords that we might have??"

Alternatively, consider passing in the token type we are looking to treat as a modifier.

We now check GetModifier in the assert which achieves what we want--don't need to repeat ourselves here, and we are limited to allowing the contextual keywords which are modifiers.

333fred · 2022-04-28T18:09:24Z

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

@@ -8013,7 +7979,7 @@ private bool IsPossibleLocalDeclarationStatement(bool isGlobalScriptLevel)
            tk = this.CurrentToken.ContextualKind;

            var isPossibleAttributeOrModifier = (IsAdditionalLocalFunctionModifier(tk) || tk == SyntaxKind.OpenBracketToken)
-                && (tk != SyntaxKind.AsyncKeyword || ShouldAsyncBeTreatedAsModifier(parsingStatementNotDeclaration: true));
+                && (tk != SyntaxKind.AsyncKeyword || ShouldContextualKeywordBeTreatedAsModifier(parsingStatementNotDeclaration: true));


Is this special case of AsyncKeyword correct now?

Yeah, we don't treat 'file' as a modifier when parsing a local declaration, just like we don't treat 'required' as a modifier on local declarations in the required-members feature branch.

333fred · 2022-04-28T18:19:34Z

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

+            EOF();
+        }
+
+        // PROTOTYPE(ft): is it fine that records parse as a single declaration here, but not other type kinds?


No, I would expect all of the type kinds to parse like this. What is causing them to parse as above?

When we try to parse the partial modifier, we check IsPartialMember():

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

Lines 1189 to 1192 in 228845e

case DeclarationModifiers.Partial:

var nextToken = PeekToken(1);

var isPartialType = this.IsPartialType();

var isPartialMember = this.IsPartialMember();

IsPartialMember simply asks, is this partial, followed by a possible type, followed by a possible member name.

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

Lines 1468 to 1475 in 228845e

this.EatToken(); // partial

if (this.ScanType() == ScanTypeFlags.NotType)

{

return false;

}

return IsPossibleMemberName();

In the case of partial file record, this is true. In the case of partial file class it is not. So in the former case we parse partial as a modifier and go from there to produce a single type declaration, while in the latter case we assume partial must be an identifier, and use it as a field type.

This is comparable to the parse we give for partial async class X. SharpLab.

My preference here would be to file a "design debt" item to improve error recovery for contextual modifiers and order-sensitive modifiers across the board, and call this parse good enough for the purposes of feature development.

333fred · 2022-04-28T18:21:21Z

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

+                """,
+                // (1,14): error CS1585: Member modifier 'ref' must precede the member type and name
+                // file partial ref struct C { }
+                Diagnostic(ErrorCode.ERR_BadModifierLocation, "ref").WithArguments("ref").WithLocation(1, 14));


This parse and diagnostic are just awful. I know you're not really changing it that much, but this is just wrong.

Have filed #61025

333fred · 2022-04-28T18:23:57Z

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

+        [Fact]
+        public void MemberNamedFile_06()
+        {
+            UsingNode($$"""


This is a breaking change: is LDM comfortable with it?

Adding a note to follow up on it.

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

333fred · 2022-04-28T18:43:56Z

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

+        }
+
+        [Fact]
+        public void MethodNamedRecord_02_CSharpNext()


This parse seems incorrect. I expect this to be a record with a missing name, not a constructor declaration(??)

I think the parse is very similar as it is when the 'file' modifier is removed here. SharpLab.

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

333fred · 2022-04-28T18:48:29Z

Done review pass (commit 14)

File modifier parsing

4fb5734

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

RikkiGibson marked this pull request as ready for review April 18, 2022 22:40

RikkiGibson requested a review from a team as a code owner April 18, 2022 22:40

RikkiGibson changed the base branch from main to features/file-types April 18, 2022 22:40

give LangVersion error in binding

fb624a1

CyrusNajmabadi reviewed Apr 18, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 18, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Errors/MessageID.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Apr 19, 2022

View reviewed changes

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

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs Show resolved Hide resolved

RikkiGibson and others added 5 commits April 20, 2022 13:42

Fix parsing members named 'file'

e4efb02

add more error tests

eff66b0

update parsing baselines

24a1f0d

Apply suggestions from code review

3f854fd

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

update gen code and disable public api check for now

9fee3df

RikkiGibson requested a review from a team as a code owner April 20, 2022 22:20

cston reviewed Apr 21, 2022

View reviewed changes

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

cston reviewed Apr 21, 2022

View reviewed changes

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

RikkiGibson removed the request for review from a team April 21, 2022 21:09

cston reviewed Apr 21, 2022

View reviewed changes

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

cston reviewed Apr 21, 2022

View reviewed changes

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

cston reviewed Apr 21, 2022

View reviewed changes

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

Handle long sequences of file modifiers. Fix some compat scenarios. A…

573ff74

…dd tests.

cston reviewed Apr 22, 2022

View reviewed changes

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

cston reviewed Apr 22, 2022

View reviewed changes

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

cston reviewed Apr 22, 2022

View reviewed changes

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

cston reviewed Apr 22, 2022

View reviewed changes

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

More tests. Always verify diagnostics.

0bd3b9e

cston reviewed Apr 22, 2022

View reviewed changes

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

use fact instead of parameterless theory

877ad9f

cston approved these changes Apr 22, 2022

View reviewed changes

RikkiGibson requested a review from 333fred April 27, 2022 19:06

333fred reviewed Apr 27, 2022

View reviewed changes

RikkiGibson added 4 commits April 27, 2022 16:23

Address feedback

9698453

Merge remote-tracking branch 'upstream/features/file-types' into ft-s…

228845e

…yntax

fix MessageID.RequiredVersion

310528e

Fix formatting

bb96370

CyrusNajmabadi reviewed Apr 28, 2022

View reviewed changes

CyrusNajmabadi approved these changes Apr 28, 2022

View reviewed changes

333fred reviewed Apr 28, 2022

View reviewed changes

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

333fred reviewed Apr 28, 2022

View reviewed changes

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

333fred reviewed Apr 28, 2022

View reviewed changes

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

Add tests

a87fb15

RikkiGibson mentioned this pull request Apr 28, 2022

Improve parser error recovery for contextual and positionally-sensitive modifiers #61025

Open

Address feedback

d7c4b69

cston approved these changes Apr 29, 2022

View reviewed changes

333fred approved these changes May 6, 2022

View reviewed changes

RikkiGibson merged commit 4550ac3 into dotnet:features/file-types May 9, 2022

RikkiGibson deleted the ft-syntax branch May 9, 2022 18:04

cston mentioned this pull request May 29, 2022

Support 'scoped' modifier for parameters and locals #61389

Merged

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File modifier parsing #60823

File modifier parsing #60823

RikkiGibson commented Apr 18, 2022 •

edited

Loading

cston Apr 21, 2022

RikkiGibson Apr 21, 2022

RikkiGibson Apr 22, 2022

333fred left a comment

CyrusNajmabadi Apr 28, 2022

RikkiGibson Apr 28, 2022

RikkiGibson Apr 28, 2022

CyrusNajmabadi Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred Apr 28, 2022

RikkiGibson Apr 28, 2022

333fred commented Apr 28, 2022

	case DeclarationModifiers.Partial:
	var nextToken = PeekToken(1);
	var isPartialType = this.IsPartialType();
	var isPartialMember = this.IsPartialMember();

	this.EatToken(); // partial

	if (this.ScanType() == ScanTypeFlags.NotType)
	{
	return false;
	}

	return IsPossibleMemberName();

File modifier parsing #60823

File modifier parsing #60823

Conversation

RikkiGibson commented Apr 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

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

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

333fred commented Apr 28, 2022

RikkiGibson commented Apr 18, 2022 •

edited

Loading