Skip to content
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

IObjectCreationExpression API shape #18115

Closed
mavasani opened this issue Mar 23, 2017 · 4 comments · Fixed by #19278
Closed

IObjectCreationExpression API shape #18115

mavasani opened this issue Mar 23, 2017 · 4 comments · Fixed by #19278
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation
Milestone

Comments

@mavasani
Copy link
Contributor

    public interface IObjectCreationExpression : IHasArgumentsExpression
    {
        /// <summary>
        /// Constructor to be invoked on the created instance.
        /// </summary>
        IMethodSymbol Constructor { get; }
        /// <summary>
        /// Explicitly-specified member initializers.
        /// </summary>
        ImmutableArray<ISymbolInitializer> MemberInitializers { get; }
    }

The shady part is MemberInitializers, which only expose a part of the IOperation tree within the object member initializer.

Let me give an example:

Class F
    Public Field As Integer
End Class

Class C
    Public Sub M1()
        Dim x2 = New F() With {.Field = 2}
    End Sub
End Class

IOperation tree for object creation:

IOperation tree for "Dim x2 = New F() With {.Field = 2}"

IVariableDeclarationStatement (1 variables) (OperationKind.VariableDeclarationStatement)
  IVariableDeclaration: x2 As F (OperationKind.VariableDeclaration)
    Initializer: IObjectCreationExpression (Constructor: Sub F..ctor()) (OperationKind.ObjectCreationExpression, Type: F)
        Member Initializers: IFieldInitializer (Field: F.Field As System.Int32) (OperationKind.FieldInitializerInCreation)
            ILiteralExpression (Text: 2) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 2)

IOperation tree for it's child node, object member initializer:

IOperation tree for "With {.Field = 2}"

IOperation (OperationKind.None)

IOperation tree for one of the child nodes of the member initializer, NamedFieldInitializerSyntax:

IOperation tree for ".Field = 2"

IAssignmentExpression (OperationKind.AssignmentExpression, Type: System.Int32)
  Left: IFieldReferenceExpression: F.Field As System.Int32 (OperationKind.FieldReferenceExpression, Type: System.Int32)
      Instance Receiver:  (OperationKind.None)
  Right: ILiteralExpression (Text: 2) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 2)

I think we either need to change MemberInitializers to return the entire IOperation tree for each initializer (which would be the IAssignmentExpression for the field initializer) OR replace MemberInitializers with InitializerOpt of new OperationKind.ObjectMemberInitializer, which exposes all the initializer expressions.

@mavasani mavasani added Area-Analyzers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation labels Mar 23, 2017
@mavasani
Copy link
Contributor Author

mavasani commented Apr 11, 2017

Design Team Decision:

  1. Existing shape of IObjectCreationExpression is fine.
  2. Ensure that we do not return IAssignmentExpression when asked for IOperation for NamedFieldInitializerSyntax, i.e. .Field = 2 - this should make the tree consistent

Open question: Do we need to still somehow ensure we expose IFieldReferenceExpression in the IOperation tree rooted at New F() With {.Field = 2}? Otherwise, analyzers analyzing IFieldReferenceExpression will not receive any callbacks when analyzing this code.

cc @AlekseyTs , @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Member

Do we need to still somehow ensure we expose IFieldReferenceExpression

That's an interesting question. Can you add it to the questions for the design meeting?

I see the benefit of it being in the tree. That way people can just register a single callback. One way we could do that would be to have IFieldInitializer point at the IFieldReferenceExpression instead of directly at the IFieldSymbol.

Note: IFieldInitializer really seems to be defined for the actual field case. i.e. "private int i = 0" or "dim i, j, as new whatever()". I'm not sure i like that it is being used in the "new with" expression.

@jinujoseph
Copy link
Contributor

jinujoseph commented Apr 18, 2017

Design Team Decision:

NamedFieldInitializerSyntax, i.e. .Field = 2 we will return IAssignmentExpression that is Initializers for IObjectCreationExpression will be all IOperations instead of ISymbolInitializers

We should probably have something like IObjectInitializer, which can contain several of these assignments and their invocations (add with arguments), For now we can say it's an array of IOperation.

  • Check if it will work for nested Initialization.
  • Check if it makes sense to make a special node instead of collection Initializer.

@jinujoseph jinujoseph modified the milestones: 15.3, 15.later Apr 18, 2017
@jinujoseph
Copy link
Contributor

blocked design decision on #18781

@mavasani mavasani modified the milestones: 15.3, 15.later May 4, 2017
mavasani added a commit to mavasani/roslyn that referenced this issue May 4, 2017
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
@jinujoseph jinujoseph added the 4 - In Review A fix for the issue is submitted for review. label May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants