-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Fix Function Pointer RefKind Display #51223
Conversation
@@ -678,9 +678,13 @@ public override void VisitParameter(IParameterSymbol symbol) | |||
// (e.g. field types, param types, etc), which just want the name whereas parameters are | |||
// used on their own or in the context of methods. | |||
|
|||
var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType); | |||
// SignatureOnlyParameterSymbol.ContainingSymbol throws. | |||
var isFunctionPointerParameter = symbol is not Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symbol is not Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol }
This is horrible - would it be possible to make SignatureOnlyParameterSymbol.ContainingSymbol
return null instead of throwing @333fred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not easily. It would require a significant refactoring of construction. I misread this as returning an actual thing, not null
. It would be possible, but whether it's the right thing to do, I'd need to think on a bit. It certainly seems like SignatureOnlyParameterSymbol
was not intended to be publicly exposed, as ContainingSymbol
throws, so the fact that we have them at all feels wrong to me.
@@ -678,9 +678,13 @@ public override void VisitParameter(IParameterSymbol symbol) | |||
// (e.g. field types, param types, etc), which just want the name whereas parameters are | |||
// used on their own or in the context of methods. | |||
|
|||
var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType); | |||
// SignatureOnlyParameterSymbol.ContainingSymbol throws. | |||
var isFunctionPointerParameter = symbol is not Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignatureOnlyParameterSymbol [](start = 136, length = 28)
Could you please elaborate how SignatureOnlyParameterSymbol comes to life here? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was the part I was trying to dig into just now. I'm not sure how this is ever true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a number of test failures till I fixed this. You can see them at https://dev.azure.com/dnceng/public/_build/results?buildId=994331&view=ms.vss-test-web.build-test-results-tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this has nothing to do with function pointer symbols, it's other scenarios that involve SignatureOnlyParameterSymbols that is being guarded against.
var isFunctionPointerParameter = symbol is not Microsoft.CodeAnalysis.CSharp.Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol } | ||
&& symbol.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.FunctionPointerSignature }; | ||
var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType) | ||
|| isFunctionPointerParameter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| isFunctionPointerParameter [](start = 16, length = 29)
I am not sure whether adding this special case here is the right thing to do. Perhaps once we visit a function pointer type we can simply adjust the format? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this approach will work well, as it seems SymbolDisplayVisitor.VisitParameter can be called on a function pointer parameter in other contexts, and we'll need to at the very least fix the spacing in such a case.
For example it's called here.
Perhaps the neatest solution to this is to expose on IParameter
that it is a function pointer parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this approach will work well, as it seems SymbolDisplayVisitor.VisitParameter can be called on a function pointer parameter in other contexts, and we'll need to at the very least fix the spacing in such a case.
It feels like we don't need to make any changes in VisitParameter
since we are handling everything in VisitMethod
. I do not feel any sympathy to callers that display parameters on their own.
In reply to: 579027094 [](ancestors = 579027094)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like we don't need to make any changes in VisitParameter since we are handling everything in VisitMethod. I do not feel any sympathy to callers that display parameters on their own.
I've been trying to find a workaround to that case. Other than reimplementing all the logic to visit a FunctionParameter at the call site (which seems wrong), or exposing a new public method SymbolDisplay.FunctionParameterToDisplayParts
I can't see anything better than handling a FunctionPointerParameterSymbol correctly in VisitParameter
.
I'll go with adding SymbolDisplay.FunctionParameterToDisplayParts
for now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go with adding SymbolDisplay.FunctionParameterToDisplayParts for now...
I'm not happy with that either. I think the correct solution is to have an IParameterSymbol publicly distinguishable from a FunctionParameterSymbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function
To clarify, I am talking about VisitParameter
In reply to: 591633037 [](ancestors = 591633037,591629245)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is broken because this method no longer special cases Function pointer parameter symbols, and so it leaves an extra space after the type, where it expects a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is broken because this method no longer special cases Function pointer parameter symbols, and so it leaves an extra space after the type, where it expects a name.
Isn't this the outcome of your change for the includeName
calculation?
In reply to: 591634752 [](ancestors = 591634752)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Please revert the change. I suggested this several times already.
In reply to: 591637214 [](ancestors = 591637214)
Based on the behavior described in #51222, it looks like the return type isn't "swallowed". Do we understand why this is not happening? Is there a way to get it "swallowed" by using some other format options? #Closed Refers to: src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs:673 in ad7ddea. [](commit_id = ad7ddea, deletion_comment = False) |
I'm not sure what you mean by this? The issue isn't with the return type, it's that the parameters only display the name, and since there isn't any name they display nothing at all. |
// ptr1(t); | ||
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t").WithArguments("", "delegate*<T, void>").WithLocation(8, 14), | ||
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t").WithArguments("T", "delegate*<T, void>").WithLocation(8, 14), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"T" [](start = 83, length = 3)
I am not sure if it is the right thing to do to unconditionally override the format here. I understand that the behavior is somewhat better now, but it doesn't feel appropriate to refer to a function pointer parameter in this fashion. For the purpose of this diagnostics, it should be identified by position rather than type. For regular method it is identified by name, I assume. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a regular method it is identified by name.
So perhaps the message for a function pointer should be:
warning CS8604: Possible null reference argument for parameter '0' in 'delegate*<T, void>'.
?
Is it ok to pass in an int
as an argument to a diagnostic which usually receives an ITypeSymbol, or do we try and keep it consistent?
Or should we create an entirely new diagnostic:
warning CS9999: Possible null reference argument for parameter at position '0' in 'delegate*<T, void>'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we create an entirely new diagnostic:
A new diagnostics would be ideal, I think. However, I believe this is an orthogonal issue and we probably shouldn't try to address it in this PR.
In reply to: 577056784 [](ancestors = 577056784)
That is understood. The question is: "Is there a way to get return type "swallowed" by using some other format options?" Sorry for the typos in the original comment. In reply to: 780034183 [](ancestors = 780034183) |
At a guess no because we explicitly handle the ReturnType in I've decided to do the same for parameters too. Since they're very different from normal method parameters, I think best to handle them explicitly. |
I should really add some tests for function pointers with modreq/modopts but the process to test those looks complex. If anyone wants to walk me through it happy to do so. |
…Parameter as they should be handled seperately
@@ -609,7 +609,14 @@ void visitFunctionPointerSignature(IMethodSymbol symbol) | |||
|
|||
foreach (var param in symbol.Parameters) | |||
{ | |||
param.Accept(this.NotFirstVisitor); | |||
AddParameterRefKindIfRequired(param.RefKind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddParameterRefKindIfRequired [](start = 20, length = 29)
This still depends on some parameter display options, it feels like we don't want to depend on any of them. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always show ref kinds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always show ref kinds?
Probably use the same option that is used for return type.
In reply to: 578556365 [](ancestors = 578556365)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type checks for SymbolDisplayMemberOptions.IncludeRef. What I'm currently doing is consistent with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you want to use SymbolDisplayMemberOptions.IncludeRef for both parameters and return types? I'm not sure how much sense that makes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you want to use SymbolDisplayMemberOptions.IncludeRef for both parameters and return types? I'm not sure how much sense that makes.
What are you proposing? I think that the goal for this PR is to remove dependency on parameter display options when we display a function pointer type. Usage of AddParameterRefKindIfRequired
simply doesn't get us there. Aligning behavior with the return type handling at least doesn't make things worse than they already are (around the return type).
In reply to: 579010475 [](ancestors = 579010475)
@333fred, I think we should consider moving the code to display a function pointer type symbol to VisitFunctionPointerType. And let this method to display underlying method symbol as a regular method. #Closed Refers to: src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs:279 in ef0d6cc. [](commit_id = ef0d6cc, deletion_comment = False) |
@@ -678,9 +685,11 @@ public override void VisitParameter(IParameterSymbol symbol) | |||
// (e.g. field types, param types, etc), which just want the name whereas parameters are | |||
// used on their own or in the context of methods. | |||
|
|||
|
|||
var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType); |
There was a problem hiding this comment.
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
It looks like there are some legitimate test failures. #Closed |
Done with review pass (commit 5) #Closed |
The reason we don't do this is because a number of places in the compiler don't have a function pointer type symbol, they have the method symbol (such as overload resolution), which they use for error messages. I'll add a comment to the source that explains this for future reference. In reply to: 781467907 [](ancestors = 781467907) Refers to: src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs:279 in ef0d6cc. [](commit_id = ef0d6cc, deletion_comment = False) |
Moving this to draft since it looks like there are still some comments from Aleksey that need to be addressed. @YairHalberstadt, let us know if you need guidance on anything and we can chat, or let us know when this is ready for another look. |
@@ -609,7 +609,17 @@ void visitFunctionPointerSignature(IMethodSymbol symbol) | |||
|
|||
foreach (var param in symbol.Parameters) | |||
{ | |||
param.Accept(this.NotFirstVisitor); | |||
if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)?
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
var includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName) | ||
&& !(symbol.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.FunctionPointerSignature }); | ||
&& (symbol is Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol } // SignatureOnlyParameterSymbol.ContainingSymbol throws NotSupportedException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& (symbol is Symbols.PublicModel.Symbol { UnderlyingSymbol: SignatureOnlyParameterSymbol } // SignatureOnlyParameterSymbol.ContainingSymbol throws NotSupportedException [](start = 16, length = 169)
I guess the motivation for this change is not clear to me. I was suggesting to revert to the original behavior:
var includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName)
&& !(symbol.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.FunctionPointerSignature });
unless there is a specific reason. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the original code can throw, as I discovered when I moved that expression to a local variable that ran unconditionally. We just happen not to have any tests where we use IncludeName
with a SignatureOnlyParameterSymbol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the original code can throw, as I discovered when I moved that expression to a local variable that ran unconditionally. We just happen not to have any tests where we use IncludeName with a SignatureOnlyParameterSymbol
We need to back this change with a targeted unit-test then. The test should break with the original code here. Also, we should consider adjusting the check by looking at the root cause of the problem that the !(symbol.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.FunctionPointerSignature })
condition was added to address. I mean the fact that display was "bad" when the parameter had an empty name. I think we should simply check for an empty name then, rather than trying to detect that indirectly. I am thinking of something like this:
var includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName)
&& !symbol.Name.IsNullOrEmpty();
In reply to: 592339443 [](ancestors = 592339443)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That ends up triggering a huge number of changes, since in error cases things often don't have names.
It breaks a lot of tests, but also means that we don't print the default value of an optional parameter when the parameter doesn't have a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That ends up triggering a huge number of changes, since in error cases things often don't have names.
It breaks a lot of tests, but also means that we don't print the default value of an optional parameter when the parameter doesn't have a name.
Could you give some examples of these behavior changes? Used to be this, now this. Also some examples of what are those parameters without names, where are they coming from.
In reply to: 592362609 [](ancestors = 592362609)
Done with review pass (commit 6) #Closed |
Perhaps we could mitigate the issue with dropping default values by taking this Refers to: src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs:729 in d31a3dc. [](commit_id = d31a3dc, deletion_comment = False) |
var includeBrackets = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeOptionalBrackets); | ||
var includeOption = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) && |
There was a problem hiding this comment.
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
Done with review pass (commit 7) #Closed |
var includeDefaultValue = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) && | ||
format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName) && | ||
symbol.HasExplicitDefaultValue && | ||
CanAddConstant(symbol.Type, symbol.ExplicitDefaultValue); |
There was a problem hiding this comment.
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?
Seems like some of the newly added tests need to have their baselines updated. |
Done review pass (commit 9) |
Done with review pass (commit 9). Other than the CI failures, LGTM. #Closed |
It looks like this is likely to defeat some purpose of the test. #Closed Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/FunctionPointerTypeSymbolTests.cs:250 in e3f11f5. [](commit_id = e3f11f5, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 12)
Thanks @YairHalberstadt! |
Fixes #51222
Fixes #51224