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

IBoundDelegateCreationExpression API #20095

Closed
333fred opened this issue Jun 7, 2017 · 11 comments
Closed

IBoundDelegateCreationExpression API #20095

333fred opened this issue Jun 7, 2017 · 11 comments
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Urgency-Now
Milestone

Comments

@333fred
Copy link
Member

333fred commented Jun 7, 2017

Given the following C# code:

class Program
{
    delegate void DType();
    void Main()
    {
        DType d1 = M1;
    }
    void M1()
    { }
}

We currently get the following IOperation tree:

IOperation tree for "DType d1 = M1"

IVariableDeclarationStatement (1 declarations) (OperationKind.VariableDeclarationStatement) (Syntax: 'DType d1 = M1;')
  IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration) (Syntax: 'DType d1 = M1;')
    Variables: Local_1: Program.DType d1
    Initializer: IMethodBindingExpression: void Program.M1() (OperationKind.MethodBindingExpression, Type: Program.DType) (Syntax: 'M1')
        Instance Receiver: IInstanceReferenceExpression (InstanceReferenceKind.Implicit) (OperationKind.InstanceReferenceExpression, Type: Program) (Syntax: 'M1')

This is inconsistent with the SemanticModel. For GetTypeInfo on the M1 IdentifierSyntaxNode, we have a Type of null and a ConvertedType of Program.DType. Note that the underlying BoundNode structure is actually a BoundLocalDeclaration, with an initializer of BoundConversion. We actually detect ConversionKind.MethodGroup and turn them into IMethodBindingExpressions. We should synthesize this expression under a conversion expression, and the type of the IMethodBindingExpression should be null.

Further complicating this is the VB equivalent. The following is the equivalent VB code:

Module Program
    Delegate Sub DType()
    Sub Main(args As String())
        Dim d1 As DType = AddressOf M1
    End Sub
    Sub M1()
    End Sub
End Module

This has an IOperation tree of:

IOperation tree for "Dim d1 As DType = AddressOf M1"

IVariableDeclarationStatement (1 declarations) (OperationKind.VariableDeclarationStatement) (Syntax: 'Dim d1 As D ... ddressOf M1')
  IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration) (Syntax: 'd1')
    Variables: Local_1: d1 As Program.DType
    Initializer: IOperation:  (OperationKind.None) (Syntax: 'AddressOf M1')

The OperationKind.None is because BoundMethodCreationExpression support in VB is not currently implemented, see #19917. The complication is that the SemanticModel API does not return the same results. AddressOf M1 has a Type of Program.DType, and a ConvertedType of Program.DType. Consistency here would, I think, imply that we shouldn't have an IConversionExpression, as we never seem to have them for identity conversions. However, this would be inconsistent with the C# API, which I think is more important here. We need to discuss how to resolve these at the IOp meeting.

@333fred 333fred added Area-Analyzers Discussion Feature - IOperation IOperation Concept-API This issue involves adding, removing, clarification, or modification of an API. labels Jun 7, 2017
@333fred
Copy link
Member Author

333fred commented Jun 7, 2017

Fyi @dotnet/analyzer-ioperation

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 8, 2017

DType d = M1 // Should be a delegate construction with a method reference
DType d = new DType(M1); // Should be a delegate construction with a method reference
DType d = new DType(() => { }); // Should be a delegate construction with a conversion and lambda
DType d = () => { }; // Should be a delegate construction with a conversion and a lambda


d as DType = addressof M1 ' Should be a delegate construction with a method reference
d as DType = new DType(addressof M1) ' Should be a delegate construction with a method reference 
d as DType = new DType(sub () end sub) ' Should be a delegate construction with a conversion and lambda
d as DType = sub () end sub ' Should be a delegate construction with a conversion and a lambda

Need IsCached (but maybe v2 if really hard to provide)

@333fred
Copy link
Member Author

333fred commented Jun 12, 2017

@CyrusNajmabadi @AlekseyTs @dotnet/analyzer-ioperation, another set of expressions to consider:

Action a = delegate() { };

Currently, this ends up being represented as an ILambda, with the appropriate IConversion from lambda to Action. Do we want this to appear the same as our idealized lambda above, DType d = () => { };, ie a delegate construction with a conversion and lambda?

@AlekseyTs
Copy link
Contributor

This is another syntax for a lambda, there should be no difference with a regular lambda, I think.

@333fred
Copy link
Member Author

333fred commented Jun 15, 2017

API proposal:

    /// <summary>
    /// Represents a delegate creation expression. This is created whenever a new delegate is created.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IDelegateCreationExpression : IHasArgumentsExpression
    {
        /// <summary>
        /// Symbol of the method being referenced by this delegate, if any.
        /// </summary>
        IMethodSymbol Method { get; }
    }

In C#, the argument will directly be the argument of the BoundDelegateCreationExpression. In VB, we will synthesize an IMethodBindingExpression from the BoundDelegateCreationExpression, and use that as the argument for the IDelegateCreationExpression.

For lambdas, we will synthesize IDelegateCreationExpressions when encountering specific IConversionExpressions. In C#, this will be any with ConversionKind.MethodGroup. In VB, I'm still determining what this list will look like, but at an initial look it appears that it will be all conversions with ConversionKind.Lambda set.

@333fred 333fred changed the title BoundDelegateCreationExpression does not have an IConversionExpression IBoundDelegateCreationExpression API Jun 16, 2017
@jinujoseph
Copy link
Contributor

Design Team Decision

It probably doesn't make sense to expose the constructor for the delegate and it doesn't make sense to expose it as arguments. We are contracting the delegate either from the method or from the conversion. 

We should probably have one node IOperationTarget Target is either conversion on top of the Lamda or it represents the method binding. 

@jinujoseph
Copy link
Contributor

Address #8913 as part of this work

@333fred
Copy link
Member Author

333fred commented Aug 31, 2017

Small point for design discussion today:

Given this code:

using System;

class P
{
    void M1()
    {
        Action m = (Action)M2;
    }

    void M2()
    {
    }
}

I'm going to create the following IOperation tree:

IDelegateCreationExpression (OperationKind.DelegateCreationExpression, Type: System.Action) (Syntax: '(Action)M2')
    Target: IMethodBindingExpression: void P.M2() (OperationKind.MethodBindingExpression, Type: System.Action) (Syntax: 'M2')
        Instance Receiver: IInstanceReferenceExpression (InstanceReferenceKind.Implicit) (OperationKind.InstanceReferenceExpression, Type: P) (Syntax: 'M2')

The question is how to indicate that this was an explicit cast. I talked with @AlekseyTs, and he proposed using the IsImplicit flag on the IDelegateCreationExpression for this. I like it, but for the sake of discussion, the other option is to put an IConversionExpression above the IDelegateCreationExpression.

@333fred
Copy link
Member Author

333fred commented Aug 31, 2017

Another small question: given the following code

using System;

class P
{
    void M1()
    {
        Action m = new Action((Action)M2);
    }

    void M2()
    {
    }
}

The underlying BoundNodes have a BoundDelegateCreation with a BoundConversion underneath. Should we detect this case and ensure there is only one IDelegateCreationExpression in the resulting tree, or show that there are in fact two creations here?

@AlekseyTs
Copy link
Contributor

Should we detect this case and ensure there is only one IDelegateCreationExpression in the resulting tree, or show that there are in fact two creations here?

I believe the two nodes are using different target methods, so we should keep both.

@CyrusNajmabadi
Copy link
Member

Design decision:

// Method reference, wrapped with explicit delegate creation.
Action m = (Action)M2;

// Method reference, wrapped with implicit delegate creation
Action m = M2;

@jinujoseph jinujoseph added Urgency-Now 4 - In Review A fix for the issue is submitted for review. Urgency-Soon and removed Urgency-Now labels Sep 14, 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 Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Urgency-Now
Projects
None yet
Development

No branches or pull requests

5 participants