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

Fix Function Pointer RefKind Display #51223

Merged
merged 12 commits into from
Mar 17, 2021
Merged

Fix Function Pointer RefKind Display #51223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9c99cbc
Fix Function Pointer Display
YairHalberstadt Feb 14, 2021
a3f324d
SignatureOnlyParameterSymbol.ContainingSymbol throws
YairHalberstadt Feb 14, 2021
ad7ddea
Fix broken test
YairHalberstadt Feb 14, 2021
04b0378
Changes based on review by @alekseyts and @333fred
YairHalberstadt Feb 18, 2021
ef0d6cc
don't check for function pointer at all in SymbolDisplayVisitor.Visit…
YairHalberstadt Feb 18, 2021
d31a3dc
Fix broken tests, and address feedback from @AlekseyTs
YairHalberstadt Mar 11, 2021
cfbe45c
check for empty name instead of function pointer symbol in VisitParam…
YairHalberstadt Mar 11, 2021
9799b7d
rename variable from includeOption to includeDefaultValue
YairHalberstadt Mar 11, 2021
6f6e883
unconditionally display ref kinds for function pointers
YairHalberstadt Mar 15, 2021
e3f11f5
Fix couple of broken tests
YairHalberstadt Mar 16, 2021
18f2bde
Merge branch 'main' of https://github.com/dotnet/roslyn into fix-func…
YairHalberstadt Mar 16, 2021
cb03900
Fix tests
YairHalberstadt Mar 17, 2021
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
102 changes: 64 additions & 38 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 @@ -609,18 +609,25 @@ void visitFunctionPointerSignature(IMethodSymbol symbol)

foreach (var param in symbol.Parameters)
{
param.Accept(this.NotFirstVisitor);
AddParameterRefKind(param.RefKind);

AddCustomModifiersIfRequired(param.RefCustomModifiers);

param.Type.Accept(this.NotFirstVisitor);

AddCustomModifiersIfRequired(param.CustomModifiers, leadingSpace: true, trailingSpace: false);

AddPunctuation(SyntaxKind.CommaToken);
AddSpace();
}

if (symbol.ReturnsByRef)
{
AddRefIfRequired();
AddRef();
}
else if (symbol.ReturnsByRefReadonly)
{
AddRefReadonlyIfRequired();
AddRefReadonly();
}

AddCustomModifiersIfRequired(symbol.RefCustomModifiers);
Expand Down Expand Up @@ -679,9 +686,13 @@ public override void VisitParameter(IParameterSymbol symbol)
// used on their own or in the context of methods.

var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

var includeType [](start = 12, length = 15)

It appears an extra empty line was added above this line. Could be an issue with the diff tool though. #Closed

var includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName)
&& !(symbol.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.FunctionPointerSignature });
var includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName) &&
symbol.Name.Length != 0;
var includeBrackets = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeOptionalBrackets);
var includeDefaultValue = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) &&
format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName) &&
symbol.HasExplicitDefaultValue &&
CanAddConstant(symbol.Type, symbol.ExplicitDefaultValue);
Copy link
Member

Choose a reason for hiding this comment

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

With the latest changes to how function pointers are printed, are any of these changes to VisitParameter necessary anymore?


if (includeBrackets && symbol.IsOptional)
{
Expand All @@ -703,26 +714,26 @@ public override void VisitParameter(IParameterSymbol symbol)
AddCustomModifiersIfRequired(symbol.CustomModifiers, leadingSpace: true, trailingSpace: false);
}

if (includeName && includeType)
{
AddSpace();
}

if (includeName)
{
if (includeType)
{
AddSpace();
}
var kind = symbol.IsThis ? SymbolDisplayPartKind.Keyword : SymbolDisplayPartKind.ParameterName;
builder.Add(CreatePart(kind, symbol, symbol.Name));
}

if (format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) &&
symbol.HasExplicitDefaultValue &&
CanAddConstant(symbol.Type, symbol.ExplicitDefaultValue))
if (includeDefaultValue)
{
if (includeName || includeType)
{
AddSpace();
AddPunctuation(SyntaxKind.EqualsToken);
AddSpace();

AddConstantValue(symbol.Type, symbol.ExplicitDefaultValue);
}
AddPunctuation(SyntaxKind.EqualsToken);
AddSpace();

AddConstantValue(symbol.Type, symbol.ExplicitDefaultValue);
}

if (includeBrackets && symbol.IsOptional)
Expand Down Expand Up @@ -936,22 +947,32 @@ private void AddRefIfRequired()
{
if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef))
{
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
AddRef();
}
}

private void AddRef()
{
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
}

private void AddRefReadonlyIfRequired()
{
if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef))
{
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
AddKeyword(SyntaxKind.ReadOnlyKeyword);
AddSpace();
AddRefReadonly();
}
}

private void AddRefReadonly()
{
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
AddKeyword(SyntaxKind.ReadOnlyKeyword);
AddSpace();
}

private void AddReadOnlyIfRequired()
{
// 'readonly' in this context is effectively a 'ref' modifier
Expand All @@ -967,21 +988,26 @@ private void AddParameterRefKindIfRequired(RefKind refKind)
{
if (format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeParamsRefOut))
{
switch (refKind)
{
case RefKind.Out:
AddKeyword(SyntaxKind.OutKeyword);
AddSpace();
break;
case RefKind.Ref:
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
break;
case RefKind.In:
AddKeyword(SyntaxKind.InKeyword);
AddSpace();
break;
}
AddParameterRefKind(refKind);
}
}

private void AddParameterRefKind(RefKind refKind)
{
switch (refKind)
{
case RefKind.Out:
AddKeyword(SyntaxKind.OutKeyword);
AddSpace();
break;
case RefKind.Ref:
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
break;
case RefKind.In:
AddKeyword(SyntaxKind.InKeyword);
AddSpace();
break;
}
}
}
Expand Down
54 changes: 27 additions & 27 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,15 +255,15 @@ void M(C c)
var comp = CreateCompilationWithFunctionPointersAndIl(source, il);

comp.VerifyDiagnostics(
// (6,26): error CS0570: 'delegate*<int>' is not supported by the language
// (6,26): error CS0570: 'delegate*<ref int>' is not supported by the language
// ref int i1 = ref c.Field1();
Diagnostic(ErrorCode.ERR_BindToBogus, "c.Field1()").WithArguments("delegate*<int>").WithLocation(6, 26),
Diagnostic(ErrorCode.ERR_BindToBogus, "c.Field1()").WithArguments("delegate*<ref int>").WithLocation(6, 26),
// (6,28): error CS0570: 'C.Field1' is not supported by the language
// ref int i1 = ref c.Field1();
Diagnostic(ErrorCode.ERR_BindToBogus, "Field1").WithArguments("C.Field1").WithLocation(6, 28),
// (7,26): error CS0570: 'delegate*<int>' is not supported by the language
// (7,26): error CS0570: 'delegate*<ref int>' is not supported by the language
// ref int i2 = ref c.Field2();
Diagnostic(ErrorCode.ERR_BindToBogus, "c.Field2()").WithArguments("delegate*<int>").WithLocation(7, 26),
Diagnostic(ErrorCode.ERR_BindToBogus, "c.Field2()").WithArguments("delegate*<ref int>").WithLocation(7, 26),
// (7,28): error CS0570: 'C.Field2' is not supported by the language
// ref int i2 = ref c.Field2();
Diagnostic(ErrorCode.ERR_BindToBogus, "Field2").WithArguments("C.Field2").WithLocation(7, 28),
Expand Down Expand Up @@ -3223,18 +3223,18 @@ void M()
// (26,36): error CS8758: Ref mismatch between 'C.M6()' and function pointer 'delegate*<object>'
// delegate*<object> ptr14 = &M6;
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M6").WithArguments("C.M6()", "delegate*<object>").WithLocation(26, 36),
// (27,40): error CS8758: Ref mismatch between 'C.M6()' and function pointer 'delegate*<object>'
// (27,40): error CS8758: Ref mismatch between 'C.M6()' and function pointer 'delegate*<ref object>'
// delegate*<ref object> ptr15 = &M6;
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M6").WithArguments("C.M6()", "delegate*<object>").WithLocation(27, 40),
// (28,40): error CS8758: Ref mismatch between 'C.M7()' and function pointer 'delegate*<object>'
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M6").WithArguments("C.M6()", "delegate*<ref object>").WithLocation(27, 40),
// (28,40): error CS8758: Ref mismatch between 'C.M7()' and function pointer 'delegate*<ref object>'
// delegate*<ref object> ptr16 = &M7;
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M7").WithArguments("C.M7()", "delegate*<object>").WithLocation(28, 40),
// (29,49): error CS8758: Ref mismatch between 'C.M5()' and function pointer 'delegate*<object>'
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M7").WithArguments("C.M7()", "delegate*<ref object>").WithLocation(28, 40),
// (29,49): error CS8758: Ref mismatch between 'C.M5()' and function pointer 'delegate*<ref readonly object>'
// delegate*<ref readonly object> ptr17 = &M5;
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M5").WithArguments("C.M5()", "delegate*<object>").WithLocation(29, 49),
// (30,49): error CS8758: Ref mismatch between 'C.M7()' and function pointer 'delegate*<object>'
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M5").WithArguments("C.M5()", "delegate*<ref readonly object>").WithLocation(29, 49),
// (30,49): error CS8758: Ref mismatch between 'C.M7()' and function pointer 'delegate*<ref readonly object>'
// delegate*<ref readonly object> ptr18 = &M7;
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M7").WithArguments("C.M7()", "delegate*<object>").WithLocation(30, 49)
Diagnostic(ErrorCode.ERR_FuncPtrRefMismatch, "M7").WithArguments("C.M7()", "delegate*<ref readonly object>").WithLocation(30, 49)
);
}

Expand Down Expand Up @@ -5314,7 +5314,7 @@ void M<T>(delegate*<ref T> ptr1, delegate*<T> ptr2) where T : C
}
}");

verifier.VerifyIL(@"C.M<T>(delegate*<T>, delegate*<T>)", @"
verifier.VerifyIL(@"C.M<T>(delegate*<ref T>, delegate*<T>)", @"
{
// Code size 34 (0x22)
.maxstack 1
Expand Down Expand Up @@ -6321,10 +6321,10 @@ protected override void M1(delegate*<{refKind2}string> ptr) {{}}
comp.VerifyDiagnostics(
// (9,29): error CS0115: 'Derived.M1(delegate*<{refKind2} string>)': no suitable method found to override
// protected override void M1(delegate*<{refKind2} string> ptr) {}
Diagnostic(ErrorCode.ERR_OverrideNotExpected, "M1").WithArguments($"Derived.M1(delegate*<string>)").WithLocation(9, 29),
Diagnostic(ErrorCode.ERR_OverrideNotExpected, "M1").WithArguments($"Derived.M1(delegate*<{(string.IsNullOrWhiteSpace(refKind2) ? "" : refKind2)}string>)").WithLocation(9, 29),
// (10,49): error CS0508: 'Derived.M2()': return type must be 'delegate*<{refKind1} string>' to match overridden member 'Base.M2()'
// protected override delegate*<{refKind2} string> M2() => throw null;
Diagnostic(ErrorCode.ERR_CantChangeReturnTypeOnOverride, "M2").WithArguments("Derived.M2()", "Base.M2()", $"delegate*<string>").WithLocation(10, 42 + refKind2.Length)
Diagnostic(ErrorCode.ERR_CantChangeReturnTypeOnOverride, "M2").WithArguments("Derived.M2()", "Base.M2()", $"delegate*<{(string.IsNullOrWhiteSpace(refKind1) ? "" : refKind1)}string>").WithLocation(10, 42 + refKind2.Length)
);
}

Expand Down Expand Up @@ -7078,12 +7078,12 @@ static void M2(delegate*<ref string, ref string> ptr1)
// (18,32): warning CS8619: Nullability of reference types in value of type 'string' doesn't match target type 'string?'.
// ref string? str3 = ref ptr1(ref str2);
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr1(ref str2)").WithArguments("string", "string?").WithLocation(18, 32),
// (19,51): warning CS8619: Nullability of reference types in value of type 'delegate*<ref string, string>' doesn't match target type 'delegate*<ref string?, string>'.
// (19,51): warning CS8619: Nullability of reference types in value of type 'delegate*<ref string, ref string>' doesn't match target type 'delegate*<ref string?, ref string>'.
// delegate*<ref string?, ref string> ptr2 = ptr1;
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr1").WithArguments("delegate*<ref string, string>", "delegate*<ref string?, string>").WithLocation(19, 51),
// (20,51): warning CS8619: Nullability of reference types in value of type 'delegate*<ref string, string>' doesn't match target type 'delegate*<ref string, string?>'.
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr1").WithArguments("delegate*<ref string, ref string>", "delegate*<ref string?, ref string>").WithLocation(19, 51),
// (20,51): warning CS8619: Nullability of reference types in value of type 'delegate*<ref string, ref string>' doesn't match target type 'delegate*<ref string, ref string?>'.
// delegate*<ref string, ref string?> ptr3 = ptr1;
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr1").WithArguments("delegate*<ref string, string>", "delegate*<ref string, string?>").WithLocation(20, 51)
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "ptr1").WithArguments("delegate*<ref string, ref string>", "delegate*<ref string, ref string?>").WithLocation(20, 51)
);
}

Expand Down Expand Up @@ -10775,7 +10775,7 @@ static ref int ReturnPtrByRef(delegate*<ref int, ref int> ptr, ref int i)
static ref int ReturnByRef(ref int i) => ref i;
}", expectedOutput: "2");

verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, int>, ref int)", @"
verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, ref int>, ref int)", @"
{
// Code size 10 (0xa)
.maxstack 2
Expand Down Expand Up @@ -10831,9 +10831,9 @@ static ref Span<int> ReturnPtrByRef(delegate*<ref Span<int>, ref Span<int>> ptr)
}", options: TestOptions.UnsafeReleaseExe);

comp.VerifyDiagnostics(
// (10,20): error CS8347: Cannot use a result of 'delegate*<ref Span<int>, Span<int>>' in this context because it may expose variables referenced by parameter '0' outside of their declaration scope
// (10,20): error CS8347: Cannot use a result of 'delegate*<ref Span<int>, ref Span<int>>' in this context because it may expose variables referenced by parameter '0' outside of their declaration scope
// return ref ptr(ref span);
Diagnostic(ErrorCode.ERR_EscapeCall, "ptr(ref span)").WithArguments("delegate*<ref System.Span<int>, System.Span<int>>", "0").WithLocation(10, 20),
Diagnostic(ErrorCode.ERR_EscapeCall, "ptr(ref span)").WithArguments("delegate*<ref System.Span<int>, ref System.Span<int>>", "0").WithLocation(10, 20),
// (10,28): error CS8168: Cannot return local 'span' by reference because it is not a ref local
// return ref ptr(ref span);
Diagnostic(ErrorCode.ERR_RefReturnLocal, "span").WithArguments("span").WithLocation(10, 28)
Expand Down Expand Up @@ -10861,7 +10861,7 @@ static ref Span<int> ReturnPtrByRef(delegate*<ref Span<int>, ref Span<int>> ptr,

var verifier = CompileAndVerify(comp, expectedOutput: "2", verify: Verification.Skipped);

verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref System.Span<int>, System.Span<int>>, ref System.Span<int>)", @"
verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref System.Span<int>, ref System.Span<int>>, ref System.Span<int>)", @"
{
// Code size 10 (0xa)
.maxstack 2
Expand Down Expand Up @@ -10892,9 +10892,9 @@ static ref int ReturnPtrByRef(delegate*<ref int, ref readonly int> ptr, ref int
}", options: TestOptions.UnsafeReleaseExe);

comp.VerifyDiagnostics(
// (8,16): error CS8333: Cannot return method 'delegate*<ref int, int>' by writable reference because it is a readonly variable
// (8,16): error CS8333: Cannot return method 'delegate*<ref int, ref readonly int>' by writable reference because it is a readonly variable
// => ref ptr(ref i);
Diagnostic(ErrorCode.ERR_RefReturnReadonlyNotField, "ptr(ref i)").WithArguments("method", "delegate*<ref int, int>").WithLocation(8, 16)
Diagnostic(ErrorCode.ERR_RefReturnReadonlyNotField, "ptr(ref i)").WithArguments("method", "delegate*<ref int, ref readonly int>").WithLocation(8, 16)
);
}

Expand All @@ -10915,7 +10915,7 @@ static ref readonly int ReturnPtrByRef(delegate*<ref int, ref int> ptr, ref int
static ref int ReturnByRef(ref int i) => ref i;
}", expectedOutput: "2");

verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, int>, ref int)", @"
verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, ref int>, ref int)", @"
{
// Code size 10 (0xa)
.maxstack 2
Expand Down Expand Up @@ -11023,7 +11023,7 @@ static ref int ReturnPtrByRef(delegate*<ref int, ref int> ptr, ref int i, ref in
static ref int ReturnByRef(ref int i) => ref i;
}", expectedOutput: "2");

verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, int>, ref int, ref int)", @"
verifier.VerifyIL("<Program>$.<<Main>$>g__ReturnPtrByRef|0_0(delegate*<ref int, ref int>, ref int, ref int)", @"
{
// Code size 10 (0xa)
.maxstack 2
Expand Down
Loading