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

Support declaration and consumption of user-defined Unsigned Right Shift operator. #60616

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Support declaration and consumption of user-defined Unsigned Right Shift operator. #60616

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e804a5e
Support declaration and consumption of Unsigned Right Shift operator.
AlekseyTs Apr 5, 2022
b227fec
Add language version checks
AlekseyTs Apr 7, 2022
a419924
PR feedback
AlekseyTs Apr 11, 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
48 changes: 36 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ private BoundExpression BindCompoundAssignment(AssignmentExpressionSyntax node,
BinaryOperatorSignature bestSignature = best.Signature;

CheckNativeIntegerFeatureAvailability(bestSignature.Kind, node, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, bestSignature.Method, bestSignature.ConstrainedToTypeOpt, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, bestSignature.Method,
isUnsignedRightShift: bestSignature.Kind.Operator() == BinaryOperatorKind.UnsignedRightShift, bestSignature.ConstrainedToTypeOpt, diagnostics);

if (CheckOverflowAtRuntime)
{
Expand Down Expand Up @@ -401,7 +402,7 @@ private BoundExpression BindDynamicBinaryOperator(
else
{
Debug.Assert(left.Type is not TypeParameterSymbol);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, userDefinedOperator, constrainedToTypeOpt: null, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, userDefinedOperator, isUnsignedRightShift: false, constrainedToTypeOpt: null, diagnostics);
}
}

Expand Down Expand Up @@ -618,8 +619,12 @@ private BoundExpression BindSimpleBinaryOperator(BinaryExpressionSyntax node, Bi
break;
}

CheckNativeIntegerFeatureAvailability(resultOperatorKind, node, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, signature.ConstrainedToTypeOpt, diagnostics);
if (foundOperator)
{
CheckNativeIntegerFeatureAvailability(resultOperatorKind, node, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method,
isUnsignedRightShift: resultOperatorKind.Operator() == BinaryOperatorKind.UnsignedRightShift, signature.ConstrainedToTypeOpt, diagnostics);
}

TypeSymbol resultType = signature.ReturnType;
BoundExpression resultLeft = left;
Expand Down Expand Up @@ -948,8 +953,9 @@ private BoundExpression BindConditionalLogicalOperator(BinaryExpressionSyntax no
{
Debug.Assert(trueOperator != null && falseOperator != null);

_ = CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, signature.ConstrainedToTypeOpt, diagnostics) &&
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, kind == BinaryOperatorKind.LogicalAnd ? falseOperator : trueOperator, signature.ConstrainedToTypeOpt, diagnostics);
_ = CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics) &&
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, kind == BinaryOperatorKind.LogicalAnd ? falseOperator : trueOperator,
isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics);

return new BoundUserDefinedConditionalLogicalOperator(
node,
Expand Down Expand Up @@ -2264,7 +2270,7 @@ private BoundExpression BindIncrementOperator(CSharpSyntaxNode node, ExpressionS
var signature = best.Signature;

CheckNativeIntegerFeatureAvailability(signature.Kind, node, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, signature.ConstrainedToTypeOpt, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics);

var resultPlaceholder = new BoundValuePlaceholder(node, signature.ReturnType).MakeCompilerGenerated();

Expand Down Expand Up @@ -2311,7 +2317,7 @@ private BoundExpression BindIncrementOperator(CSharpSyntaxNode node, ExpressionS
/// <summary>
/// Returns false if reported an error, true otherwise.
/// </summary>
private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNode node, MethodSymbol? methodOpt, TypeSymbol? constrainedToTypeOpt, BindingDiagnosticBag diagnostics)
private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNode node, MethodSymbol? methodOpt, bool isUnsignedRightShift, TypeSymbol? constrainedToTypeOpt, BindingDiagnosticBag diagnostics)
Copy link
Member

@333fred 333fred Apr 11, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Consider just passing the operator kind, and letting the "is unsigned right shift" logic be contained within this method, rather than needing to be specified by a caller. #WontFix

{
bool result = true;

Expand Down Expand Up @@ -2342,10 +2348,28 @@ private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNo
}
}

if (methodOpt is not null && SyntaxFacts.IsCheckedOperator(methodOpt.Name) &&
Compilation.SourceModule != methodOpt.ContainingModule)
if (methodOpt is null)
{
if (isUnsignedRightShift)
{
result &= CheckFeatureAvailability(node, MessageID.IDS_FeatureUnsignedRightShift, diagnostics);
}
}
else
{
result &= CheckFeatureAvailability(node, MessageID.IDS_FeatureCheckedUserDefinedOperators, diagnostics);
Debug.Assert((methodOpt.Name == WellKnownMemberNames.UnsignedRightShiftOperatorName) == isUnsignedRightShift);

if (Compilation.SourceModule != methodOpt.ContainingModule)
Copy link
Member

@jcouv jcouv Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't understand why the unsigned right shift operator LangVer check is conditioned on resolved method being in source. I would have expected an unconditional LangVer diagnostic for >>> where ever the best signature is found. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't understand why the unsigned right shift operator LangVer check is conditioned on resolved method being in source. I would have expected an unconditional LangVer diagnostic for >>> where ever the best signature is found.

  1. This logic is not new
  2. If the operator is declared in the same module (note, not just in source), we perform the check on the declaration and report the error, if needed. Reporting the same error at every consumption site would only add more noise.

{
if (SyntaxFacts.IsCheckedOperator(methodOpt.Name))
{
result &= CheckFeatureAvailability(node, MessageID.IDS_FeatureCheckedUserDefinedOperators, diagnostics);
}
else if (isUnsignedRightShift)
{
result &= CheckFeatureAvailability(node, MessageID.IDS_FeatureUnsignedRightShift, diagnostics);
}
}
}

return result;
Expand Down Expand Up @@ -2686,7 +2710,7 @@ private BoundExpression BindUnaryOperatorCore(CSharpSyntaxNode node, string oper
var resultConstant = FoldUnaryOperator(node, resultOperatorKind, resultOperand, resultType, diagnostics);

CheckNativeIntegerFeatureAvailability(resultOperatorKind, node, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, signature.ConstrainedToTypeOpt, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics);

return new BoundUnaryOperator(
node,
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,7 +2547,7 @@ internal BoundExpression BindBooleanExpression(ExpressionSyntax node, BindingDia
destination: best.Signature.OperandType,
diagnostics: diagnostics);

CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, signature.ConstrainedToTypeOpt, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, signature.Method, isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics);

// Consider op_true to be compiler-generated so that it doesn't appear in the semantic model.
// UNDONE: If we decide to expose the operator in the semantic model, we'll have to remove the
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_TupleOperators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private TupleBinaryOperatorInfo BindTupleBinaryOperatorInfo(BinaryExpressionSynt
PrepareBoolConversionAndTruthOperator(binary.Type, node, kind, diagnostics,
out BoundExpression conversionIntoBoolOperator, out BoundValuePlaceholder conversionIntoBoolOperatorPlaceholder,
out UnaryOperatorSignature boolOperator);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, boolOperator.Method, boolOperator.ConstrainedToTypeOpt, diagnostics);
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, boolOperator.Method, isUnsignedRightShift: false, boolOperator.ConstrainedToTypeOpt, diagnostics);

return new TupleBinaryOperatorInfo.Single(binary.Left.Type, binary.Right.Type, binary.OperatorKind, binary.Method, binary.ConstrainedToType,
conversionIntoBoolOperatorPlaceholder, conversionIntoBoolOperator, boolOperator);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/Operators/OperatorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ internal static string BinaryOperatorNameFromSyntaxKindIfAny(SyntaxKind kind, bo
case SyntaxKind.GreaterThanToken: return WellKnownMemberNames.GreaterThanOperatorName;
case SyntaxKind.GreaterThanEqualsToken: return WellKnownMemberNames.GreaterThanOrEqualOperatorName;
case SyntaxKind.GreaterThanGreaterThanToken: return WellKnownMemberNames.RightShiftOperatorName;
// PROTOTYPE(UnsignedRightShift): case SyntaxKind.GreaterThanGreaterThanGreaterThanToken: return WellKnownMemberNames.UnsignedRightShiftOperatorName;
case SyntaxKind.GreaterThanGreaterThanGreaterThanToken: return WellKnownMemberNames.UnsignedRightShiftOperatorName;
case SyntaxKind.ExclamationEqualsToken: return WellKnownMemberNames.InequalityOperatorName;
default:
return null;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/Operators/OperatorKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ internal enum BinaryOperatorKind
ULongUnsignedRightShift = ULong | UnsignedRightShift,
NIntUnsignedRightShift = NInt | UnsignedRightShift,
NUIntUnsignedRightShift = NUInt | UnsignedRightShift,
// PROTOTYPE(UnsignedRightShift): UserDefinedUnsignedRightShift = UserDefined | UnsignedRightShift,
UserDefinedUnsignedRightShift = UserDefined | UnsignedRightShift,
LiftedIntUnsignedRightShift = Lifted | Int | UnsignedRightShift,
LiftedUIntUnsignedRightShift = Lifted | UInt | UnsignedRightShift,
LiftedLongUnsignedRightShift = Lifted | Long | UnsignedRightShift,
Expand Down
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 @@ -7088,4 +7088,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExpressionTreeContainsUTF8StringLiterals" xml:space="preserve">
<value>An expression tree may not contain UTF8 string conversion or literal.</value>
</data>
<data name="IDS_FeatureUnsignedRightShift" xml:space="preserve">
<value>unsigned right shift</value>
</data>
</root>
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/CodeGen/EmitOperators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,8 @@ private static bool IsUnsigned(SpecialType type)
private static bool IsUnsignedBinaryOperator(BoundBinaryOperator op)
{
BinaryOperatorKind opKind = op.OperatorKind;
Debug.Assert(opKind.Operator() != BinaryOperatorKind.UnsignedRightShift);

BinaryOperatorKind type = opKind.OperandTypes();
switch (type)
{
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,8 @@ internal enum MessageID

IDS_FeatureCheckedUserDefinedOperators = MessageBase + 12821,
IDS_FeatureUTF8StringLiterals = MessageBase + 12822,

IDS_FeatureUnsignedRightShift = MessageBase + 12901, // PROTOTYPE(UnsignedRightShift): pack numbers
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -370,6 +372,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureAutoDefaultStructs: // semantic check
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
return LanguageVersion.Preview;

// C# 10.0 features.
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Parser/DocumentationCommentParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,8 @@ CurrentToken.Kind is (SyntaxKind.GreaterThanToken or SyntaxKind.GreaterThanEqual
operatorToken.Text + operatorToken2.Text + operatorToken3.Text,
operatorToken.ValueText + operatorToken2.ValueText + operatorToken3.ValueText,
operatorToken3.GetTrailingTrivia());

operatorToken = CheckFeatureAvailability(operatorToken, MessageID.IDS_FeatureUnsignedRightShift, forceWarning: true);
Copy link
Member

@jcouv jcouv Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Why do a parser LangVer check as opposed to add check in Binder_Cref? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do a parser LangVer check as opposed to add check in Binder_Cref?

Just following the established pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these are warnings, not errors

}
else
{
Expand All @@ -1057,7 +1059,7 @@ CurrentToken.Kind is (SyntaxKind.GreaterThanToken or SyntaxKind.GreaterThanEqual
int width = nonOverloadableOperator.Width;
SyntaxDiagnosticInfo rawInfo = new SyntaxDiagnosticInfo(offset, width, ErrorCode.ERR_OvlOperatorExpected);
SyntaxDiagnosticInfo crefInfo = new SyntaxDiagnosticInfo(offset, width, ErrorCode.WRN_ErrorOverride, rawInfo, rawInfo.Code);
operatorToken = WithAdditionalDiagnostics(operatorToken, crefInfo); // PROTOTYPE(UnsignedRightShift): Add a test for this warning
operatorToken = WithAdditionalDiagnostics(operatorToken, crefInfo);
}
}
else
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,7 @@ private MethodKind ComputeMethodKind()
case WellKnownMemberNames.CheckedMultiplyOperatorName:
case WellKnownMemberNames.MultiplyOperatorName:
case WellKnownMemberNames.RightShiftOperatorName:
case WellKnownMemberNames.UnsignedRightShiftOperatorName:
case WellKnownMemberNames.CheckedSubtractionOperatorName:
case WellKnownMemberNames.SubtractionOperatorName:
return IsValidUserDefinedOperatorSignature(2) ? MethodKind.UserDefinedOperator : MethodKind.Ordinary;
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceUserDefinedOperatorSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ public static SourceUserDefinedOperatorSymbol CreateUserDefinedOperatorSymbol(
diagnostics.Add(ErrorCode.ERR_OperatorCantBeChecked, syntax.CheckedKeyword.GetLocation(), SyntaxFacts.GetText(SyntaxFacts.GetOperatorKind(name)));
}

if (name == WellKnownMemberNames.UnsignedRightShiftOperatorName)
{
MessageID.IDS_FeatureUnsignedRightShift.CheckFeatureAvailability(diagnostics, syntax, syntax.OperatorToken.GetLocation());
}

var interfaceSpecifier = syntax.ExplicitInterfaceSpecifier;

TypeSymbol explicitInterfaceType;
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceUserDefinedOperatorSymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ private void CheckOperatorSignatures(BindingDiagnosticBag diagnostics)

case WellKnownMemberNames.LeftShiftOperatorName:
case WellKnownMemberNames.RightShiftOperatorName:
case WellKnownMemberNames.UnsignedRightShiftOperatorName:
Copy link
Member

@jcouv jcouv Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: consider updating comment in CheckShiftSignature (add >>>) #Closed

CheckShiftSignature(diagnostics);
break;

Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ public static bool IsAssignmentExpressionOperatorToken(SyntaxKind token)
case SyntaxKind.CaretEqualsToken:
case SyntaxKind.LessThanLessThanEqualsToken:
case SyntaxKind.GreaterThanGreaterThanEqualsToken:
// PROTOTYPE(UnsignedRightShift): case SyntaxKind.GreaterThanGreaterThanGreaterThanEqualsToken:
case SyntaxKind.GreaterThanGreaterThanGreaterThanEqualsToken:
case SyntaxKind.PlusEqualsToken:
case SyntaxKind.MinusEqualsToken:
case SyntaxKind.AsteriskEqualsToken:
Expand Down Expand Up @@ -1040,7 +1040,7 @@ public static SyntaxKind GetOperatorKind(string operatorMetadataName)

case WellKnownMemberNames.OnesComplementOperatorName: return SyntaxKind.TildeToken;
case WellKnownMemberNames.RightShiftOperatorName: return SyntaxKind.GreaterThanGreaterThanToken;
// PROTOTYPE(UnsignedRightShift): case WellKnownMemberNames.UnsignedRightShiftOperatorName: return SyntaxKind.GreaterThanGreaterThanGreaterThanToken;
case WellKnownMemberNames.UnsignedRightShiftOperatorName: return SyntaxKind.GreaterThanGreaterThanGreaterThanToken;

case WellKnownMemberNames.CheckedSubtractionOperatorName:
case WellKnownMemberNames.SubtractionOperatorName:
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.

Loading