Add IOperation support for tuple expressions #20749

mavasani · 2017-07-10T17:29:36Z

    public interface ITupleExpression : IOperation
    {
        /// <summary>
        /// Elements for tuple expression.
        /// </summary>
        ImmutableArray<IOperation> Elements { get; }
    }

Fixes dotnet#10856 ``` public interface ITupleExpression : IOperation { /// <summary> /// Elements for tuple expression. /// </summary> ImmutableArray<IOperation> Elements { get; } } ```

…e expression and parenting variable declaration so we test the operation tree for tuple conversions.

mavasani · 2017-07-10T19:42:28Z

This PR is revival of #19636, with following extra changes:

Added additional unit tests recommended by @AlekseyTs to test the IOperation tree binding for both the tuple expressions and its parent declaration so we test the tuple conversions.
There seem to be lot of redundant IConversionExpression nodes generated for tuple conversions. I have opened a separate bug to track analyzing and fixing these: Do not expose redundant implicit identity conversion on top of tuple expressions and foreach collection #20756 #Resolved

jcouv · 2017-07-11T18:09:06Z

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

+            bool isInvalid = boundTupleExpression.HasErrors;
+            SyntaxNode syntax = boundTupleExpression.Syntax;
+            ITypeSymbol type = boundTupleExpression.Type;
+            Optional<object> constantValue = ConvertToOptional(boundTupleExpression.ConstantValue);


Re-opening question from previous PR: tuples don't have constant values.
@CyrusNajmabadi prefered to keep as-is. But I don't see the benefit. #Resolved

I'm fine making them not having constant values.

I'd still remove this logic. Tuples don't have constant values.

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

Sure, I have changed this to be initialized to default(Optional<object>)

jcouv · 2017-07-11T18:14:35Z

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

+{
+    static void Main()
+    {
+        /*<bind>*/(int A, int B) t = (1, 2)/*</bind>*/;


All the tests involve literals (which get individually converted). I'd suggest also testing with typed expressions.

int a = 1; int b = 2; (long, long) t = (a, b);

I expect to see a tuple with type (int, int), which is converted to (long, long). I'm not sure how such tuple conversion is represented in IOperation. #Resolved

Sure, will add tests.

jcouv · 2017-07-11T18:17:59Z

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

+            ILiteralExpression (Text: 1) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1) (Syntax: '1')
+";
+            var expectedDiagnostics = new DiagnosticDescription[] {
+                // CS0266: Cannot implicitly convert type 'int' to 'uint'. An explicit conversion exists (are you missing a cast?)


I don't think the diagnostics are interesting here. Consider x = 0; y = 0; in Deconstruct. Or simply throw null;. #Resolved

Sure, done.

jcouv · 2017-07-11T18:21:05Z

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

+        /// <summary>
+        /// Elements for tuple expression.
+        /// </summary>
+        ImmutableArray<IOperation> Elements { get; }


This is a general comment beyond this PR, but I'm concerned about materializing more data structures to represent existing information. I would have expected IOperation to be much more "view-like". #Resolved

It wsa an early decision we consciously made. There are negatives to the materialization approach, but it puts us in a good space in terms of defining the tree and getting a good test infrastructure in place for it. We can then optimize it later as necessary as an implementation detail. #Resolved

jcouv · 2017-07-11T18:24:21Z

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

+            string expectedOperationTree = @"
+IOperation:  (OperationKind.None) (Syntax: '(uint x, ui ... Point(0, 1)')
+  Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.UInt32 x, System.UInt32 y)) (Syntax: '(uint x, uint y)')
+      Elements(2): ILocalReferenceExpression: x (OperationKind.LocalReferenceExpression, Type: System.UInt32) (Syntax: 'uint x')


Was there a discussion about how to represent declaration expressions in IOperation? I think the bound nodes have a flag to signify that those aren't just references to locals, but actually declaring locals. #Resolved

Filed #20798 to bring this up in the next design meeting.

jcouv · 2017-07-11T18:38:42Z

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

+  IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration) (Syntax: 'C t = (0, n ... *</bind>*/;')
+    Variables: Local_1: C t
+    Initializer: IConversionExpression (ConversionKind.OperatorMethod, Implicit) (OperatorMethod: C C.op_Implicit((System.Int32, System.String) x)) (OperationKind.ConversionExpression, Type: C) (Syntax: '(0, null)')
+        IConversionExpression (ConversionKind.CSharp, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32, System.String)) (Syntax: '(0, null)')


@VSadov, do we expect a conversion here?

jcouv · 2017-07-11T18:40:01Z

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

+        }
+
+        [Fact, WorkItem(10856, "https://github.com/dotnet/roslyn/issues/10856")]
+        public void TupleExpression_Deconstruction()


TupleExpression_Deconstruction [](start = 20, length = 30)

Consider adding a test for deconstruction-foreach: foreach (var (x, y) in new Point[] { new Point(0, 1) }) { ... } #Resolved

It seems like no IOperation is exposed for the deconstruction inside foreach, we just expose the IArrayCreationExpression on the right. I will file an issue to track this bug.

Filed #20889

jcouv · 2017-07-11T18:42:08Z

It would have been preferable to continue on the old PR, to maintain comment history.

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

mavasani · 2017-07-11T18:48:44Z

It would have been preferable to continue on the old PR, to maintain comment history.

Yes, but I had accidentally created my base branch for that PR on the dotnet upstream, instead of my own fork and GitHub didn't allow me to change the base fork on an existing PR.

cston · 2017-07-13T14:47:20Z

src/Compilers/Core/Portable/Generated/Operations.xml.Generated.cs

+    }
+
+    /// <summary>
+    /// Represents a C# try or a VB Try statement.


tuple rather than try.

cston · 2017-07-13T15:07:47Z

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

+        }
+
+        [Fact, WorkItem(10856, "https://github.com/dotnet/roslyn/issues/10856")]
+        public void TupleExpression_NamedElementsInTupleType_ParentVariableDeclaration()


Do we need separate tests for initializer and _ParentVariableDeclaration in each case? Would one _ParentVariableDeclaration test be sufficient for declarations?

This was explicitly requested by Aleksey in the original PR so that we test conversions.

cston · 2017-07-13T15:13:04Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_ITupleExpression.vb

+        End Sub
+
+        <Fact, WorkItem(10856, "https://github.com/dotnet/roslyn/issues/10856")>
+        Public Sub TupleExpression_ImplicitConversionFromNull_ParentVaraibleDeclaration()


mavasani · 2017-07-14T18:18:06Z

@jcouv @cston addressed your feedback

cston · 2017-07-14T18:20:43Z

LGTM

mavasani · 2017-07-17T16:08:46Z

Ping any further feedback @jcouv?

I am going to resolve the merge conflicts and fix all the added unit tests due to dumper test change.

jcouv

LGTM

mavasani added 3 commits May 18, 2017 15:05

Add IOperation support for tuple expressions

50fee94

Fixes dotnet#10856 ``` public interface ITupleExpression : IOperation { /// <summary> /// Elements for tuple expression. /// </summary> ImmutableArray<IOperation> Elements { get; } } ```

Merge remote-tracking branch 'upstream/features/ioperation' into Tuples

4764d10

Merge remote-tracking branch 'upstream/features/ioperation' into Tuples2

263954a

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

mavasani mentioned this pull request Jul 10, 2017

Add IOperation support for tuple expressions #19636

Closed

mavasani added 2 commits July 10, 2017 12:37

Address PR feedback from Aleksey and add unit tests for both the tupl…

b240fdd

…e expression and parenting variable declaration so we test the operation tree for tuple conversions.

Merge remote-tracking branch 'upstream/features/ioperation' into Tuples2

270b4f9

mavasani requested review from cston, jcouv and a team July 10, 2017 19:43

jcouv reviewed Jul 11, 2017

View reviewed changes

mavasani mentioned this pull request Jul 11, 2017

IOperation API for declaration expressions #20798

Closed

Pilchie added the Area-Analyzers label Jul 12, 2017

cston reviewed Jul 13, 2017

View reviewed changes

CyrusNajmabadi approved these changes Jul 13, 2017

View reviewed changes

Address PR feedback and add unit tests

fd44c29

mavasani added 2 commits July 14, 2017 11:25

Merge remote-tracking branch 'upstream/features/ioperation' into Tuples2

68057b1

Fix build break

20215df

mavasani added 2 commits July 17, 2017 10:48

Merge remote-tracking branch 'upstream/features/ioperation' into Tuples2

b74f065

Address feedback: tuples cannot have constant value

1d7fd67

jcouv approved these changes Jul 17, 2017

View reviewed changes

mavasani merged commit bd297ad into dotnet:features/ioperation Jul 17, 2017

mavasani deleted the Tuples2 branch July 17, 2017 19:04

mavasani mentioned this pull request Jul 17, 2017

Implement IOperation support for tuple literals #10856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IOperation support for tuple expressions #20749

Add IOperation support for tuple expressions #20749

mavasani commented Jul 10, 2017

mavasani commented Jul 10, 2017 •

edited by jcouv

Loading

jcouv Jul 11, 2017 •

edited

Loading

CyrusNajmabadi Jul 13, 2017 •

edited by jcouv

Loading

jcouv Jul 17, 2017

mavasani Jul 17, 2017

jcouv Jul 11, 2017 •

edited

Loading

mavasani Jul 11, 2017

jcouv Jul 11, 2017 •

edited

Loading

mavasani Jul 14, 2017

jcouv Jul 11, 2017 •

edited

Loading

CyrusNajmabadi Jul 13, 2017 •

edited by jcouv

Loading

jcouv Jul 11, 2017 •

edited

Loading

mavasani Jul 11, 2017

jcouv Jul 11, 2017

jcouv Jul 11, 2017 •

edited

Loading

mavasani Jul 14, 2017

mavasani Jul 14, 2017

jcouv commented Jul 11, 2017

mavasani commented Jul 11, 2017

cston Jul 13, 2017

cston Jul 13, 2017 •

edited

Loading

mavasani Jul 14, 2017

cston Jul 13, 2017

mavasani commented Jul 14, 2017

cston commented Jul 14, 2017

mavasani commented Jul 17, 2017

jcouv left a comment

Add IOperation support for tuple expressions #20749

Add IOperation support for tuple expressions #20749

Conversation

mavasani commented Jul 10, 2017

mavasani commented Jul 10, 2017 • edited by jcouv Loading

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jul 13, 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

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jul 13, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jul 11, 2017

mavasani commented Jul 11, 2017

Choose a reason for hiding this comment

cston Jul 13, 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 14, 2017

cston commented Jul 14, 2017

mavasani commented Jul 17, 2017

jcouv left a comment

Choose a reason for hiding this comment

mavasani commented Jul 10, 2017 •

edited by jcouv

Loading

jcouv Jul 11, 2017 •

edited

Loading

CyrusNajmabadi Jul 13, 2017 •

edited by jcouv

Loading

jcouv Jul 11, 2017 •

edited

Loading

jcouv Jul 11, 2017 •

edited

Loading

jcouv Jul 11, 2017 •

edited

Loading

CyrusNajmabadi Jul 13, 2017 •

edited by jcouv

Loading

jcouv Jul 11, 2017 •

edited

Loading

jcouv Jul 11, 2017 •

edited

Loading

cston Jul 13, 2017 •

edited

Loading