-
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
VB support for CallerArgumentExpression #54132
VB support for CallerArgumentExpression #54132
Conversation
src/Compilers/VisualBasic/Portable/Symbols/ReducedExtensionMethodSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/ReducedExtensionMethodSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/SubstitutedParameterSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Source/LambdaParameterSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Retargeting/RetargetingParameterSymbol.vb
Outdated
Show resolved
Hide resolved
@@ -149,6 +149,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |||
End Get | |||
End Property | |||
|
|||
Friend Overrides ReadOnly Property CallerArgumentExpressionParameterIndex As Integer | |||
Get | |||
Return _originalParam.CallerArgumentExpressionParameterIndex |
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 looks fragile and probably incorrect, at least for delegate EndInvoke method. This implementation assumes that the target parameter is still going to be present and will remain at the same position, which probably can be wrong. The more robust approach is to reinterpret the attribute in the new signature. But before we do this, let's create some tests and figure out if this code path is even reachable in interesting scenarios. Perhaps simply returningh -1 is going to be good enough. #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.
BTW, I bet C# has similar class, we should check it as well.
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.
@AlekseyTs EndInvoke should only be cloning parameters that are ref
or out
:
roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateMethodSymbol.cs
Lines 402 to 409 in 22a7203
foreach (SourceParameterSymbol p in invoke.Parameters) | |
{ | |
if (p.RefKind != RefKind.None) | |
{ | |
var synthesizedParam = new SourceClonedParameterSymbol(originalParam: p, newOwner: this, newOrdinal: ordinal++, suppressOptional: true); | |
parameters.Add(synthesizedParam); | |
} | |
} |
So it is not going to ever clone a caller argument expression parameter. Hence, this path is unreachable for an EndInvoke. However, the implementation is needed for indexers and delegate invoke methods.
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.
So it is not going to ever clone a caller argument expression parameter.
Why do you think so? Is there a test that demonstrates 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.
However, the implementation is needed for indexers and delegate invoke methods.
I see nothing in this type that limits its application to these scenarios. Therefore, the implementation is not robust in the long term in my opinion.
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.
Why do you think so?
Per the code above that creates SourceClonedParameterSymbol
for EndInvoke
, a parameter is only created if its RefKind == RefKind.None
.
CallerArgumentExpressionAttribute
is only allowed for optional parameters (which should have RefKind.None.
Applying the attribute to a ref or out parameter generates a compile-time error:
error CS9006: The CallerArgumentExpressionAttribute may only be applied to parameters with default values
These are just my thoughts, I may be missing some interesting cases that you may have.
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.
CallerArgumentExpressionAttribute is only allowed for optional parameters (which should have RefKind.None.
Is this accurate? One cannot do this in IL or VB?
Applying the attribute to a ref or out parameter generates a compile-time error:
error CS9006: The CallerArgumentExpressionAttribute may only be applied to parameters with default values
Why are we talking about C# error, when we are working in VB? What prevents applying this attribute in IL for example?
This is perfectly valid code:
class Test
Shared Sub M(Optional ByRef x as String = "ddd")
End Sub
Shared Sub Test()
M()
End SUb
End Class
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 nothing in this type that limits its application to these scenarios. Therefore, the implementation is not robust in the long term in my opinion.
One way out could be making this type MustInherit, creating specialized NotInheritable derived types for specific scenarios, implementing the API in the derived types only.
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.
Behavioral-wise:
-
VB delegates doesn't allow optional parameter, hence, calling CallerArgumentExpressionIndex should never be done for EndInvoke invocations (the binder calls this property only if we're binding an optional parameter).
-
For IL, shouldn't we be dealing with metadata symbols, not source symbols? or I'm misunderstanding the concept here?
Implementation-wise:
I'm okay with making the type abstract and introducing derived type.
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 misunderstanding the concept here?
The discussion like that can go forever and will never end because reviewer's concerns can almost never be addressed by gueses, speculations or asumptions. The best way to alleviate reviewer's concerns is to create a test for the scenario in question and demonstrate the behavior, in this case the behavior of the API. Specifically here we are discussing an API behavior, whether there is an error reported or not has very little impact to the question what API's behavior is and what it should be. We are not discussing an issue with lowering/code generation. Since the type is not specific to any single particular scenario (a unique reason for creating a clone), each scenario should be tested individually and the tests should demonstrate that the implementation is in fact necessary and correct. Neccessity can be demonstrated by observing a break for a successful scenario when the implementation is removed/changed. For example, you are saying: "the implementation is needed for indexers and delegate invoke methods." Please demonstrate the fact with unit tests.
I'm okay with making the type abstract and introducing derived type.
I would prefer that we will create more specialized derived types. This way one can reason when instances are created and how they are used, can confirm that the code path is tested. Today anyone can create an instance of SourceClonedParameterSymbol
for whatever reason and "take" an implementation of this API, whether appropriate or not, with it without even be aware of the possible impact. I think we should prevent this from happening.
Return -1 | ||
End If | ||
|
||
Return _clonedFrom.CallerArgumentExpressionParameterIndex |
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 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.
@AlekseyTs I don't have enough info about COM, is it expected that order of parameters can change, etc?
Trying to test this code path locally with the following test, but there are few errors:
<Fact>
Public Sub ComClass()
Dim source As String = "
Imports Microsoft.VisualBasic
<ComClass(ComClass1.ClassId, ComClass1.InterfaceId, ComClass1.EventsId)>
Public Class ComClass1
' Use the Region directive to define a section named COM Guids.
#Region ""COM GUIDs""
' These GUIDs provide the COM identity for this class
' and its COM interfaces. You can generate
' these guids using guidgen.exe
Public Const ClassId As String = ""7666AC25-855F-4534-BC55-27BF09D49D46""
Public Const InterfaceId As String = ""54388137-8A76-491e-AA3A-853E23AC1217""
Public Const EventsId As String = ""EA329A13-16A0-478d-B41F-47583A761FF2""
#End Region
Public Sub New()
MyBase.New()
End Sub
Public Sub M(x As Integer, Optional y As String = """")
End Sub
End Class
Module Program
Sub Main()
Dim comObj As New ComClass1()
comObj.M(1+ 0)
End Sub
End Module
"
Dim compilation = CreateCompilation(source, targetFramework:=TargetFramework.NetCoreApp, references:={Net451.MicrosoftVisualBasic}, options:=TestOptions.ReleaseExe, parseOptions:=TestOptions.RegularLatest)
CompileAndVerify(compilation, expectedOutput:="").VerifyDiagnostics() ' PROTOTYPE(caller-expr): Figure out how to fix these:
' (5) : error BC35000: Requested operation is not available because the runtime library function 'System.Runtime.InteropServices.GuidAttribute..ctor' is not defined.
' (5) : error BC35000: Requested operation is not available because the runtime library function 'System.Runtime.InteropServices.ClassInterfaceAttribute..ctor' is not defined.
' (5) : error BC35000: Requested operation is not available because the runtime library function 'System.Runtime.InteropServices.DispIdAttribute..ctor' is not defined.
End Sub
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 don't have enough info about COM, is it expected that order of parameters can change, etc?
I do not remember these details. Since you are the PR author, it is on you to "defend" the changes. I would start with reviewing parts of the code that create symbols like this and analyze how and in what fashion this is happening. For tests, I would try to locate tests that are targeting this feature and would try to clone/adjust them as appropriate.
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.
@AlekseyTs I experimented with this. As far as I can tell, the SynthesizedComParameter
s are only exposed to emitter. It's not exposed to binder at all. It feels safe to just throw ExceptionUtilities.Unreachable
if my conclusions are correct.
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 experimented with this. As far as I can tell, the SynthesizedComParameters are only exposed to emitter. It's not exposed to binder at all. It feels safe to just throw ExceptionUtilities.Unreachable if my conclusions are correct.
Looks this way to me too.
src/Compilers/VisualBasic/Portable/Symbols/Source/SourceParameterSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Source/SourceParameterSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Wrapped/WrappedParameterSymbol.vb
Outdated
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/PEParameterSymbol.vb
Outdated
Show resolved
Hide resolved
So far, there are no tests. In reply to: 862484319 |
Thanks for the review @AlekseyTs! I'll work on it (but I may delay on this due to exams). |
src/Compilers/VisualBasic/Portable/Binding/Binder_Invocation.vb
Outdated
Show resolved
Hide resolved
Done with review pass (commit 2) |
closing and reopening for new build. |
|
Some jobs are failing with:
and others with:
I'm seeing CI issues with another recent PR too. Will give some time before re-triggering a new build. |
@333fred, @dotnet/roslyn-compiler For the second review. |
''' The index of a CallerArgumentExpression. The value -2 means uninitialized, -1 means | ||
''' Not found. Otherwise, the index of the CallerArgumentExpression. | ||
''' </summary> | ||
Private _lazyCallerArgumentExpressionParameterIndex As Integer = -2 |
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 attribute.HasValue AndAlso PEModule.TryExtractStringValueFromAttribute(attribute.Handle, parameterName) Then | ||
Dim parameters = ContainingSymbol.GetParameters() | ||
For i = 0 To parameters.Length - 1 | ||
If IdentifierComparison.Equals(parameters(i).Name, parameterName) Then |
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.
IdentifierComparison.Equals(parameters(i).Name, parameterName)
Couple of notes:
- I think the C# version of this method is broken, as it's using default string comparison. It should be using
Ordinal
. - This is potentially going to mean differing behavior between C# and VB. I think we likely should be using Ordinal here as well, not case-insensitive comparison. If VB defined a parameter named
param
but referred to it asParam
in the attribute, we would accept that here, but not in C#. #Closed
|
||
Friend Overrides ReadOnly Property CallerArgumentExpressionParameterIndex As Integer | ||
Get | ||
' This parameter cannot be optional. Hence, this property shouldn't be accessed. |
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 unsure why this reasoning applies to this parameter, but not the rest of the caller info attributes? #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.
@333fred It should apply, but I avoided changing the existing implementation. Let me change it so things are less confusing.
If attribute.ConstructorArguments.Single().TryDecodeValue(SpecialType.System_String, parameterName) Then | ||
Dim parameters = containingSymbol.GetParameters() | ||
For i = 0 To parameters.Length - 1 | ||
If IdentifierComparison.Equals(parameters(i).Name, parameterName) Then |
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.
End Sub | ||
|
||
Private Const P As String = NameOf(P) | ||
Sub Log(p As Integer, <CallerArgumentExpression(P)> Optional arg As String = ""<default-arg>"") |
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.
Done review pass (commit 44) |
@333fred I addressed your feedback. Can you take another look? Thanks! |
If attribute.HasValue AndAlso PEModule.TryExtractStringValueFromAttribute(attribute.Handle, parameterName) Then | ||
Dim parameters = ContainingSymbol.GetParameters() | ||
For i = 0 To parameters.Length - 1 | ||
If parameters(i).Name.Equals(parameterName, StringComparison.Ordinal) Then |
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.
parameters(i).Name.Equals(parameterName, StringComparison.Ordinal)
I do not think I agree with this change. Languages are already different in terms of case-sensitivity. I believe, the difference in behavior can be observed with named arguments, for example. And probably with other similar scenarios. @333fred, if you believe language rules should not apply for this scenario, please bring this to LDM #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.
@AlekseyTs Should I add a prototype until this is discussed in LDM?
My 2 cents: The current language case-insensitivity doesn't affect consuming the code in C#, but in this case, it will affect it as @333fred stated. So I'm leaning towards being case-sensitive.
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.
Should I add a prototype until this is discussed in LDM?
I would be fine with behavior on which I signed off (i.e. case-insensitive behavior consistent with named arguments) and a prototype comment to confirm that at LDM. I personally fine with putting the burden of interop with other languages (if one is required) on the developer and leave it to him/her to match the case.
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 do disagree, but as long as we have a prototype comment to follow up on this I'm ok with merging.
The sign-off is dismissed due to recent changes.
If attribute.ConstructorArguments.Single().TryDecodeValue(SpecialType.System_String, parameterName) Then | ||
Dim parameters = containingSymbol.GetParameters() | ||
For i = 0 To parameters.Length - 1 | ||
If parameters(i).Name.Equals(parameterName, StringComparison.Ordinal) Then |
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 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 47)
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 47)
@Youssef1313 Thanks for the contribution. |
Proposal: dotnet/csharplang#287
Test plan: #52745
Addresses part of LDM notes: https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-06-14.md