IObjectCreationExpression API Change #19278

mavasani · 2017-05-04T22:46:41Z

Customer scenario

All IOperation based analyzers will generate large number of false positives OR not analyze code within object and collection initializers. Given that such a code pattern is very common, this can be a dogfood blocker for IOperation analyzers (FXCop).

Bugs this fixes:

Fixes #18115

There are still 2 remaining bugs affecting this area:

IOperation API shape for object and collection initializer expressions #18781: IOperation API shape for collection initializer expressions
Missing Field/Property reference expression nodes in object creation initializer node #19276: Missing Field/Property reference expression nodes in object creation initializer node

Workarounds, if any

The analyzer users will have to either suppress all IOperation analyzers on methods containing object/collection initializers or disable these analyzers.

Risk

Low. We are just returning the operation nodes as they are in the bound tree rather than synthesizing new symbol initializer nodes.

Performance impact

None

Is this a regression from a previous update?

No, this was the original IOperation API implementation

Root cause analysis:

The underlying reason is that the current API shape of IObjectCreationExpression only exposes member initializers, not collection initializers.

There are couple of changes in this PR:

API change: ImmutableArray<ISymbolInitializer> MemberInitializers is changed to ImmutableArray<IOperation> Initializers as per design time decision.
Implementation changes:
1. Instead of returning the member initializers as synthesized ISymbolInitializer nodes, we now return member initializers as IAssignmentExpression nodes. This ensures completeness of IOperation tree.
2. Now we also return the collection initializer expressions within an object creation expression.

How was the bug found?

Dogfooding.

There are couple of changes here: 1. API change: `ImmutableArray<ISymbolInitializer> MemberInitializers` is changed to `ImmutableArray<IOperation> Initializers`. 2. Implementation changes: 1. Instead of returning the member initializers as synthesized ISymbolInitializer nodes, we now return member intializers as IAssignmentExpression nodes. This ensures completeness of IOperation tree. 2. Now we also return the collection intializer expressions within an object creation expression. Fixes dotnet#18115 There are 2 bugs still affecting this area: 1. dotnet#18781: IOperation API shape for collection initializer expressions 2. dotnet#19276: Missing Field/Property reference expression nodes in object creation initializer node

mavasani · 2017-05-04T22:47:05Z

Tagging @dotnet/analyzer-ioperation for review.

mavasani · 2017-05-04T22:47:27Z

Also tagging @AlekseyTs and @cston

cston · 2017-05-05T15:18:11Z

...pilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.vb

@@ -238,33 +238,48 @@ Friend Class [Class]
    Public Property C As [Class]

    Public Sub M(x As Integer, y As Integer, z As Integer)
-        Dim c = New [Class]() With {'BIND:"New [Class]() With {"
+        Dim c = New [Class]() With {'BIND:"New [Class]() With {"'BIND:"New [Class]() With {'BIND:"New [Class]() With {""


What are the extra 'BIND ... comments for? #Pending

Seems like they got left behind accidentally when I copied another existing test and modified it. There are few more tests with extra bind comments, I will clean them separately. #Closed

cston · 2017-05-05T15:18:50Z

LGTM

…s. I have filed dotnet#19300 to track the skipped tests

mavasani · 2017-05-05T20:26:16Z

@dotnet-bot retest windows_debug_unit64_prtest please

mavasani · 2017-05-08T16:17:03Z

@AlekseyTs Can you take a look? Thanks.

AlekseyTs · 2017-05-09T20:41:40Z

...pilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.vb

-        Member Initializers(1): IPropertyInitializer (Property: Property [Class].X As System.Int32) (OperationKind.PropertyInitializerInCreation) (Syntax: '.X = z')
-            IParameterReferenceExpression: z (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'z')
+  Initializers(4): IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Void) (Syntax: '.X = x')
+      Left: IIndexedPropertyReferenceExpression: Property [Class].X As System.Int32 (OperationKind.PropertyReferenceExpression, Type: System.Int32) (Syntax: 'X')


Left: IIndexedPropertyReferenceExpression: Property [Class].X As System.Int32 (OperationKind.PropertyReferenceExpression, Type: System.Int32) (Syntax: 'X') [](start = 6, length = 155)

It doesn't look like the property is an indexed property. The same issue is with other properties.

Yes, this is a bug that is already fixed in features/ioperation branch. I also updated the test in that branch accordingly: https://github.com/dotnet/roslyn/blob/features/ioperation/src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.vb#L94

AlekseyTs · 2017-05-09T20:58:11Z

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

+  Initializers(4): IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Int32) (Syntax: 'X = x')
+      Left: IOperation:  (OperationKind.None) (Syntax: 'X')
+      Right: IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x')
+    IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Collections.Generic.List<System.Int32>) (Syntax: 'Y = { x, y, 3 }')


IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Collections.Generic.List<System.Int32>) (Syntax: 'Y = { x, y, 3 }') [](start = 4, length = 139)

This feels surprising. Does the assignment really happen in this scenario?

Would you like me to file an issue or are you suggesting we not expose the IAssignmentExpression in the operation tree?

I think opening an issue should be fine at this point.

Filed #19375

AlekseyTs · 2017-05-09T21:16:54Z

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

+
+namespace Microsoft.CodeAnalysis.CSharp.UnitTests
+{
+    public partial class IOperationTests : SemanticModelTestBase


IOperationTests [](start = 25, length = 15)

It looks like there are no tests for an initializer target in the form of an argument list.

initializer_target
: identifier
| '[' argument_list ']'
;

Yes, this PR is not meant to add complete set of tests for IObjectCreationExpression. I will add them later as part of unit testing effort #17588

AlekseyTs

LGTM

mavasani · 2017-05-09T22:15:53Z

Tagging @MattGertz for ask mode approval.

…or an IOperation API change merged yesterday (dotnet#19278).

dnfclas added the cla-already-signed label May 4, 2017

cston reviewed May 5, 2017

View reviewed changes

mavasani mentioned this pull request May 5, 2017

OperationKind.FieldInitializerInCreation and OperationKind.PropertyInitializerInCreation are redundant #19300

Closed

Fix one unit test and skip couple of unit tests due to the API change…

1d9b64a

…s. I have filed dotnet#19300 to track the skipped tests

jinujoseph added the Area-Compilers label May 8, 2017

AlekseyTs reviewed May 9, 2017

View reviewed changes

AlekseyTs approved these changes May 9, 2017

View reviewed changes

mavasani mentioned this pull request May 9, 2017

Unexpected IAssignmentExpression in an object initializer #19375

Closed

MattGertz added the Approved to merge label May 9, 2017

mavasani merged commit c16e998 into dotnet:master May 9, 2017

mavasani added a commit to mavasani/roslyn that referenced this pull request May 10, 2017

Re-enable roslyn analyzers on Roslyn.sln and update public api file f…

1cbe2ce

…or an IOperation API change merged yesterday (dotnet#19278).

mavasani mentioned this pull request May 10, 2017

Re-enable roslyn analyzers on Roslyn.sln and update public api file f… #19396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IObjectCreationExpression API Change #19278

IObjectCreationExpression API Change #19278

mavasani commented May 4, 2017

mavasani commented May 4, 2017

mavasani commented May 4, 2017

cston May 5, 2017 •

edited by mavasani

Loading

mavasani May 5, 2017 •

edited

Loading

cston commented May 5, 2017

mavasani commented May 5, 2017

mavasani commented May 8, 2017

AlekseyTs May 9, 2017

mavasani May 9, 2017

AlekseyTs May 9, 2017

mavasani May 9, 2017

AlekseyTs May 9, 2017

mavasani May 9, 2017

AlekseyTs May 9, 2017

mavasani May 9, 2017

AlekseyTs left a comment

mavasani commented May 9, 2017

IObjectCreationExpression API Change #19278

IObjectCreationExpression API Change #19278

Conversation

mavasani commented May 4, 2017

mavasani commented May 4, 2017

mavasani commented May 4, 2017

cston May 5, 2017 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani May 5, 2017 • edited Loading

Choose a reason for hiding this comment

cston commented May 5, 2017

mavasani commented May 5, 2017

mavasani commented May 8, 2017

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

AlekseyTs left a comment

Choose a reason for hiding this comment

mavasani commented May 9, 2017

cston May 5, 2017 •

edited by mavasani

Loading

mavasani May 5, 2017 •

edited

Loading