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

IOperation: BoundLambda - value returned by IOperation.Type property might be inconsistent across languages #8851

Closed
AlekseyTs opened this issue Feb 17, 2016 · 9 comments

Comments

@AlekseyTs
Copy link
Contributor

BoundLambda node in VB never has a Type, whereas in C# I believe it's Type is a target delegate type. If implementation of IOperation.Type simply returns value of the Type property, then returned value for VB will always be null and often (if not always) not null for C#.

I think IOperation.Type for ILambdaExpression should always be null and the target type (a delegate type or an Expression) can be discovered from the conversion on top of the lambda.

@AlekseyTs AlekseyTs added Bug Language-C# Language-VB Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. labels Feb 17, 2016
@AlekseyTs AlekseyTs added this to the 1.2 milestone Feb 17, 2016
@genlu genlu added the Feature - IOperation IOperation label Feb 18, 2016
@srivatsn srivatsn modified the milestones: 1.2, 1.3 Feb 26, 2016
@ManishJayaswal ManishJayaswal modified the milestones: 1.3, 2.0 (RC) May 31, 2016
@jinujoseph jinujoseph removed the Concept-API This issue involves adding, removing, clarification, or modification of an API. label May 3, 2017
@jinujoseph jinujoseph modified the milestones: 15.6, 15.3 May 8, 2017
@333fred 333fred modified the milestones: 15.later, 15.3 May 10, 2017
@333fred
Copy link
Member

333fred commented May 25, 2017

@AlekseyTs is correct. Example time.

Given the following C# code:

using System;

class Program
{
    static void Main(string[] args)
    {
        Action test = () => { };
    }
}

We have a syntax tree that looks like this:
image

If we get the IOperation node for the ParenthesizedLambdaExpression, we currently get this tree:

IOperation tree for "() => { }"

ILambdaExpression (Signature: lambda expression) (OperationKind.LambdaExpression, Type: System.Action) (Syntax: '() => { }')
  IBlockStatement (1 statements) (OperationKind.BlockStatement) (Syntax: '{ }')
    IReturnStatement (OperationKind.ReturnStatement) (Syntax: '{ }')

For the following VB code:

Module Program
    Sub Main(args As String())
        Dim test = Sub()
                   End Sub
    End Sub
End Module

We get the following sytnax tree:
image

Getting the IOperation of the MultiLineSubLambdaExpression returns this IOperation tree:

IOperation tree for "Sub()
                   End Sub"

IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: Sub <generated method>()) (Syntax: 'Sub() ... End Sub')
  ILambdaExpression (Signature: Sub ()) (OperationKind.LambdaExpression, Type: null) (Syntax: 'Sub() ... End Sub')
    IBlockStatement (2 statements) (OperationKind.BlockStatement) (Syntax: 'Sub() ... End Sub')
      ILabelStatement (Label: exit) (OperationKind.LabelStatement) (Syntax: 'End Sub')
      IReturnStatement (OperationKind.ReturnStatement) (Syntax: 'End Sub')

The difference here is twofold:

  1. In VB, as Aleksey said, the type of the ILambdaExpression is null, and in C# it's Action.
  2. C# uses GetOperationOptions.Lowest when retrieving the bound nodes for a syntax node, so it gets the ILambdaExpression without the IConversionExpression. VB uses GetOperationOptions.Highest, so it gets the IConversionExpression. This is tracked by VB and C# implementation of SemanticModel.GetOperation uses different GetOperationOptions #17861, and is another example of why C# should probably be using Highest as well.

@AlekseyTs
Copy link
Contributor Author

and is another example of why C# should probably be using Highest as well.

I actually think that we shouldn't get conversion node for an implicit conversion, therefore should go for the lowest node. Conversion can be discovered from looking at the parent node.

@333fred
Copy link
Member

333fred commented May 25, 2017

Perhaps we should have an extension on ILambdaExpression for this? @jinujoseph, can we add this to next week's IOperation meeting?

@333fred 333fred modified the milestones: 15.6, 15.later May 25, 2017
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented May 25, 2017

Perhaps we should have an extension on ILambdaExpression for this?

For getting the conversion? That isn't lambda specific, that is how it should work for all expressions, I believe. So, I am not sure if any special helper is needed, it is easy enough to look at the parent.

@333fred
Copy link
Member

333fred commented May 25, 2017

I guess I'm just concerned that just getting the parent to find the the type of an expression doesn't feel like a particularly natural thing for me to do.

@CyrusNajmabadi
Copy link
Member

We need to investigate SemanticModel.GetTypeInfo for lambdas. See in C# what GetTypeINfo returns for both "TypeInfo.Type" and "TypeInfo.ConvertedType".

Our initial impression is that we probably want C# and VB to be the same (but htat might mean htat C# differs in IOperation from SemanticModel). We liked that lambdas were typeless as htat definitely matches VB's language interpretation, and we think it matches C#'s (though we need to confirm).

@CyrusNajmabadi
Copy link
Member

If users want the type of a lambda, they would walk up to the parent conversion node if it exists. (we may provide an extension for this).

@333fred
Copy link
Member

333fred commented Jun 5, 2017

I tested the following cases:

Declaration TypeInfo.Type TypeInfo.ConvertedType
Action a = () => { }; null NamedType System.Action
Dim a = Sub() End Sub Nothing Nothing
Dim a As Action = Sub() End Sub Nothing NamedType System.Action

Looks like we'll want to make C#'s version be consistent with VB, as that's what the SemanticModel does.

@333fred
Copy link
Member

333fred commented Jun 7, 2017

Fixed in e047f24.

@333fred 333fred closed this as completed Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants