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.

Sign up for GitHub

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

File modifier parsing #60823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4fb5734
File modifier parsing
RikkiGibson Apr 18, 2022
fb624a1
give LangVersion error in binding
RikkiGibson Apr 18, 2022
e4efb02
Fix parsing members named 'file'
RikkiGibson Apr 20, 2022
eff66b0
add more error tests
RikkiGibson Apr 20, 2022
24a1f0d
update parsing baselines
RikkiGibson Apr 20, 2022
3f854fd
Apply suggestions from code review
RikkiGibson Apr 20, 2022
9fee3df
update gen code and disable public api check for now
RikkiGibson Apr 20, 2022
573ff74
Handle long sequences of file modifiers. Fix some compat scenarios. A…
RikkiGibson Apr 21, 2022
0bd3b9e
More tests. Always verify diagnostics.
RikkiGibson Apr 22, 2022
877ad9f
use fact instead of parameterless theory
RikkiGibson Apr 22, 2022
9698453
Address feedback
RikkiGibson Apr 27, 2022
228845e
Merge remote-tracking branch 'upstream/features/file-types' into ft-s…
RikkiGibson Apr 27, 2022
310528e
fix MessageID.RequiredVersion
RikkiGibson Apr 28, 2022
bb96370
Fix formatting
RikkiGibson Apr 28, 2022
a87fb15
Add tests
RikkiGibson Apr 28, 2022
d7c4b69
Address feedback
RikkiGibson Apr 28, 2022
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
98 changes: 32 additions & 66 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,6 @@ private static bool IsPossibleStartOfTypeDeclaration(SyntaxKind kind)
case SyntaxKind.SealedKeyword:
case SyntaxKind.StaticKeyword:
case SyntaxKind.UnsafeKeyword:
// PROTOTYPE(ft): it seems strange that we don't need this for any of the tests so far to pass.
// case SyntaxKind.FileKeyword:
case SyntaxKind.OpenBracketToken:
return true;
default:
Expand Down Expand Up @@ -1177,7 +1175,6 @@ internal static DeclarationModifiers GetModifier(SyntaxKind kind, SyntaxKind con

private void ParseModifiers(SyntaxListBuilder tokens, bool forAccessors)
{
bool? isFollowedByType = null;
while (true)
{
var newMod = GetModifier(this.CurrentToken);
Expand Down Expand Up @@ -1249,47 +1246,17 @@ private void ParseModifiers(SyntaxListBuilder tokens, bool forAccessors)
}

case DeclarationModifiers.File:
// 'file' is only a modifier if it is followed by an optional sequence of modifiers and then by a type keyword.
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.

{
isFollowedByType ??= computeFollowedByType();
if (isFollowedByType.GetValueOrDefault())
{
// this is a file modifier.
modTok = ConvertToKeyword(EatToken());
break;
}

return;

bool computeFollowedByType()
{
var resetPoint = this.GetResetPoint();
try
{
while (true)
{
if (IsTypeDeclarationStart())
{
return true;
}
else if (!SyntaxFacts.IsKeywordKind(CurrentToken.ContextualKind))
{
return false;
}

EatToken();
}
}
finally
{
this.Reset(ref resetPoint);
this.Release(ref resetPoint);
}
}
}

// 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 @@ -1326,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 @@ -1346,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 @@ -1377,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 @@ -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));
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs
View file
Open in desktop
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.FileKeyword; i++) // PROTOTYPE(ft): may conflict with required
for (int i = (int)SyntaxKind.YieldKeyword; i <= (int)SyntaxKind.FileKeyword; i++) // PROTOTYPE(ft): will conflict with required
{
yield return (SyntaxKind)i;
}
Expand Down
Loading