Skip to content

Improve SymbolDisplay to handle operator symbols that are not valid operators for the target language. #64109

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

Merged
AlekseyTs merged 1 commit into from
Sep 21, 2022
Merged

Improve SymbolDisplay to handle operator symbols that are not valid operators for the target language. #64109

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
95 changes: 70 additions & 25 deletions src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,6 @@ public override void VisitMethod(IMethodSymbol symbol)
case MethodKind.StaticConstructor:
break;
case MethodKind.Destructor:
case MethodKind.Conversion:
// If we're using the metadata format, then include the return type.
// Otherwise we eschew it since it is redundant in a conversion
// signature.
Expand All @@ -337,6 +336,18 @@ public override void VisitMethod(IMethodSymbol symbol)
goto default;
}

break;

case MethodKind.Conversion:
// If we're using the metadata format, then include the return type.
// Otherwise we eschew it since it is redundant in a conversion
// signature.
if (format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) ||
tryGetUserDefinedOperatorTokenKind(symbol.MetadataName) == SyntaxKind.None)
{
goto default;
}

break;
default:
// The display code is called by the debugger; if a developer is debugging Roslyn and attempts
Expand Down Expand Up @@ -503,11 +514,11 @@ public override void VisitMethod(IMethodSymbol symbol)

if (sourceUserDefinedOperatorSymbolBase is SourceUserDefinedConversionSymbol)
{
addUserDefinedConversionName(symbol, operatorName);
addUserDefinedConversionName(symbol, tryGetUserDefinedConversionTokenKind(operatorName), operatorName);
Copy link
Member

@jaredpar jaredpar Sep 19, 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.

There is an implicit dependency here that tryGetUserDefinedConversionTokenKind does not return SyntaxKind.None. Why is it safe to assume that on this code path but other code paths explicitly handle its return? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Sep 19, 2022

Choose a reason for hiding this comment

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

Why is it safe to assume that on this code path but other code paths explicitly handle its return?

This code path handles C# symbols (see is SourceUserDefinedOperatorSymbolBase check above, for those the helper is expected to always succeed.

}
else
{
addUserDefinedOperatorName(symbol, operatorName);
addUserDefinedOperatorName(symbol, tryGetUserDefinedOperatorTokenKind(operatorName), operatorName);
}
break;
}
Expand All @@ -525,7 +536,16 @@ public override void VisitMethod(IMethodSymbol symbol)
}
else
{
addUserDefinedOperatorName(symbol, symbol.MetadataName);
SyntaxKind operatorKind = tryGetUserDefinedOperatorTokenKind(symbol.MetadataName);

if (operatorKind == SyntaxKind.None)
{
builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol, symbol.Name));
}
else
{
addUserDefinedOperatorName(symbol, operatorKind, symbol.MetadataName);
}
}
break;
}
Expand All @@ -537,7 +557,16 @@ public override void VisitMethod(IMethodSymbol symbol)
}
else
{
addUserDefinedConversionName(symbol, symbol.MetadataName);
SyntaxKind conversionKind = tryGetUserDefinedConversionTokenKind(symbol.MetadataName);

if (conversionKind == SyntaxKind.None)
{
builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol, symbol.Name));
}
else
{
addUserDefinedConversionName(symbol, conversionKind, symbol.MetadataName);
}
}
break;
}
Expand Down Expand Up @@ -654,16 +683,34 @@ void visitFunctionPointerSignature(IMethodSymbol symbol)
AddPunctuation(SyntaxKind.GreaterThanToken);
}

void addUserDefinedOperatorName(IMethodSymbol symbol, string operatorName)
static SyntaxKind tryGetUserDefinedOperatorTokenKind(string operatorName)
{
if (operatorName == WellKnownMemberNames.TrueOperatorName)
{
return SyntaxKind.TrueKeyword;
}
else if (operatorName == WellKnownMemberNames.FalseOperatorName)
{
return SyntaxKind.FalseKeyword;
}
else
{
return SyntaxFacts.GetOperatorKind(operatorName);
}
}

void addUserDefinedOperatorName(IMethodSymbol symbol, SyntaxKind operatorKind, string operatorName)
{
Debug.Assert(operatorKind != SyntaxKind.None);

AddKeyword(SyntaxKind.OperatorKeyword);
AddSpace();

if (operatorName == WellKnownMemberNames.TrueOperatorName)
if (operatorKind == SyntaxKind.TrueKeyword)
{
AddKeyword(SyntaxKind.TrueKeyword);
}
else if (operatorName == WellKnownMemberNames.FalseOperatorName)
else if (operatorKind == SyntaxKind.FalseKeyword)
{
AddKeyword(SyntaxKind.FalseKeyword);
}
Expand All @@ -676,40 +723,38 @@ void addUserDefinedOperatorName(IMethodSymbol symbol, string operatorName)
}

builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol,
SyntaxFacts.GetText(SyntaxFacts.GetOperatorKind(operatorName))));
SyntaxFacts.GetText(operatorKind)));
}
}

void addUserDefinedConversionName(IMethodSymbol symbol, string operatorName)
static SyntaxKind tryGetUserDefinedConversionTokenKind(string operatorName)
{
// "System.IntPtr.explicit operator System.IntPtr(int)"

bool isChecked = false;

if (operatorName == WellKnownMemberNames.ExplicitConversionName)
{
AddKeyword(SyntaxKind.ExplicitKeyword);
}
else if (operatorName == WellKnownMemberNames.CheckedExplicitConversionName)
if (operatorName is WellKnownMemberNames.ExplicitConversionName or WellKnownMemberNames.CheckedExplicitConversionName)
{
isChecked = true;
AddKeyword(SyntaxKind.ExplicitKeyword);
return SyntaxKind.ExplicitKeyword;
}
else if (operatorName == WellKnownMemberNames.ImplicitConversionName)
{
AddKeyword(SyntaxKind.ImplicitKeyword);
return SyntaxKind.ImplicitKeyword;
}
else
{
builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol,
SyntaxFacts.GetText(SyntaxFacts.GetOperatorKind(operatorName))));
return SyntaxKind.None;
}
}

void addUserDefinedConversionName(IMethodSymbol symbol, SyntaxKind conversionKind, string operatorName)
{
// "System.IntPtr.explicit operator System.IntPtr(int)"

Debug.Assert(conversionKind != SyntaxKind.None);
AddKeyword(conversionKind);

AddSpace();
AddKeyword(SyntaxKind.OperatorKeyword);
AddSpace();

if (isChecked)
if (operatorName == WellKnownMemberNames.CheckedExplicitConversionName)
{
AddKeyword(SyntaxKind.CheckedKeyword);
AddSpace();
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/Test/Core/Compilation/OperationTreeVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ public static string GetOperationTree(Compilation compilation, IOperation operat
{
var walker = new OperationTreeVerifier(compilation, operation, initialIndent);
walker.Visit(operation);

var visitor = TestOperationVisitor.Singleton;
foreach (var op in operation.DescendantsAndSelf())
{
visitor.Visit(op);
}

return walker._builder.ToString();
}

Expand Down
51 changes: 33 additions & 18 deletions src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,9 +736,7 @@ public override void VisitUnaryOperator(IUnaryOperation operation)
AssertConstrainedToType(operatorMethod, operation.ConstrainedToType);
Assert.Same(operation.Operand, operation.ChildOperations.Single());

// Directly get the symbol for this operator from the semantic model. This allows us to exercise
// potentially creating synthesized intrinsic operators.
CheckBuiltInOperators(operation.SemanticModel, operation.Syntax);
CheckOperators(operation.SemanticModel, operation.Syntax);
}

public override void VisitBinaryOperator(IBinaryOperation operation)
Expand Down Expand Up @@ -773,29 +771,38 @@ public override void VisitBinaryOperator(IBinaryOperation operation)

AssertEx.Equal(new[] { operation.LeftOperand, operation.RightOperand }, operation.ChildOperations);

// Directly get the symbol for this operator from the semantic model. This allows us to exercise
// potentially creating synthesized intrinsic operators.
CheckBuiltInOperators(operation.SemanticModel, operation.Syntax);
CheckOperators(operation.SemanticModel, operation.Syntax);
}

private static void CheckBuiltInOperators(SemanticModel semanticModel, SyntaxNode syntax)
private static void CheckOperators(SemanticModel semanticModel, SyntaxNode syntax)
{
// Directly get the symbol for this operator from the semantic model. This allows us to exercise
// potentially creating synthesized intrinsic operators.
var symbolInfo = semanticModel?.GetSymbolInfo(syntax) ?? default;

foreach (var symbol in symbolInfo.GetAllSymbols())
{
if (symbol is IMethodSymbol { MethodKind: MethodKind.BuiltinOperator } method)
if (symbol is IMethodSymbol method)
{
switch (method.Parameters.Length)
VisualBasic.SymbolDisplay.ToDisplayString(method, SymbolDisplayFormat.TestFormat);
VisualBasic.SymbolDisplay.ToDisplayString(method);
CSharp.SymbolDisplay.ToDisplayString(method, SymbolDisplayFormat.TestFormat);
CSharp.SymbolDisplay.ToDisplayString(method);

if (method.MethodKind == MethodKind.BuiltinOperator)
{
case 1:
semanticModel.Compilation.CreateBuiltinOperator(symbol.Name, method.ReturnType, method.Parameters[0].Type);
break;
case 2:
semanticModel.Compilation.CreateBuiltinOperator(symbol.Name, method.ReturnType, method.Parameters[0].Type, method.Parameters[1].Type);
break;
default:
AssertEx.Fail($"Unexpected parameter count for built in method: {method.ToDisplayString()}");
break;
switch (method.Parameters.Length)
{
case 1:
semanticModel.Compilation.CreateBuiltinOperator(symbol.Name, method.ReturnType, method.Parameters[0].Type);
break;
case 2:
semanticModel.Compilation.CreateBuiltinOperator(symbol.Name, method.ReturnType, method.Parameters[0].Type, method.Parameters[1].Type);
break;
default:
AssertEx.Fail($"Unexpected parameter count for built in method: {method.ToDisplayString()}");
break;
}
}
}
}
Expand Down Expand Up @@ -836,6 +843,14 @@ public override void VisitConversion(IConversionOperation operation)
}

Assert.Same(operation.Operand, operation.ChildOperations.Single());

if (operatorMethod != null)
{
VisualBasic.SymbolDisplay.ToDisplayString(operatorMethod, SymbolDisplayFormat.TestFormat);
VisualBasic.SymbolDisplay.ToDisplayString(operatorMethod);
CSharp.SymbolDisplay.ToDisplayString(operatorMethod, SymbolDisplayFormat.TestFormat);
CSharp.SymbolDisplay.ToDisplayString(operatorMethod);
}
}

private static void AssertConstrainedToType(ISymbol member, ITypeSymbol constrainedToType)
Expand Down
67 changes: 55 additions & 12 deletions src/Compilers/VisualBasic/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,24 +242,24 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
End If

Case MethodKind.Conversion
If format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then

Dim tokenKind As SyntaxKind = TryGetConversionTokenKind(symbol)

If tokenKind = SyntaxKind.None OrElse format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
AddKeyword(SyntaxKind.FunctionKeyword)
AddSpace()
Else
If CaseInsensitiveComparison.Equals(symbol.Name, WellKnownMemberNames.ImplicitConversionName) Then
AddKeyword(SyntaxKind.WideningKeyword)
AddSpace()
Else
AddKeyword(SyntaxKind.NarrowingKeyword)
AddSpace()
End If
AddKeyword(tokenKind)
AddSpace()

AddKeyword(SyntaxKind.OperatorKeyword)
AddSpace()
End If

Case MethodKind.UserDefinedOperator, MethodKind.BuiltinOperator
If format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
Dim tokenKind As SyntaxKind = TryGetOperatorTokenKind(symbol)

If tokenKind = SyntaxKind.None OrElse format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
AddKeyword(SyntaxKind.FunctionKeyword)
AddSpace()
Else
Expand Down Expand Up @@ -343,14 +343,19 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
End If

Case MethodKind.UserDefinedOperator, MethodKind.BuiltinOperator
If format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then

Dim tokenKind As SyntaxKind = TryGetOperatorTokenKind(symbol)

If tokenKind = SyntaxKind.None OrElse format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol, symbol.Name, visitedParents))
Else
AddKeyword(OverloadResolution.GetOperatorTokenKind(symbol.Name))
AddKeyword(tokenKind)
End If

Case MethodKind.Conversion
If format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
Dim tokenKind As SyntaxKind = TryGetConversionTokenKind(symbol)

If tokenKind = SyntaxKind.None OrElse format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames) Then
builder.Add(CreatePart(SymbolDisplayPartKind.MethodName, symbol, symbol.Name, visitedParents))
Else
AddKeyword(SyntaxKind.CTypeKeyword)
Expand All @@ -366,6 +371,44 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
End Select
End Sub

Private Shared Function TryGetOperatorTokenKind(symbol As IMethodSymbol) As SyntaxKind
Dim nameToCheck As String = symbol.Name

If symbol.MethodKind = MethodKind.BuiltinOperator Then
Select Case nameToCheck
Case WellKnownMemberNames.CheckedAdditionOperatorName
nameToCheck = WellKnownMemberNames.AdditionOperatorName
Case WellKnownMemberNames.CheckedDivisionOperatorName
nameToCheck = WellKnownMemberNames.IntegerDivisionOperatorName
Case WellKnownMemberNames.CheckedMultiplyOperatorName
nameToCheck = WellKnownMemberNames.MultiplyOperatorName
Case WellKnownMemberNames.CheckedSubtractionOperatorName
nameToCheck = WellKnownMemberNames.SubtractionOperatorName
Case WellKnownMemberNames.CheckedUnaryNegationOperatorName
nameToCheck = WellKnownMemberNames.UnaryNegationOperatorName
End Select
End If

Dim opInfo As OverloadResolution.OperatorInfo = OverloadResolution.GetOperatorInfo(nameToCheck)

If (opInfo.IsUnary AndAlso opInfo.UnaryOperatorKind <> UnaryOperatorKind.Error) OrElse
(opInfo.IsBinary AndAlso opInfo.BinaryOperatorKind <> BinaryOperatorKind.Error) Then
Return OverloadResolution.GetOperatorTokenKind(opInfo)
Else
Return SyntaxKind.None
End If
End Function

Private Shared Function TryGetConversionTokenKind(symbol As IMethodSymbol) As SyntaxKind
If CaseInsensitiveComparison.Equals(symbol.Name, WellKnownMemberNames.ImplicitConversionName) Then
Return SyntaxKind.WideningKeyword
ElseIf CaseInsensitiveComparison.Equals(symbol.Name, WellKnownMemberNames.ExplicitConversionName) Then
Return SyntaxKind.NarrowingKeyword
Else
Return SyntaxKind.None
End If
End Function

Private Sub AddMethodGenericParameters(method As IMethodSymbol)
If method.Arity > 0 AndAlso format.GenericsOptions.IncludesOption(SymbolDisplayGenericsOptions.IncludeTypeParameters) Then
AddTypeArguments(method.TypeArguments)
Expand Down
Loading