Fix Function Pointer RefKind Display#51223
Conversation
|
|
||
| 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.
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.
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.
|
|
||
| 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.
SignatureOnlyParameterSymbol [](start = 136, length = 28)
Could you please elaborate how SignatureOnlyParameterSymbol comes to life here? #Closed
There was a problem hiding this comment.
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.
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.
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.
|| 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.
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.
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.
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.
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.
this function
To clarify, I am talking about VisitParameter
In reply to: 591633037 [](ancestors = 591633037,591629245)
There was a problem hiding this comment.
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.
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.
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. |
| // (8,14): warning CS8604: Possible null reference argument for parameter 'T' in 'delegate*<T, void>'. | ||
| // 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.
"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.
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.
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
| foreach (var param in symbol.Parameters) | ||
| { | ||
| param.Accept(this.NotFirstVisitor); | ||
| AddParameterRefKindIfRequired(param.RefKind); |
There was a problem hiding this comment.
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.
Always show ref kinds?
There was a problem hiding this comment.
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.
Return type checks for SymbolDisplayMemberOptions.IncludeRef. What I'm currently doing is consistent with that.
There was a problem hiding this comment.
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.
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) |
| // used on their own or in the context of methods. | ||
|
|
||
|
|
||
| var includeType = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeType); |
There was a problem hiding this comment.
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. |
| foreach (var param in symbol.Parameters) | ||
| { | ||
| param.Accept(this.NotFirstVisitor); | ||
| if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeRef)) |
There was a problem hiding this comment.
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.
I'm ok with that
There was a problem hiding this comment.
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.
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.
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.
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.
&& (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.
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.
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.
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.
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 includeName = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeName) && | ||
| symbol.Name.Length != 0; | ||
| var includeBrackets = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeOptionalBrackets); | ||
| var includeOption = format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeDefaultValue) && |
There was a problem hiding this comment.
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.
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) |
…tion-pointer-display
|
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) |
|
Thanks @YairHalberstadt! |
Fixes #51222
Fixes #51224