Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File modifier parsing #60823

Merged
merged 16 commits into from
May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/config/globalconfigs/Common.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ dotnet_diagnostic.RS0006.severity = error
dotnet_diagnostic.RS0012.severity = warning
dotnet_diagnostic.RS0014.severity = warning
dotnet_diagnostic.RS0015.severity = warning
dotnet_diagnostic.RS0016.severity = error
dotnet_diagnostic.RS0016.severity = none # PROTOTYPE(ft): re-enable before feature merge
dotnet_diagnostic.RS0017.severity = error
dotnet_diagnostic.RS0018.severity = warning
dotnet_diagnostic.RS0022.severity = error
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7091,4 +7091,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureUnsignedRightShift" xml:space="preserve">
<value>unsigned right shift</value>
</data>
<data name="IDS_FeatureFileTypes" xml:space="preserve">
<value>file types</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@ internal enum DeclarationModifiers : uint
Async = 1 << 20,
Ref = 1 << 21, // used only for structs

All = (1 << 23) - 1, // all modifiers
Unset = 1 << 23, // used when a modifiers value hasn't yet been computed

// PROTOTYPE(ft): leaving 22 free since required is using it
File = 1 << 23, // used only for types

All = (1 << 24) - 1, // all modifiers
Unset = 1 << 24, // used when a modifiers value hasn't yet been computed

AccessibilityMask = PrivateProtected | Private | Protected | Internal | ProtectedInternal | Public,
}
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ internal enum MessageID
IDS_FeatureUTF8StringLiterals = MessageBase + 12822,

IDS_FeatureUnsignedRightShift = MessageBase + 12823,

IDS_FeatureFileTypes = MessageBase + 12824, // PROTOTYPE(ft): finalize feature name
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -373,6 +375,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureCheckedUserDefinedOperators: // semantic check for declarations, parsing check for doc comments
case MessageID.IDS_FeatureUTF8StringLiterals: // semantic check
case MessageID.IDS_FeatureUnsignedRightShift: // semantic check for declarations and consumption, parsing check for doc comments
case MessageID.IDS_FeatureFileTypes: // semantic check
return LanguageVersion.Preview;

// C# 10.0 features.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

70 changes: 40 additions & 30 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,8 @@ internal static DeclarationModifiers GetModifier(SyntaxKind kind, SyntaxKind con
return DeclarationModifiers.Partial;
case SyntaxKind.AsyncKeyword:
return DeclarationModifiers.Async;
case SyntaxKind.FileKeyword:
return DeclarationModifiers.File;
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return;
}

// LangVersion errors for 'file' modifier are given during binding.
modTok = ConvertToKeyword(EatToken());
break;

case DeclarationModifiers.Async:
if (!ShouldAsyncBeTreatedAsModifier(parsingStatementNotDeclaration: false))
if (!ShouldContextualKeywordBeTreatedAsModifier(parsingStatementNotDeclaration: false))
{
return;
}
Expand Down Expand Up @@ -1281,16 +1293,16 @@ bool isStructOrRecordKeyword(SyntaxToken token)
}
}

private bool ShouldAsyncBeTreatedAsModifier(bool parsingStatementNotDeclaration)
private bool ShouldContextualKeywordBeTreatedAsModifier(bool parsingStatementNotDeclaration)
{
Debug.Assert(this.CurrentToken.ContextualKind == SyntaxKind.AsyncKeyword);
Debug.Assert(this.CurrentToken.ContextualKind != this.CurrentToken.Kind);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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??"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Adapted from CParser::IsAsyncMethod.

if (IsNonContextualModifier(PeekToken(1)))
{
// If the next token is a (non-contextual) modifier keyword, then this token is
// definitely the async keyword
// definitely a modifier
return true;
}

Expand All @@ -1301,21 +1313,20 @@ private bool ShouldAsyncBeTreatedAsModifier(bool parsingStatementNotDeclaration)

try
{
this.EatToken(); //move past contextual 'async'
this.EatToken(); //move past contextual token

if (!parsingStatementNotDeclaration &&
(this.CurrentToken.ContextualKind == SyntaxKind.PartialKeyword))
{
this.EatToken(); // "partial" doesn't affect our decision, so look past it.
}

// Comment directly from CParser::IsAsyncMethod.
// ... 'async' [partial] <typedecl> ...
// ... 'async' [partial] <event> ...
// ... 'async' [partial] <implicit> <operator> ...
// ... 'async' [partial] <explicit> <operator> ...
// ... 'async' [partial] <typename> <operator> ...
// ... 'async' [partial] <typename> <membername> ...
// ... 'TOKEN' [partial] <typedecl> ...
// ... 'TOKEN' [partial] <event> ...
// ... 'TOKEN' [partial] <implicit> <operator> ...
// ... 'TOKEN' [partial] <explicit> <operator> ...
// ... 'TOKEN' [partial] <typename> <operator> ...
// ... 'TOKEN' [partial] <typename> <membername> ...
// DEVNOTE: Although we parse async user defined conversions, operators, etc. here,
// anything other than async methods are detected as erroneous later, during the define phase

Expand All @@ -1332,56 +1343,56 @@ private bool ShouldAsyncBeTreatedAsModifier(bool parsingStatementNotDeclaration)

if (ScanType() != ScanTypeFlags.NotType)
{
// We've seen "async TypeName". Now we have to determine if we should we treat
// 'async' as a modifier. Or is the user actually writing something like
// "public async Goo" where 'async' is actually the return type.
// We've seen "TOKEN TypeName". Now we have to determine if we should we treat
// 'TOKEN' as a modifier. Or is the user actually writing something like
// "public TOKEN Goo" where 'TOKEN' is actually the return type.

if (IsPossibleMemberName())
{
// we have: "async Type X" or "async Type this", 'async' is definitely a
// we have: "TOKEN Type X" or "TOKEN Type this", 'TOKEN' is definitely a
// modifier here.
return true;
}

var currentTokenKind = this.CurrentToken.Kind;

// The file ends with "async TypeName", it's not legal code, and it's much
// The file ends with "TOKEN TypeName", it's not legal code, and it's much
// more likely that this is meant to be a modifier.
if (currentTokenKind == SyntaxKind.EndOfFileToken)
{
return true;
}

// "async TypeName }". In this case, we just have an incomplete member, and
// we should definitely default to 'async' being considered a return type here.
// "TOKEN TypeName }". In this case, we just have an incomplete member, and
// we should definitely default to 'TOKEN' being considered a return type here.
if (currentTokenKind == SyntaxKind.CloseBraceToken)
{
return true;
}

// "async TypeName void". In this case, we just have an incomplete member before
// an existing member. Treat this 'async' as a keyword.
// "TOKEN TypeName void". In this case, we just have an incomplete member before
// an existing member. Treat this 'TOKEN' as a keyword.
if (SyntaxFacts.IsPredefinedType(this.CurrentToken.Kind))
{
return true;
}

// "async TypeName public". In this case, we just have an incomplete member before
// an existing member. Treat this 'async' as a keyword.
// "TOKEN TypeName public". In this case, we just have an incomplete member before
// an existing member. Treat this 'TOKEN' as a keyword.
if (IsNonContextualModifier(this.CurrentToken))
{
return true;
}

// "async TypeName class". In this case, we just have an incomplete member before
// an existing type declaration. Treat this 'async' as a keyword.
// "TOKEN TypeName class". In this case, we just have an incomplete member before
// an existing type declaration. Treat this 'TOKEN' as a keyword.
if (IsTypeDeclarationStart())
{
return true;
}

// "async TypeName namespace". In this case, we just have an incomplete member before
// an existing namespace declaration. Treat this 'async' as a keyword.
// "TOKEN TypeName namespace". In this case, we just have an incomplete member before
// an existing namespace declaration. Treat this 'TOKEN' as a keyword.
if (currentTokenKind == SyntaxKind.NamespaceKeyword)
{
return true;
Expand Down Expand Up @@ -2661,8 +2672,7 @@ static bool isAcceptableNonDeclarationStatement(StatementSyntax statement, bool
private bool IsMisplacedModifier(SyntaxListBuilder modifiers, SyntaxList<AttributeListSyntax> attributes, TypeSyntax type, out MemberDeclarationSyntax result)
{
if (GetModifier(this.CurrentToken) != DeclarationModifiers.None &&
this.CurrentToken.ContextualKind != SyntaxKind.PartialKeyword &&
this.CurrentToken.ContextualKind != SyntaxKind.AsyncKeyword &&
this.CurrentToken.ContextualKind is not (SyntaxKind.PartialKeyword or SyntaxKind.AsyncKeyword or SyntaxKind.FileKeyword) &&
IsComplete(type))
{
var misplacedModifier = this.CurrentToken;
Expand Down Expand Up @@ -7969,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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this special case of AsyncKeyword correct now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isPossibleAttributeOrModifier)
{
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ internal static DeclarationModifiers CheckModifiers(
modifierErrors |= !Binder.CheckFeatureAvailability(errorLocation.SourceTree, MessageID.IDS_FeaturePrivateProtected, diagnostics, errorLocation);
}

if ((result & DeclarationModifiers.File) != 0)
{
modifierErrors |= !Binder.CheckFeatureAvailability(errorLocation.SourceTree, MessageID.IDS_FeatureFileTypes, diagnostics, errorLocation);
}

return result;
}

Expand Down Expand Up @@ -281,6 +286,8 @@ internal static string ConvertSingleModifierToSyntaxText(DeclarationModifiers mo
return SyntaxFacts.GetText(SyntaxKind.AsyncKeyword);
case DeclarationModifiers.Ref:
return SyntaxFacts.GetText(SyntaxKind.RefKeyword);
case DeclarationModifiers.File:
return SyntaxFacts.GetText(SyntaxKind.FileKeyword);
default:
throw ExceptionUtilities.UnexpectedValue(modifier);
}
Expand Down Expand Up @@ -328,6 +335,8 @@ private static DeclarationModifiers ToDeclarationModifier(SyntaxKind kind)
return DeclarationModifiers.Volatile;
case SyntaxKind.RefKeyword:
return DeclarationModifiers.Ref;
case SyntaxKind.FileKeyword:
return DeclarationModifiers.File;
default:
throw ExceptionUtilities.UnexpectedValue(kind);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ private DeclarationModifiers MakeModifiers(TypeKind typeKind, BindingDiagnosticB
{
Symbol containingSymbol = this.ContainingSymbol;
DeclarationModifiers defaultAccess;
var allowedModifiers = DeclarationModifiers.AccessibilityMask;
// PROTOTYPE(ft): confirm whether 'file' types can be nested
var allowedModifiers = DeclarationModifiers.AccessibilityMask | DeclarationModifiers.File;

if (containingSymbol.Kind == SymbolKind.Namespace)
{
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,9 @@ public enum SyntaxKind : ushort
/// <summary>Represents <see langword="unmanaged"/>.</summary>
UnmanagedKeyword = 8446,

/// <summary>Represents <see langword="file"/>.</summary>
FileKeyword = 8447,

// when adding a contextual keyword following functions must be adapted:
// <see cref="SyntaxFacts.GetContextualKeywordKinds"/>
// <see cref="SyntaxFacts.IsContextualKeyword(SyntaxKind)"/>
Expand Down
7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1138,7 +1138,7 @@ public static SyntaxKind GetPreprocessorKeywordKind(string text)

public static IEnumerable<SyntaxKind> GetContextualKeywordKinds()
{
for (int i = (int)SyntaxKind.YieldKeyword; i <= (int)SyntaxKind.UnmanagedKeyword; i++)
for (int i = (int)SyntaxKind.YieldKeyword; i <= (int)SyntaxKind.FileKeyword; i++) // PROTOTYPE(ft): will conflict with required
{
yield return (SyntaxKind)i;
}
Expand Down Expand Up @@ -1191,6 +1191,7 @@ public static bool IsContextualKeyword(SyntaxKind kind)
case SyntaxKind.RecordKeyword:
case SyntaxKind.ManagedKeyword:
case SyntaxKind.UnmanagedKeyword:
case SyntaxKind.FileKeyword:
return true;
default:
return false;
Expand Down Expand Up @@ -1310,6 +1311,8 @@ public static SyntaxKind GetContextualKeywordKind(string text)
return SyntaxKind.ManagedKeyword;
case "unmanaged":
return SyntaxKind.UnmanagedKeyword;
case "file":
return SyntaxKind.FileKeyword;
default:
return SyntaxKind.None;
}
Expand Down Expand Up @@ -1751,6 +1754,8 @@ public static string GetText(SyntaxKind kind)
return "managed";
case SyntaxKind.UnmanagedKeyword:
return "unmanaged";
case SyntaxKind.FileKeyword:
return "file";
default:
return string.Empty;
}
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading