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

Parse unchecked gracefully in operators #61309

Merged
merged 1 commit into from
May 19, 2022
Merged

Parse unchecked gracefully in operators #61309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7094,4 +7094,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureRelaxedShiftOperator" xml:space="preserve">
<value>relaxed shift operator</value>
</data>
<data name="ERR_MisplacedUnchecked" xml:space="preserve">
<value>Unexpected keyword 'unchecked'</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2068,6 +2068,7 @@ internal enum ErrorCode

ERR_CannotBeConvertedToUTF8 = 9026,
ERR_ExpressionTreeContainsUTF8StringLiterals = 9027,
ERR_MisplacedUnchecked = 9028,

#endregion

Expand Down
21 changes: 17 additions & 4 deletions src/Compilers/CSharp/Portable/Parser/DocumentationCommentParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,7 @@ private OperatorMemberCrefSyntax ParseOperatorMemberCref()
{
Debug.Assert(CurrentToken.Kind == SyntaxKind.OperatorKeyword);
SyntaxToken operatorKeyword = EatToken();
SyntaxToken checkedKeyword = TryEatCheckedKeyword(isConversion: false);
SyntaxToken checkedKeyword = TryEatCheckedKeyword(isConversion: false, ref operatorKeyword);

SyntaxToken operatorToken;

Expand Down Expand Up @@ -1113,9 +1113,9 @@ CurrentToken.Kind is (SyntaxKind.GreaterThanToken or SyntaxKind.GreaterThanEqual
return SyntaxFactory.OperatorMemberCref(operatorKeyword, checkedKeyword, operatorToken, parameters);
}

private SyntaxToken TryEatCheckedKeyword(bool isConversion)
private SyntaxToken TryEatCheckedKeyword(bool isConversion, ref SyntaxToken operatorKeyword)
{
SyntaxToken checkedKeyword = TryEatToken(SyntaxKind.CheckedKeyword); // https://github.com/dotnet/roslyn/issues/60394 : consider gracefully recovering from erroneous use of 'unchecked' at this location
SyntaxToken checkedKeyword = tryEatCheckedOrHandleUnchecked(ref operatorKeyword);

if (checkedKeyword is not null &&
(isConversion || SyntaxFacts.IsAnyOverloadableOperator(CurrentToken.Kind)))
Expand All @@ -1124,6 +1124,19 @@ private SyntaxToken TryEatCheckedKeyword(bool isConversion)
}

return checkedKeyword;

SyntaxToken tryEatCheckedOrHandleUnchecked(ref SyntaxToken operatorKeyword)
{
if (CurrentToken.Kind == SyntaxKind.UncheckedKeyword)
{
// if we encounter `operator unchecked`, we place the `unchecked` as skipped trivia on `operator`
var misplacedToken = this.AddError(this.EatToken(), ErrorCode.ERR_MisplacedUnchecked);
operatorKeyword = AddTrailingSkippedSyntax(operatorKeyword, misplacedToken);
return null;
}

return TryEatToken(SyntaxKind.CheckedKeyword);
}
}

/// <summary>
Expand All @@ -1136,7 +1149,7 @@ private ConversionOperatorMemberCrefSyntax ParseConversionOperatorMemberCref()
SyntaxToken implicitOrExplicit = EatToken();

SyntaxToken operatorKeyword = EatToken(SyntaxKind.OperatorKeyword);
SyntaxToken checkedKeyword = TryEatCheckedKeyword(isConversion: true);
SyntaxToken checkedKeyword = TryEatCheckedKeyword(isConversion: true, ref operatorKeyword);

TypeSyntax type = ParseCrefType(typeArgumentsMustBeIdentifiers: false);

Expand Down
19 changes: 16 additions & 3 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 @@ -3348,7 +3348,7 @@ private ConversionOperatorDeclarationSyntax TryParseConversionOperatorDeclaratio
{
possibleConversion = false;
}
else if (this.PeekToken(1).Kind == SyntaxKind.CheckedKeyword) // https://github.com/dotnet/roslyn/issues/60394 : consider gracefully recovering from erroneous use of 'unchecked' at this location
else if (this.PeekToken(1).Kind is SyntaxKind.CheckedKeyword or SyntaxKind.UncheckedKeyword)
{
possibleConversion = !SyntaxFacts.IsAnyOverloadableOperator(this.PeekToken(2).Kind);
}
Expand Down Expand Up @@ -3392,7 +3392,7 @@ private ConversionOperatorDeclarationSyntax TryParseConversionOperatorDeclaratio
}

opKeyword = this.EatToken(SyntaxKind.OperatorKeyword);
var checkedKeyword = this.TryEatToken(SyntaxKind.CheckedKeyword); // https://github.com/dotnet/roslyn/issues/60394 : consider gracefully recovering from erroneous use of 'unchecked' at this location
var checkedKeyword = TryEatCheckedOrHandleUnchecked(ref opKeyword);

this.Release(ref point);
point = GetResetPoint();
Expand Down Expand Up @@ -3510,14 +3510,27 @@ ExplicitInterfaceSpecifierSyntax tryParseExplicitInterfaceSpecifier()
}
}

private SyntaxToken TryEatCheckedOrHandleUnchecked(ref SyntaxToken operatorKeyword)
{
if (CurrentToken.Kind == SyntaxKind.UncheckedKeyword)
Copy link
Member

Choose a reason for hiding this comment

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

@CyrusNajmabadi, is this an example of an error you'd prefer to see done in binding, or is parsing ok for a misplaced token error?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we can use the existing ERR_SyntaxError error?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, this needs to be in the parser in the current form. The reason for that is that the syntax model itself is restrictive here. e.g. it requires a SyntaxToken CheckedKeyword. As such, we cannot fit the user code into the syntax model, and we have to make a correction at the parsing level, which means having a diagnostic at the parser level.

My overall preference (if we had time/motivation) would be to relax the syntax model so that both of checked/unchecked were legal from the model's perspective, and then have it be the binder that checks and states taht 'unchecked' is not actually allowed semantically.

But this is also not a big deal for me (given how rare these will be) so keeping the status quo (and taking this PR) seems totally reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried relaxing the syntax model to allow either checked or unchecked keywords, but ran into an unfortunate issue with public APIs...

The syntax.xml change updates the API OperatorMemberCref(SyntaxToken operatorToken, CrefParameterListSyntax? parameters) to OperatorMemberCrefSyntax OperatorMemberCref(SyntaxToken checkedKeyword, SyntaxToken operatorToken, CrefParameterListSyntax? parameters).

But that conflicts with existing manually defined public API: OperatorMemberCref(SyntaxToken operatorKeyword, SyntaxToken operatorToken, CrefParameterListSyntax? parameters).

@333fred Given that, I think we should keep the parser-level handling.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we could likely only do this if we haven't publicly shipped the 'checked operator' work yet. if it isn't shipped, then we can revise. otehrwise, we're locked in.

Copy link
Member Author

Choose a reason for hiding this comment

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

The checked operator feature is still in preview. But the public API that is causing a conflict predates that (doesn't involved checked at all). So I think we're stuck.

Copy link
Member Author

Choose a reason for hiding this comment

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

@333fred ptal

{
// if we encounter `operator unchecked`, we place the `unchecked` as skipped trivia on `operator`
var misplacedToken = this.AddError(this.EatToken(), ErrorCode.ERR_MisplacedUnchecked);
operatorKeyword = AddTrailingSkippedSyntax(operatorKeyword, misplacedToken);
return null;
}

return TryEatToken(SyntaxKind.CheckedKeyword);
}

private OperatorDeclarationSyntax ParseOperatorDeclaration(
SyntaxList<AttributeListSyntax> attributes,
SyntaxListBuilder modifiers,
TypeSyntax type,
ExplicitInterfaceSpecifierSyntax explicitInterfaceOpt)
{
var opKeyword = this.EatToken(SyntaxKind.OperatorKeyword);
var checkedKeyword = this.TryEatToken(SyntaxKind.CheckedKeyword); // https://github.com/dotnet/roslyn/issues/60394 : consider gracefully recovering from erroneous use of 'unchecked' at this location
var checkedKeyword = TryEatCheckedOrHandleUnchecked(ref opKeyword);
SyntaxToken opToken;
int opTokenErrorOffset;
int opTokenErrorWidth;
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Parser/SyntaxParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1086,7 +1086,6 @@ protected TNode CheckFeatureAvailability<TNode>(TNode node, MessageID feature, b
where TNode : GreenNode
{
LanguageVersion availableVersion = this.Options.LanguageVersion;
LanguageVersion requiredVersion = feature.RequiredVersion();

// There are special error codes for some features, so handle those separately.
switch (feature)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,7 +1587,7 @@ public static SyntaxTriviaList ParseLeadingTrivia(string text, int offset = 0)
/// <summary>
/// Parse a list of trivia rules for leading trivia.
/// </summary>
internal static SyntaxTriviaList ParseLeadingTrivia(string text, CSharpParseOptions? options, int offset = 0)
internal static SyntaxTriviaList ParseLeadingTrivia(string text, CSharpParseOptions options, int offset = 0)
{
using (var lexer = new InternalSyntax.Lexer(MakeSourceText(text, offset), options))
{
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

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
View file
Open in desktop

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
View file
Open in desktop

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
View file
Open in desktop

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
View file
Open in desktop

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
View file
Open in desktop

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
View file
Open in desktop

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.pl.xlf
View file
Open in desktop

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.pt-BR.xlf
View file
Open in desktop

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.ru.xlf
View file
Open in desktop

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.tr.xlf
View file
Open in desktop

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.zh-Hans.xlf
View file
Open in desktop

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.zh-Hant.xlf
View file
Open in desktop

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

Loading