Fix IOperation APIs for object and collection initializers #20689

mavasani · 2017-07-06T23:53:13Z

See #18781 (comment) for the final design.

See dotnet#18781 (comment) for the final design. Fixes dotnet#18781

mavasani · 2017-07-06T23:55:06Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1329,6 +1329,10 @@
    <Field Name="Expanded" Type="bool"/>
    <Field Name="ArgsToParamsOpt" Type="ImmutableArray&lt;int&gt;" Null="allow"/>
    <Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
+
+    <!-- BinderOpt and UseSetterForDefaultArgumentGeneration are added as a temporary solution for IOperation implementation and should probably be removed in the future -->


This is similar to the current approach used for BoundIndexerAccess

See https://github.com/mavasani/roslyn/blob/07421be2f2618cae7226438c06401fe0a40d7553/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml#L1505. @genlu for any context. #Resolved

Thanks for the context.
Do we have an issue tracking removing this in the future?

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

Filed #20787

mavasani · 2017-07-06T23:58:09Z

Also tagging @dotnet/analyzer-ioperation for review #Resolved

CyrusNajmabadi · 2017-07-07T02:10:09Z

👍 #Resolved

jinujoseph · 2017-07-07T21:28:47Z

cc/ @dotnet/roslyn-compiler #Resolved

mavasani · 2017-07-10T17:03:58Z

Ping.. need compiler signoff on the PR. #Resolved

cston · 2017-07-10T19:04:16Z

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

+            {
+                case SymbolKind.Field:
+                    var field = (FieldSymbol)boundObjectInitializerMember.MemberSymbol;
+                    return new LazyFieldReferenceExpression(field, instance, field, isInvalid, syntax, type, constantValue);


Is the second field parameter necessary for LazyFieldReferenceExpression? (Are the two parameter values ever different?) Same question for LazyEventReferenceExpression and LazyPropertyReferenceExpression.

Seems like it is redundant and there are more than one Operation types with such redundant parameters. I have added this to the list of comments to be addressed in the operation node generator: #20204 (comment)

Tagging @heejaechang as an FYI

cston · 2017-07-10T19:13:12Z

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

+                            return DeriveArguments(boundObjectInitializerMember.HasErrors,
+                                boundObjectInitializerMember.BinderOpt,
+                                property,
+                                property.SetMethod,


CreateBoundIndexerAccessOperation() uses GetOwnOrInheritedSetMethod(). Are there cases where that will give a different accessor here? If so, consider extracting a helper method that can be used by the factory methods.

Thanks, will change to use that method here as well.

jcouv · 2017-07-10T19:29:39Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

-                boundMember.Type,
-                hasErrors);
+                binderOpt: this,
+                useSetterForDefaultArgumentGeneration: true,


useSetterForDefaultArgumentGeneration [](start = 16, length = 37)

Is there any case where useSetterForDefaultArgumentGeneration is false?

This code path is while binding left of object member initializer, so it should always invoke the setter.

I'm suggesting useSetterForDefaultArgumentGeneration should not exist if it is always true. I could not find code that could set it to false, but we should double-check.

Sure, let me make that change.

jcouv · 2017-07-10T19:39:25Z

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

@@ -237,7 +247,7 @@ private IInvocationExpression CreateBoundCallOperation(BoundCall boundCall)
                        boundCall.ReceiverOpt != null &&
                        (boundCall.Method.IsVirtual || boundCall.Method.IsAbstract || boundCall.Method.IsOverride) &&
                        !boundCall.ReceiverOpt.SuppressVirtualCalls;
-            Lazy<ImmutableArray<IArgument>> argumentsInEvaluationOrder = new Lazy<ImmutableArray<IArgument>>(() => DeriveArguments(boundCall, boundCall.BinderOpt, boundCall.Method, boundCall.Method, boundCall.Arguments, boundCall.ArgumentNamesOpt, boundCall.ArgsToParamsOpt, boundCall.ArgumentRefKindsOpt, boundCall.Method.Parameters, boundCall.Expanded, boundCall.Syntax, boundCall.InvokedAsExtensionMethod));
+            Lazy<ImmutableArray<IArgument>> argumentsInEvaluationOrder = new Lazy<ImmutableArray<IArgument>>(() => DeriveArguments(boundCall.HasErrors, boundCall.BinderOpt, boundCall.Method, boundCall.Method, boundCall.Arguments, boundCall.ArgumentNamesOpt, boundCall.ArgsToParamsOpt, boundCall.ArgumentRefKindsOpt, boundCall.Method.Parameters, boundCall.Expanded, boundCall.Syntax, boundCall.InvokedAsExtensionMethod));


Break line #Resolved

jcouv · 2017-07-10T19:41:36Z

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

+
+        private IMemberReferenceExpression CreateBoundObjectInitializerMemberOperation(BoundObjectInitializerMember boundObjectInitializerMember)
+        {
+            Lazy<IOperation> instance = new Lazy<IOperation>(() => new InstanceReferenceExpression(InstanceReferenceKind.Implicit, boundObjectInitializerMember.HasErrors, syntax: boundObjectInitializerMember.Syntax, type: boundObjectInitializerMember.MemberSymbol.ContainingType, constantValue: default(Optional<object>)));


break line
In general, we don't have a strict limit, but I'm confused why some code above (for example, call to DeriveArguments) is on multiple lines (good, readable), but some other lines are crazy long...

I will add break lines for the couple of cases here. The formatting is not consistent across this file, so I have added a comment to #20204 (comment). Tagging @heejaechang as an FYI, we should address this in the operation node generator.

cston · 2017-07-10T20:18:20Z

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

@@ -73,7 +73,7 @@ internal static IArgument CreateArgumentOperation(ArgumentKind kind, IParameterS
            //      Note this check doesn't cover all scenarios. For example, when a parameter is a generic type but the type of the type argument 
            //      is undefined.
            if ((object)optionalParametersMethod == null 
-                || boundNode.HasAnyErrors
+                || hasErrors


It looks like callers are now passing boundNode.HasErrors rather than boundNode.HasAnyErrors. I'm not sure what the implications are but was that change intentional?

I'll change it to HasAnyErrors at all call sites, it was not an intentional change.

Requiring callers to pass HasAnyErrors seems error prone. Is it possible to revert to passing in the BoundNode instead?

Yes, that seems better.

cston · 2017-07-10T20:25:50Z

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

+            var expectedDiagnostics = new DiagnosticDescription[] {
+                // CS1061: 'MyList<string>' does not contain a definition for 'Add' and no extension method 'Add' accepting a first argument of type 'MyList<string>' could be found (are you missing a using directive or an assembly reference?)
+                //         var coll = /*<bind>*/new MyList<string> { "str" }/*</bind>*/;
+                Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, @"""str""").WithArguments("MyList<string>", "Add").WithLocation(7, 51)


The location was dropped from the comment, here and in other cases below.

Operation unit test generator doesn't seem to include the diagnostic location in the generated comment for expected diagnostics. Filed #20788 to track updating the generator.

cston · 2017-07-10T20:44:59Z

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

+        /// <summary>
+        /// Add method invoked on collection. Might be null for dynamic invocation.
+        /// </summary>
+        IMethodSymbol AddMethod { get; }


It looks like AddMethod is always null for dynamic invocation. Consider changing comment from Might be null ....

cston · 2017-07-10T20:48:23Z

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

+        End Function
+
+        Private Function CreateBoundCollectionElementInitializerOperation(boundExpression As BoundExpression) As IOperation
+            If (boundExpression.Kind <> BoundKind.Call) Then


Parentheses are not needed.

jcouv · 2017-07-10T20:57:50Z

src/Test/Utilities/Portable/Compilation/TestOperationWalker.cs

+
+        public override void VisitCollectionElementInitializerExpression(ICollectionElementInitializerExpression operation)
+        {
+            var addMethod = operation.AddMethod;


Two unused locals #Resolved

The behavior of this override and a few above doesn't add anything to the base behavior. They can be removed.

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

jcouv · 2017-07-10T21:00:29Z

        var ctor = operation.Constructor;

Unused local #Resolved

Refers to: src/Test/Utilities/Portable/Compilation/TestOperationWalker.cs:414 in 827341b. [](commit_id = 827341b, deletion_comment = False)

jcouv · 2017-07-10T21:02:56Z

        var text = operation.Text;

Seems a recurring pattern (unused local, override only calls base). Am I missing something? #Resolved

Refers to: src/Test/Utilities/Portable/Compilation/TestOperationWalker.cs:397 in 827341b. [](commit_id = 827341b, deletion_comment = False)

jcouv · 2017-07-10T21:13:17Z

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

-              Left: IOperation:  (OperationKind.None) (Syntax: 'Property2')
-              Right: IConversionExpression (ConversionKind.Invalid, Implicit) (OperationKind.ConversionExpression, Type: B, IsInvalid) (Syntax: '1')
-                  ILiteralExpression (Text: 1) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1) (Syntax: '1')
+          Initializer: IObjectOrCollectionInitializerExpression (OperationKind.ObjectOrCollectionInitializerExpression, Type: F, IsInvalid) (Syntax: '{ Property2 = 1 }')


Nit: I raised this question earlier, but I don't remember if it was answered. Do we know why some indents are 2 spaces, but others are 4? #Resolved

Some formatting will be fixed by Jinu's PR, but probably not this issue.

Filed #20789 to track the indenting issues.

mavasani · 2017-07-10T21:13:32Z

I believe @genlu wrote this walker as a checker that runs on any code and verifies no crashes on accessing properties off operation nodes. #Resolved

mavasani · 2017-07-11T16:28:53Z

@jcouv @cston - addressed your feedback. #Resolved

cston · 2017-07-11T16:32:43Z

LGTM

…ctAndCollectionInitializer2

jcouv

LGTM

mavasani · 2017-07-11T19:20:59Z

windows_debug_unit32_prtest failed due to flaky test failure tracked by #14592

mavasani · 2017-07-11T19:21:13Z

@dotnet-bot retest windows_debug_unit32_prtest please

mavasani · 2017-07-11T19:22:43Z

windows_debug_unit64_prtest failed due to flaky test #20370

mavasani · 2017-07-11T19:22:57Z

@dotnet-bot retest windows_debug_unit64_prtest please

mavasani · 2017-07-11T19:41:53Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

mavasani · 2017-07-11T19:43:14Z

@dotnet-bot retest windows_release_vs-integration_prtest please

Fix IOperation APIs for object and collection initializers

07421be

See dotnet#18781 (comment) for the final design. Fixes dotnet#18781

dnfclas added the cla-already-signed label Jul 6, 2017

mavasani commented Jul 6, 2017

View reviewed changes

mavasani added Area-Analyzers Area-Compilers labels Jul 6, 2017

mavasani requested review from a team, cston and jcouv July 6, 2017 23:56

Fix unit test failures

827341b

jinujoseph approved these changes Jul 7, 2017

View reviewed changes

cston reviewed Jul 10, 2017

View reviewed changes

jcouv reviewed Jul 10, 2017

View reviewed changes

heejaechang mentioned this pull request Jul 10, 2017

Create Generator similar to bound node generator that creates basic IOperation types #20204

Closed

cston reviewed Jul 10, 2017

View reviewed changes

jcouv reviewed Jul 10, 2017

View reviewed changes

mavasani mentioned this pull request Jul 11, 2017

Inconsistent indent in operation tree dumper #20789

Closed

Address PR feedback

a47c51c

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

241c5f1

…ctAndCollectionInitializer2

jcouv approved these changes Jul 11, 2017

View reviewed changes

mavasani added 3 commits July 11, 2017 10:03

Fix syntax error from merge conflict resolution

13d9b05

Fix unit test failures from merge resolution

22c7761

Fix unit test failures from baseline changes due to IsInvalid merge

51045b5

mavasani merged commit 7ee5148 into dotnet:features/ioperation Jul 11, 2017

mavasani deleted the ObjectAndCollectionInitializer2 branch July 11, 2017 20:24

Fix IOperation APIs for object and collection initializers #20689

Fix IOperation APIs for object and collection initializers #20689

Conversation

mavasani commented Jul 6, 2017

mavasani Jul 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Jul 6, 2017 • edited by jcouv Loading

CyrusNajmabadi commented Jul 7, 2017 • edited by jcouv Loading

jinujoseph commented Jul 7, 2017 • edited by jcouv Loading

mavasani commented Jul 10, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jul 10, 2017 • edited Loading

jcouv commented Jul 10, 2017 • edited Loading

jcouv Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Jul 10, 2017 • edited by jcouv Loading

mavasani commented Jul 11, 2017 • edited by jcouv Loading

cston commented Jul 11, 2017

jcouv left a comment

Choose a reason for hiding this comment

mavasani commented Jul 11, 2017

mavasani commented Jul 11, 2017

mavasani commented Jul 11, 2017

mavasani commented Jul 11, 2017

mavasani commented Jul 11, 2017

mavasani commented Jul 11, 2017

mavasani Jul 6, 2017 •

edited by jcouv

Loading

mavasani commented Jul 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi commented Jul 7, 2017 •

edited by jcouv

Loading

jinujoseph commented Jul 7, 2017 •

edited by jcouv

Loading

mavasani commented Jul 10, 2017 •

edited by jcouv

Loading

jcouv Jul 10, 2017 •

edited

Loading

jcouv Jul 10, 2017 •

edited

Loading

jcouv commented Jul 10, 2017 •

edited

Loading

jcouv commented Jul 10, 2017 •

edited

Loading

jcouv Jul 10, 2017 •

edited

Loading

mavasani commented Jul 10, 2017 •

edited by jcouv

Loading

mavasani commented Jul 11, 2017 •

edited by jcouv

Loading