Add SymbolDisplayMemberOptions.IncludeRef option to include ref keyword for ref-returning members
#14654
Conversation
|
@dotnet/roslyn-compiler, @dotnet/roslyn-ide please review Note this is a public API change, and a breaking change since |
| @@ -61,7 +61,8 @@ public override void VisitProperty(IPropertySymbol symbol) | |||
| if (format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeType)) | |||
| { | |||
| var property = symbol as PropertySymbol; | |||
| if (property != null) | |||
There was a problem hiding this comment.
Why do we have this cast to PropertySymbol. That goes against a goal of SymbolDisplay as it means we can't build proper displays when passing in a VB symbol or an IDE synthesized symbol. If it's because RefKind isn't on IPropertySymbol, then that needs ot change as the IDE (and other consumers) def needs to know about that and won't have access to the internal PropertySymbol.
There was a problem hiding this comment.
Looking at the API, it looks like you should be using "ReturnsByRef"
There was a problem hiding this comment.
I see two "as" casts in the code that should be fixed up. One to PropertySymbol and one to MethodSymbol. Can you take care of both? Thanks!
There was a problem hiding this comment.
Good catch, thanks. Changed to use ReturnsByRef.
| @@ -720,19 +725,16 @@ private void AddCustomModifiersIfRequired(ImmutableArray<CustomModifier> customM | |||
|
|
|||
| private void AddRefKindIfRequired(RefKind refKind) | |||
There was a problem hiding this comment.
AddRefKindIfRequired [](start = 21, length = 20)
Consider renaming to AddRefKindIfNotNone or something similar. #Closed
There was a problem hiding this comment.
Renamed to AddRefKindIfNotNone.
| { | ||
| AddKeyword(SyntaxKind.ParamsKeyword); | ||
| AddSpace(); | ||
| AddRefKindIfRequired(symbol.RefKind); |
There was a problem hiding this comment.
AddRefKindIfRequired(symbol.RefKind); [](start = 20, length = 37)
Is the change around this consistent with the current VB behavior? Is there a test for scenario when IncludeParamsRefOut is false for VB?
There was a problem hiding this comment.
The behavior is not consistent with VB. In C#, ref, out, params is only included if the type is also included, whereas in VB, ByRef and ParamArray are included independent of the type. (I've updated the TestParameterMethodNameTypeModifiers tests to track that behavior.)
We can change C# to handle IncludeParamsRefOut independent of IncludeType but that will be a breaking change.
|
LGTM |
|
👎 right now. |
|
My issues were addressed. Thanks! |
|
LGTM |
|
It looks like a bunch of tests are affected by the change and need a baseline adjustment. |
…word for ref-returning members
|
@dotnet/roslyn-compiler, please provide a second review, thanks. |
Fixes #11356.