Add IOperation APIs for Dynamic invocations and indexer access #21711

mavasani · 2017-08-23T23:26:05Z

Fixes #20114 and #20122

Fixes dotnet#20114 and dotnet#20122

mavasani · 2017-08-23T23:28:00Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IDynamicInvocationExpression.cs

+";
+            string expectedOperationTree = @"
+IDynamicInvocationExpression (OperationKind.DynamicInvocationExpression, Type: dynamic) (Syntax: 'c.M2(d)')
+  Expression: IOperation:  (OperationKind.None) (Syntax: 'c.M2')


OperationKind.None here seems to be due to a BoundMethodGroup node in the tree - we seem to be losing the IOperation tree for the receiver here ('c' for this example). @heejaechang is going to bring it up for discussion in tomorrow's design meeting. #Pending

mavasani · 2017-08-23T23:29:32Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory.vb

+                                                                                                                        boundLateInvocation.ArgumentsOpt.SelectAsArray(Function(n) Create(n)))
+                                                                                                                 End Function)
+            Dim argumentNames As ImmutableArray(Of String) = boundLateInvocation.ArgumentNamesOpt.NullToEmpty()
+            Dim argumentRefKinds As ImmutableArray(Of RefKind) = ImmutableArray(Of RefKind).Empty


BoundLateInvocation node in VB doesn't seem to be storing the array of RefKinds for each argument. @AlekseyTs, is this expected or should I open a compiler bug?

BoundLateInvocation node in VB doesn't seem to be storing the array of RefKinds for each argument.

I believe ref kinds in C# are coming from syntax, VB doesn't have syntax like that. #Closed

That said, if we are using language-agnostic IOperation nodes, we should probably be able to distinguish situations when

ref kinds are specified - C#, and

ref kinds are not specified - VB #Closed

@AlekseyTs - We can do so by returning a default array for VB. Does that sound fine?

mavasani · 2017-08-25T23:56:56Z

@dotnet retest ubuntu_14_debug_prtest please #Closed

AlekseyTs · 2017-08-28T23:52:53Z

src/Compilers/Core/Portable/Operations/IDynamicObjectCreationExpression.cs

@@ -12,6 +12,11 @@ namespace Microsoft.CodeAnalysis.Semantics
    public interface IDynamicObjectCreationExpression : IHasDynamicArgumentsExpression
    {
        /// <summary>
+        /// Name of the dynamically invoked member.
+        /// </summary>
+        string MemberName { get; }


string MemberName { get; } [](start = 8, length = 26)

I am not sure if this property belongs in IDynamicObjectCreationExpression. Isn't the member is always an instance constructor, which has predefined name? #Closed

The doc comment is actually misleading, it should be name of the dynamically created type, and the field name should be "Name".

it should be name of the dynamically created type

Why is the name of the type interesting? #Closed

Hmm, users can derive this name from syntax or the child dynamic member reference expression. Let me remove this property.

users can derive this name from syntax or the child dynamic member reference expression.

I assume the type symbol is available as the Type property of the node itself. Correct? #Closed

AlekseyTs · 2017-08-29T00:12:16Z

src/Compilers/Core/Portable/Operations/IDynamicInvocationExpression.cs

+    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
+    /// change it in the future.
+    /// </remarks>
+    public interface IDynamicInvocationExpression : IHasDynamicArgumentsExpression


IDynamicInvocationExpression [](start = 21, length = 28)

Should we expose information about type arguments? #Closed

Type arguments are stored neither on BoundDynamicInvocation node in C# nor on BoundLateInvocation node in VB. A dynamic invocation generally has the dynamic member reference as it's Expression child, which already exposes the type arguments.

AlekseyTs · 2017-08-29T00:13:44Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory.vb

+        Private Function CreateBoundLateInvocationOperation(boundLateInvocation As BoundLateInvocation) As IOperation
+            Dim expression As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() Create(boundLateInvocation.Member))
+
+            Dim isDynamicPropertyReference = boundLateInvocation.MethodOrPropertyGroupOpt?.Kind = BoundKind.PropertyGroup


isDynamicPropertyReference [](start = 16, length = 26)

I believe It is not always possible to accurately calculate this value for VB. #Closed

@AlekseyTs do you have an example where this could likely be wrong? I was able to write unit tests where this value is correctly computed to true for property reference and false for method reference.

do you have an example where this could likely be wrong?

Aren't there cases when we simply don't know whether the name refers to a property, or to a method? #Closed

I get your point, lets discuss the correct way to represent this in the next design meeting.

I believe that from VB point of view there is no (o very little) difference between late bound method invocation or late bound property indexing. At the same time, it looks like there is no such thing as dynamic property indexing on C# side. #Closed

AlekseyTs · 2017-08-29T00:14:51Z

Would it make sense to have distinct nodes for VB and C#? #Closed

mavasani · 2017-08-29T16:30:45Z

Would it make sense to have distinct nodes for VB and C#?

@AlekseyTs Are you suggesting this because of differences in the language specs? I am not very clear on why we need the distinction, but we can bring this up at the next design meeting. #Resolved

AlekseyTs · 2017-08-29T17:10:58Z

Are you suggesting this because of differences in the language specs?

Yes, because the difference in the languages. VB late binding is very different from C# dynamic. Semantics is different. #Closed

1) Use IDynamicInvocationExpression for VB late bound invocation and C# dynamic invocation. 2) Use IDynamicIndexerAccessExpression for C# dynamic indexer access; not used in VB. 3) Remove IDynamicObjectCreationExpression.MemberName property. 4) Add extension methods on IHasDynamicArgumentExpression to get optional argument name and ref kind for an argument at a given index.

mavasani · 2017-09-05T17:06:53Z

This PR has been updated as per last week's design decisions and is ready for review. #Closed

mavasani · 2017-09-06T16:00:54Z

Ping for reviews. #Closed

cston · 2017-09-06T16:28:31Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+        {
+            Lazy<IOperation> expression = new Lazy<IOperation>(() => Create(boundDynamicInvocation.Expression));
+            ImmutableArray<ISymbol> applicableSymbols = StaticCast<ISymbol>.From(boundDynamicInvocation.ApplicableMethods);
+            Lazy<ImmutableArray<IOperation>> arguments = new Lazy<ImmutableArray<IOperation>>(() => boundDynamicInvocation.Arguments.SelectAsArray(n => Create(n)));


Consider extracting a helper method for creating a Lazy<ImmutableArray<IOperation>> from ImmutableArray<BoundExpression> since there appear to be many instances of this. #Pending

Good idea, but there are large number of instances of this and I would prefer it to be a separate cleanup change, especially given we want to write an operation generator. I have added this suggestion to the issue tracking implementation of the operation generator: #20204 (comment) #Pending

cston · 2017-09-06T16:34:53Z

...Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IDynamicIndexerAccessExpression.cs

+        var x = /*<bind>*/d[i]/*</bind>*/;
+    }
+
+    public int this[int i] => 0;


Property is not needed here nor in the test below.

cston · 2017-09-06T16:36:10Z

...Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IDynamicIndexerAccessExpression.cs

+    {
+        var x = /*<bind>*/c.M2[i]/*</bind>*/;
+    }
+}


Is this test different from the test above? #Resolved

Good catch, I intended the parameter to be C c not dynamic c. I have fixed it now. #Resolved

cston · 2017-09-06T17:03:58Z

src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs


    static void M(dynamic d, C c)
    {
        // This could be either of the one-parameter overloads so we allow it.
        c.Goo(d);

-        // Doesn't constraints of generic overloads.
-        c.Goo<short>(d, d);
+        /*<bind>*/c.Goo<short>(d, d)/*</bind>*/; // Doesn't constraints of generic overloads.


What does the comment mean?

I am not sure - I have just retained the existing comment. I had to move it outside the /*<bind>*/ annotation as otherwise the test framework was unable to find the correct syntax node to fetch operation.

cston · 2017-09-06T17:25:26Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+            }
+
+            var expression = (HasDynamicArgumentsExpression)dynamicExpression;
+            if (expression.ArgumentNames.IsDefaultOrEmpty)


Please extract a local for expression.ArgumentNames. #Resolved

cston · 2017-09-06T17:26:01Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+            }
+
+            var expression = (HasDynamicArgumentsExpression)dynamicExpression;
+            if (expression.ArgumentRefKinds.IsDefault)


Please extract a local for expression.ArgumentRefKinds.

cston · 2017-09-06T17:28:04Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+            if (index < 0 || index >= expression.ArgumentNames.Length)
+            {
+                throw new ArgumentOutOfRangeException(nameof(index));
+            }


This check is unnecessary, ArgumentNames[index] will throw the same exception. Same comment for GetArgumentRefKind. #Resolved

AlekseyTs · 2017-09-08T20:22:41Z

src/Compilers/Core/Portable/Operations/IHasDynamicArgumentsExpression.cs

-
-        /// <summary>
-        /// List of applicable symbols that are dynamically bound to the <see cref="Name"/>.
+        /// List of applicable symbols that are dynamically bound.
        /// </summary>
        ImmutableArray<ISymbol> ApplicableSymbols { get; }


ImmutableArray ApplicableSymbols { get; } [](start = 8, length = 50)

I believe we decided to not expose any applicable symbols. In any case, given the name of the interface, it feels like this property doesn't belong in it. #Closed

If we remove the property, is there a value in having this interface at all?

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

I have removed the interface and moved the Arguments property to each IDynamicXXXExpression interface. #Closed

mavasani · 2017-09-11T23:16:26Z

@AlekseyTs - addressed your feedback. One more round of review please.

mavasani · 2017-09-12T11:45:51Z

@dotnet-bot retest windows_debug_unit32_prtest please

… expression for a bound method group that is a child of dynamic invocation

AlekseyTs · 2017-09-13T23:05:55Z

@mavasani Could you please confirm that build failures aren't caused by the changes in this PR? #Closed

mavasani · 2017-09-13T23:10:11Z

Thanks @AlekseyTs - some failures are irrelevant and some seem to be from tests not marked with IOperation compiler trait attribute. Let me fix this up.

…amicInvocation

mavasani · 2017-09-14T01:24:48Z

@dotnet-bot retest windows_debug_unit64_prtest please

mavasani · 2017-09-14T01:24:59Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

mavasani · 2017-09-14T01:25:13Z

@dotnet-bot retest windows_release_vs-integration_prtest please

mavasani · 2017-09-14T14:11:58Z

@dotnet/roslyn-infrastructure All the VSI runs seems to be failing with the following:

 	Microsoft.VisualStudio.Shell.15.0.dll!Microsoft.Internal.VisualStudio.PlatformUI.WindowHelper.ShowModal(System.Windows.Window window, System.IntPtr parent)	Unknown
 	Microsoft.VisualStudio.CommonIDE.dll!Microsoft.VisualStudio.CommonIDE.ProductKeyDialogLauncher.LaunchProductKeyDialogHelper(Microsoft.VisualStudio.LicenseManagement.Interop.IVsLicensingState licensingState, Microsoft.Internal.VisualStudio.Shell.Interop.IVsProductKeyDialogLauncherCallback callback, System.IDisposable telemetryActivity)	Unknown
 	Microsoft.VisualStudio.CommonIDE.dll!Microsoft.VisualStudio.CommonIDE.ProductKeyDialogLauncher.LaunchProductKeyDialog(Microsoft.Internal.VisualStudio.Shell.Interop.IVsProductKeyDialogLauncherCallback callback)	Unknown

Can someone please update the product key on the machines?

AlekseyTs · 2017-09-14T17:17:43Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+        /// </summary>
+        /// <param name="dynamicExpression">Dynamic or late bound expression.</param>
+        /// <param name="index">Argument index.</param>
+        internal static string GetArgumentName(this HasDynamicArgumentsExpression dynamicExpression, int index)


int index [](start = 101, length = 9)

Index should be checked for range even when argumentNames.IsDefaultOrEmpty. #Closed

AlekseyTs · 2017-09-14T17:20:48Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+            return GetArgumentRefKind((HasDynamicArgumentsExpression)dynamicExpression, index);
+        }
+
+        internal static RefKind? GetArgumentRefKind(this HasDynamicArgumentsExpression dynamicExpression, int index)


index [](start = 110, length = 5)

Index should be checked for range even when argumentRefKinds.IsDefaultOrEmpty #Closed

AlekseyTs · 2017-09-14T17:38:25Z

src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs

+";
+            string expectedOperationTree = @"
+IInvocationExpression ( ? A.B.()) (OperationKind.InvocationExpression, Type: ?, IsInvalid) (Syntax: 'M(d)')
+  Instance Receiver: IOperation:  (OperationKind.None, IsInvalid) (Syntax: 'M')


Instance Receiver: IOperation: (OperationKind.None, IsInvalid) (Syntax: 'M') [](start = 2, length = 77)

Why are we getting None here? Please open an issue to follow up. #Closed

Filed #22121 #Resolved

AlekseyTs · 2017-09-14T17:41:14Z

src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs

+  Instance Receiver: IOperation:  (OperationKind.None, IsInvalid) (Syntax: 'M')
+  Arguments(1):
+      IArgument (ArgumentKind.Explicit, Matching Parameter: null) (OperationKind.Argument, IsInvalid) (Syntax: 'd')
+        ILocalReferenceExpression: d (OperationKind.LocalReferenceExpression, Type: dynamic, IsInvalid) (Syntax: 'd')


ILocalReferenceExpression: d (OperationKind.LocalReferenceExpression, Type: dynamic, IsInvalid) (Syntax: 'd') [](start = 8, length = 109)

I am not sure why this node is marked as invalid. Please open an issue to follow up. #Closed

#22122 #Resolved

AlekseyTs · 2017-09-14T17:57:21Z

src/Compilers/Core/Portable/Operations/OperationExtensions.cs

+        /// </summary>
+        /// <param name="dynamicExpression">Dynamic or late bound expression.</param>
+        /// <param name="index">Argument index.</param>
+        public static string GetArgumentName(this IDynamicInvocationExpression dynamicExpression, int index)


GetArgumentName [](start = 29, length = 15)

For all new public methods, please add targeted tests to cover scenarios with an invalid input, i.e. when validation fails and we throw. #Closed

AlekseyTs · 2017-09-14T17:58:17Z

Done with review pass (iteration 10) #Closed

…amicInvocation

mavasani · 2017-09-14T21:44:12Z

@AlekseyTs - addressed your feedback.

mavasani · 2017-09-14T23:05:28Z

@dotnet-bot retest ubuntu_16_debug_prtest please

AlekseyTs

LGTM

mavasani · 2017-09-15T00:26:33Z

Integration tests queues are failing due to an unrelated expired product key issue.

Add IOperation APIs for Dynamic invocations and indexer access

2cf8641

Fixes dotnet#20114 and dotnet#20122

dnfclas added the cla-already-signed label Aug 23, 2017

mavasani commented Aug 23, 2017

View reviewed changes

Fix unit test failures

dd969b6

mavasani added Area-Analyzers Area-Compilers labels Aug 24, 2017

mavasani requested review from a team, cston and jcouv August 24, 2017 16:10

AlekseyTs reviewed Aug 28, 2017

View reviewed changes

AlekseyTs reviewed Aug 29, 2017

View reviewed changes

mavasani mentioned this pull request Aug 31, 2017

IOperation API support for BoundDynamicInvocation #20114

Closed

cston reviewed Sep 6, 2017

View reviewed changes

AlekseyTs reviewed Sep 8, 2017

View reviewed changes

Fix unit test failures

87d7b01

Address feedback from Aleksey and generate a dynamic member reference…

d355ab8

… expression for a bound method group that is a child of dynamic invocation

mavasani added 2 commits September 13, 2017 16:12

Merge remote-tracking branch 'upstream/features/ioperation' into IDyn…

51e9d53

…amicInvocation

Fix unit test failures from merge

334961f

AlekseyTs reviewed Sep 14, 2017

View reviewed changes

This was referenced Sep 14, 2017

Unexpected OperationKind.None for instance receiver of dynamic invocation with errors #22121

Closed

Unexpected IsInvalid = true for local reference in dynamic invocation with errors #22122

Closed

mavasani added 2 commits September 14, 2017 14:08

Address PR feedback and add more tests

cfee241

Merge remote-tracking branch 'upstream/features/ioperation' into IDyn…

b20852b

…amicInvocation

AlekseyTs approved these changes Sep 15, 2017

View reviewed changes

mavasani merged commit 339083e into dotnet:features/ioperation Sep 15, 2017

mavasani deleted the IDynamicInvocation branch September 15, 2017 00:26

mavasani mentioned this pull request Sep 15, 2017

IOperation API shape for BoundDynamicIndexerAccess #20122

Closed

jinujoseph mentioned this pull request Sep 20, 2017

IOperation: VB - BoundLateInvocation should implement specific interface #8916

Closed

Add IOperation APIs for Dynamic invocations and indexer access #21711

Add IOperation APIs for Dynamic invocations and indexer access #21711

Conversation

mavasani commented Aug 23, 2017

mavasani Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Aug 25, 2017 • edited Loading

AlekseyTs Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Aug 29, 2017 • edited Loading

mavasani commented Aug 29, 2017 • edited Loading

AlekseyTs commented Aug 29, 2017 • edited Loading

mavasani commented Sep 5, 2017 • edited Loading

mavasani commented Sep 6, 2017 • edited Loading

cston Sep 6, 2017 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 6, 2017 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 6, 2017 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 6, 2017 • edited by mavasani Loading

Choose a reason for hiding this comment

AlekseyTs Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Sep 11, 2017 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

mavasani commented Sep 11, 2017

mavasani commented Sep 12, 2017

AlekseyTs commented Sep 13, 2017 • edited Loading

mavasani commented Sep 13, 2017

mavasani commented Sep 14, 2017

mavasani commented Sep 14, 2017

mavasani commented Sep 14, 2017

mavasani commented Sep 14, 2017

AlekseyTs Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani Sep 14, 2017 • edited Loading

mavasani Aug 23, 2017 •

edited

Loading

AlekseyTs Aug 24, 2017 •

edited

Loading

AlekseyTs Aug 24, 2017 •

edited

Loading

mavasani commented Aug 25, 2017 •

edited

Loading

AlekseyTs Aug 28, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs Aug 29, 2017 •

edited

Loading

AlekseyTs commented Aug 29, 2017 •

edited

Loading

mavasani commented Aug 29, 2017 •

edited

Loading

AlekseyTs commented Aug 29, 2017 •

edited

Loading

mavasani commented Sep 5, 2017 •

edited

Loading

mavasani commented Sep 6, 2017 •

edited

Loading

cston Sep 6, 2017 •

edited by mavasani

Loading

mavasani Sep 11, 2017 •

edited

Loading

cston Sep 6, 2017 •

edited by mavasani

Loading

mavasani Sep 11, 2017 •

edited

Loading

cston Sep 6, 2017 •

edited by mavasani

Loading

cston Sep 6, 2017 •

edited

Loading

cston Sep 6, 2017 •

edited by mavasani

Loading

AlekseyTs Sep 8, 2017 •

edited

Loading

mavasani Sep 11, 2017 •

edited by AlekseyTs

Loading

AlekseyTs commented Sep 13, 2017 •

edited

Loading

AlekseyTs Sep 14, 2017 •

edited

Loading

AlekseyTs Sep 14, 2017 •

edited

Loading

AlekseyTs Sep 14, 2017 •

edited

Loading

mavasani Sep 14, 2017 •

edited

Loading

AlekseyTs Sep 14, 2017 •

edited

Loading

mavasani Sep 14, 2017 •

edited

Loading

AlekseyTs Sep 14, 2017 •

edited

Loading

AlekseyTs commented Sep 14, 2017 •

edited

Loading