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

Jump to bottom

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 7 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
79 changes: 49 additions & 30 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,7 +609,17 @@ void visitFunctionPointerSignature(IMethodSymbol symbol)

foreach (var param in symbol.Parameters)
{
param.Accept(this.NotFirstVisitor);
if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef))
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef)) [](start = 20, length = 79)

It looks like this breaks some tests because diagnostics now doesn't display ref-ness of parameter types by default. Perhaps we should display ref-ness in function pointer types unconditionally (including return ref-ness)? #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'm ok with that

Copy link
Contributor

Choose a reason for hiding this comment

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

@333fred

Perhaps we should display ref-ness in function pointer types unconditionally (including return ref-ness)?

What do you think? Another option is to add a whole new display option (SymbolDisplayMiscellaneousOptions?) that controls that for function pointer types.

In reply to: 592331577 [](ancestors = 592331577)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like all the remaining broken tests are due to this issue. I'm going to wait to fix them till a decision is made.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should display ref-ness in function pointer types unconditionally (including return ref-ness)

I think we should do this. You can overload methods by function-pointer-parameter-refness, so if you had M(delegate*<ref string, void>) and M(delegate*<string, void>), that would lead to bad diagnostics.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all the remaining broken tests are due to this issue. I'm going to wait to fix them till a decision is made.

It looks like we are converging on displaying ref-ness in function pointer types unconditionally (including the return ref-ness)

In reply to: 592505606 [](ancestors = 592505606)

{
AddParameterRefKind(param.RefKind);
}

AddCustomModifiersIfRequired(param.RefCustomModifiers);

param.Type.Accept(this.NotFirstVisitor);

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

AddPunctuation(SyntaxKind.CommaToken);
AddSpace();
}
Expand Down Expand Up @@ -679,9 +689,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 includeOption = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) &&
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

includeOption [](start = 16, length = 13)

The name "includeOption" feels somewhat confusing. "includeDefaultValue" ? #Closed

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 +717,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 (includeOption)
{
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 @@ -967,21 +981,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
14 changes: 7 additions & 7 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,11 +415,11 @@ public record A(int i,) { }
AssertEx.Equal(expectedMembers,
comp.GetMember<NamedTypeSymbol>("A").GetMembers().OfType<PropertySymbol>().ToTestDisplayStrings());

AssertEx.Equal(new[] { "A..ctor(System.Int32 i, ? )", "A..ctor(A original)" },
AssertEx.Equal(new[] { "A..ctor(System.Int32 i, ?)", "A..ctor(A original)" },
comp.GetMember<NamedTypeSymbol>("A").Constructors.ToTestDisplayStrings());

var primaryCtor = comp.GetMember<NamedTypeSymbol>("A").Constructors.First();
Assert.Equal("A..ctor(System.Int32 i, ? )", primaryCtor.ToTestDisplayString());
Assert.Equal("A..ctor(System.Int32 i, ?)", primaryCtor.ToTestDisplayString());
Assert.IsType<ParameterSyntax>(primaryCtor.Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
}

Expand Down Expand Up @@ -451,7 +451,7 @@ public record A(int i, // A
);

var primaryCtor = comp.GetMember<NamedTypeSymbol>("A").Constructors.First();
Assert.Equal("A..ctor(System.Int32 i, ? , ? )", primaryCtor.ToTestDisplayString());
Assert.Equal("A..ctor(System.Int32 i, ?, ?)", primaryCtor.ToTestDisplayString());
Assert.IsType<ParameterSyntax>(primaryCtor.Parameters[0].DeclaringSyntaxReferences.Single().GetSyntax());
Assert.IsType<ParameterSyntax>(primaryCtor.Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
Assert.IsType<ParameterSyntax>(primaryCtor.Parameters[2].DeclaringSyntaxReferences.Single().GetSyntax());
Expand All @@ -477,7 +477,7 @@ public class C
);

var ctor = comp.GetMember<NamedTypeSymbol>("C").Constructors.Single();
Assert.Equal("C..ctor(System.Int32 i, ? )", ctor.ToTestDisplayString());
Assert.Equal("C..ctor(System.Int32 i, ?)", ctor.ToTestDisplayString());
Assert.IsType<ParameterSyntax>(ctor.Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
Assert.Equal(0, ctor.Parameters[1].Locations.Single().SourceSpan.Length);
}
Expand Down Expand Up @@ -505,7 +505,7 @@ public record A(int i, int ) { }
comp.GetMember<NamedTypeSymbol>("A").GetMembers().OfType<PropertySymbol>().ToTestDisplayStrings());

var ctor = comp.GetMember<NamedTypeSymbol>("A").Constructors[0];
Assert.Equal("A..ctor(System.Int32 i, System.Int32 )", ctor.ToTestDisplayString());
Assert.Equal("A..ctor(System.Int32 i, System.Int32)", ctor.ToTestDisplayString());
Assert.IsType<ParameterSyntax>(ctor.Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
Assert.Equal(0, ctor.Parameters[1].Locations.Single().SourceSpan.Length);
}
Expand Down Expand Up @@ -538,7 +538,7 @@ public record A(int, string ) { }
AssertEx.Equal(expectedMembers,
comp.GetMember<NamedTypeSymbol>("A").GetMembers().OfType<PropertySymbol>().ToTestDisplayStrings());

AssertEx.Equal(new[] { "A..ctor(System.Int32 , System.String )", "A..ctor(A original)" },
AssertEx.Equal(new[] { "A..ctor(System.Int32, System.String)", "A..ctor(A original)" },
comp.GetMember<NamedTypeSymbol>("A").Constructors.ToTestDisplayStrings());

Assert.IsType<ParameterSyntax>(comp.GetMember<NamedTypeSymbol>("A").Constructors[0].Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
Expand Down Expand Up @@ -572,7 +572,7 @@ public record A(int, int ) { }
AssertEx.Equal(expectedMembers,
comp.GetMember<NamedTypeSymbol>("A").GetMembers().OfType<PropertySymbol>().ToTestDisplayStrings());

AssertEx.Equal(new[] { "A..ctor(System.Int32 , System.Int32 )", "A..ctor(A original)" },
AssertEx.Equal(new[] { "A..ctor(System.Int32, System.Int32)", "A..ctor(A original)" },
comp.GetMember<NamedTypeSymbol>("A").Constructors.ToTestDisplayStrings());

Assert.IsType<ParameterSyntax>(comp.GetMember<NamedTypeSymbol>("A").Constructors[0].Parameters[1].DeclaringSyntaxReferences.Single().GetSyntax());
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5223,7 +5223,7 @@ static void F(int x = 2, = 3) { }
var model = comp.GetSemanticModel(tree);
var decls = tree.GetCompilationUnitRoot().DescendantNodes().OfType<ParameterSyntax>().ToArray();
var symbol1 = VerifyParameter(model, decls[0], 0, "[System.Int32 x = 2]", "System.Int32", 2);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
Assert.Same(symbol1.ContainingSymbol, symbol2.ContainingSymbol);
}

Expand All @@ -5244,7 +5244,7 @@ void F(int x, = 3) { }
var model = comp.GetSemanticModel(tree);
var decls = tree.GetCompilationUnitRoot().DescendantNodes().OfType<ParameterSyntax>().ToArray();
var symbol1 = VerifyParameter(model, decls[0], 0, "System.Int32 x", null, null);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
Assert.Same(symbol1.ContainingSymbol, symbol2.ContainingSymbol);
}

Expand All @@ -5265,7 +5265,7 @@ void F(int x = 2, = 3) { }
var model = comp.GetSemanticModel(tree);
var decls = tree.GetCompilationUnitRoot().DescendantNodes().OfType<ParameterSyntax>().ToArray();
var symbol1 = VerifyParameter(model, decls[0], 0, "[System.Int32 x = 2]", "System.Int32", 2);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
var symbol2 = VerifyParameter(model, decls[1], 1, "[? = null]", "System.Int32", 3);
Assert.Same(symbol1.ContainingSymbol, symbol2.ContainingSymbol);
}

Expand Down
105 changes: 105 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7652,5 +7652,110 @@ record Person(string First, string Last);
SymbolDisplayPartKind.Space,
SymbolDisplayPartKind.RecordClassName);
}

[Fact, WorkItem(51222, "https://github.com/dotnet/roslyn/issues/51222")]
public void TestFunctionPointerWithoutIncludeTypesInParameterOptions()
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
var text = @"
class A {
delegate*<int, string> f;
}";

Func<NamespaceSymbol, Symbol> findSymbol = global =>
((FieldSymbol)global.GetTypeMembers("A", 0).Single()
.GetMembers("f").Single()).Type;

var format = new SymbolDisplayFormat();

TestSymbolDescription(
text,
findSymbol,
format,
"delegate*<Int32, String>");
}

[Fact, WorkItem(51222, "https://github.com/dotnet/roslyn/issues/51222")]
public void TestFunctionPointerWithTupleParameter()
{
var text = @"
class A {
delegate*<(int, string), void> f;
}";

Func<NamespaceSymbol, Symbol> findSymbol = global =>
((FieldSymbol)global.GetTypeMembers("A", 0).Single()
.GetMembers("f").Single()).Type;

var format = new SymbolDisplayFormat();

TestSymbolDescription(
text,
findSymbol,
format,
"delegate*<(Int32, String), Void>");
}

[Fact, WorkItem(51222, "https://github.com/dotnet/roslyn/issues/51222")]
public void TestFunctionPointerWithTupleParameterWithNames()
{
var text = @"
class A {
delegate*<(int i, string s), (int i, string s)> f;
}";

Func<NamespaceSymbol, Symbol> findSymbol = global =>
((FieldSymbol)global.GetTypeMembers("A", 0).Single()
.GetMembers("f").Single()).Type;

var format = new SymbolDisplayFormat();

TestSymbolDescription(
text,
findSymbol,
format,
"delegate*<(Int32 i, String s), (Int32 i, String s)>");
}

[Fact, WorkItem(51222, "https://github.com/dotnet/roslyn/issues/51222")]
public void TestFunctionPointerWithRefParametersAndIncludeParameterRefOut()
{
var text = @"
class A {
delegate*<in int, ref readonly string> f;
}";

Func<NamespaceSymbol, Symbol> findSymbol = global =>
((FieldSymbol)global.GetTypeMembers("A", 0).Single()
.GetMembers("f").Single()).Type;

var format = new SymbolDisplayFormat(parameterOptions: SymbolDisplayParameterOptions.IncludeParamsRefOut);

TestSymbolDescription(
text,
findSymbol,
format,
"delegate*<Int32, String>");
}

[Fact, WorkItem(51222, "https://github.com/dotnet/roslyn/issues/51222")]
public void TestFunctionPointerWithRefParametersAndIncludeRef()
{
var text = @"
class A {
delegate*<in int, ref readonly string> f;
}";

Func<NamespaceSymbol, Symbol> findSymbol = global =>
((FieldSymbol)global.GetTypeMembers("A", 0).Single()
.GetMembers("f").Single()).Type;

var format = new SymbolDisplayFormat(memberOptions: SymbolDisplayMemberOptions.IncludeRef);

TestSymbolDescription(
text,
findSymbol,
format,
"delegate*<in Int32, ref readonly String>");
}
}
}