Deconstruction-assignment for tuples by jcouv · Pull Request #11457 · dotnet/roslyn

jcouv · 2016-05-20T17:03:25Z

This PR mainly adds deconstruction-assignment with the right-hand-side being a tuple type or a tuple literal (with or without type).

This also adds tests for follow-ups from previous feedback:

ref returning methods on the left. Check sideeffects.
Foo()[Bar()] on the left, where all Foo, Bar and the indexer have sideeffects - print something
check IL in simple/common cases
Optimize the case where LHS variable is simple ref value

@dotnet/roslyn-compiler for review.

cston · 2016-05-20T17:12:40Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

            var checkedVariables = checkedVariablesBuilder.ToImmutableAndFree();

+            // tuple literal such as `(1, 2)`, `(null, null)`, `(x.P, y.M())`
+            if (boundRHS.Kind == BoundKind.TupleLiteral || ((object)boundRHS.Type != null && boundRHS.Type.IsTupleType))


Is the first condition necessary or would a literal also satisfy the second condition?

The reason for the first condition is tuple literals that don't have a natural type, such as (null, null).

cston · 2016-05-20T17:26:01Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+            if (boundRHS.Type == null)
+            {
+                Error(diagnostics, ErrorCode.ERR_DeconstructRequiresExpression, node);
+                return BadExpression(node, new[] { boundRHS });


Consider using overload BadExpression(CSharpSyntaxNode, BoundNode).

Thanks. Done

jcouv · 2016-05-20T19:15:37Z

I just pushed a change to address Chuck's feedback so far.

cston · 2016-05-20T20:20:47Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+        }
+
+        [Fact]
+        public void DeconstructIL()


Is this a subset of the Deconstruct test above?

Yes, it is now that I added VerifyIL to the test above. I'll remove the redundant one.

cston · 2016-05-20T20:26:35Z

LGTM

AlekseyTs · 2016-05-23T05:44:15Z

...ers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs

+
+        private ImmutableArray<BoundExpression> AccessTupleFields(BoundDeconstructionAssignmentOperator node, BoundExpression loweredRight, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores)
+        {
+            var tupleType = loweredRight.Type.IsTupleType ? loweredRight.Type : TupleTypeSymbol.Create((NamedTypeSymbol)loweredRight.Type);


It looks like this function is never called when IsTupleType is false. Consider simply asserting this instead.

Yes. This is un-necessary now that the conversion is applied to all tuple cases. Thanks

Turns out I was wrong. Tests won't pass without this logic. The key is that the node on the right may have a tuple type before lowering, but after lowering it becomes a tuple constructor call (which returns underlying type).

AlekseyTs · 2016-05-23T06:02:40Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+                //         (x, y) = (1, 2);
+                Diagnostic(ErrorCode.ERR_NoImplicitConv, "(1, 2)").WithArguments("(int, int)", "(byte, string)").WithLocation(9, 18)
+                );
+        }


Consider adding deconstruction tests for tuples on the right that have names.

Added test with names.

Add tests where RHS is used on the left:

(object i, object ii) x = (1,2); object y; (x.ii, y) = x;

should result in x: {1, 1} , y: 2

I'll add the one you suggested. Note that I already have the one Mads suggested (x, y) = (y, x);.

Also try a variant of above:

(object i, object ii) x = (1,2); object y; ref var a = ref x; (a.ii, y) = x;

should result in x: {1, 1} , y: 2

these are some cases where you need a temp for the RHS even when it is a local/parameter.

gafter · 2016-05-23T17:25:11Z

tempZ = &evaluate expressionZ

I believe the LDM said that we want to evaluate all side-effects on the left before side-effects on this right.

Refers to: docs/features/deconstruction.md:69 in 7170bd2. [](commit_id = 7170bd2, deletion_comment = False)

AlekseyTs · 2016-05-23T20:53:10Z

src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs


+        /// <summary>
+        /// If this is a field of a tuple type,
+        /// id is an index of the element (zero-based).


Probably shouldn't mention Id

Also, should probably say "If this is a field representing a tuple element", rather than "If this is a field of a tuple type", because some fields of a tuple type return negative numbers.

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

jcouv · 2016-05-23T23:48:57Z

@AlekseyTs Pushed the latest change with doc tweaks and improving the logic to collect fields from tuple symbol.
Calling ArrayBuilder.SetItem on last position seems the best way to force it to size.

AlekseyTs · 2016-05-24T04:03:24Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

+
+        private ImmutableArray<FieldSymbol> CollectTupleElementFields()
+        {
+            var builder = ArrayBuilder<FieldSymbol>.GetInstance(_elementTypes.Length);


This is subtle, I think you can simply pass null as the second parameter for ArrayBuilder.GetInstance to have the builder allocate the items.

Ah! That's much nicer. Thanks

AlekseyTs · 2016-05-24T04:10:49Z

LGTM, but do consider my last two comments.

gafter · 2016-05-24T17:20:18Z

This implementation fails to compile this test:

namespace System
{
    public struct ValueTuple<T1, T2>
    {
        public T1 Item1;
        public T2 Item2;

        public override string ToString()
        {
            T1 k;
            T2 v;
            // error: no Destruct instance or extension method was found for ValueTuple<T1, T2>
            (k, v) = this;
            return $"({k}, {v})";
        }
    }
}

VSadov · 2016-05-24T17:28:07Z

@gafter the example that you gave is essentially the same as

ValueTuple<T1, T2> t = this;
(k, v) = t;

I wonder if that has similar problems.

Overall, it would seem acceptable for tuple types to behave differently inside their own definition types, since that scenario is diminishingly small, but I see no reasons why it would be the case here.

gafter · 2016-05-24T17:39:21Z

@VSadov It isn't the same, in the current implementation, because inside the body of ValueTuple'2, the type ValueTuple<T1, T2> is a tuple type and the type of this isn't a tuple type. They are not the same type!

AlekseyTs · 2016-05-24T18:15:55Z

There is a hole in the way we assign the type for this parameter symbol. I think we should tackle it separately because its not exclusively related to deconstruction. It does affect deconstruction though.

VSadov · 2016-05-24T18:16:01Z

@gafter interesting.
It seems not a deconstruction issue though, but general appearance of "this" inside the VT definition. We should proceed with this PR and enter a bug to figure what to do with "this".

jcouv · 2016-05-24T18:17:17Z

please test all

VSadov · 2016-05-24T18:18:33Z

We can make "this" to be a tuple inside VT definition, or we can make an exception for that case - whatever.
After all definition of Int32 contains itself and not that many people care :-)

jcouv · 2016-05-24T18:19:58Z

@gafter @VSadov Interesting case. It's possible this behavior (not treating "this" inside ValueTuple as a tuple type) actually helps us.
Without this behavior, we could have a circularity problem in trying to build the ValueTuple library with new compiler..

AlekseyTs · 2016-05-24T18:21:15Z

I don't think we would face a circularity problem the check for tuple-compatible type is "local".

gafter · 2016-05-24T18:56:59Z

See also #11530 for another symptom of the same issue.

jaredpar · 2016-05-24T20:50:04Z

test open please

Julien Couvreur added 4 commits May 19, 2016 13:30

Deconstruction-assignment: adding more tests

6967473

Working on tuple deconstruction

dcb9c69

Fixing typeless tuple case

a2c290b

Apply conversion on all tuples, not just typeless ones

4f443f0

jcouv added Area-Compilers Feature - Tuples Tuples labels May 20, 2016

jcouv self-assigned this May 20, 2016

dnfclas added the cla-already-signed label May 20, 2016

cston reviewed May 20, 2016
View reviewed changes

Updating doc

e046522

cston reviewed May 20, 2016
View reviewed changes

PR feedback (1)

7170bd2

cston reviewed May 20, 2016
View reviewed changes

AlekseyTs reviewed May 23, 2016
View reviewed changes

jcouv mentioned this pull request May 23, 2016

[Umbrella] Deconstruction work items #11299

Closed

AlekseyTs reviewed May 23, 2016
View reviewed changes

jcouv mentioned this pull request May 23, 2016

[Umbrella] Tuples Work Items #10945

Closed

AlekseyTs reviewed May 23, 2016
View reviewed changes

jcouv force-pushed the tuple-deconstruction branch from f49cbfe to a5e4fc6 Compare May 23, 2016 23:48

PR feedback (3)

66662a3

jcouv force-pushed the tuple-deconstruction branch from a5e4fc6 to 66662a3 Compare May 24, 2016 02:06

AlekseyTs reviewed May 24, 2016
View reviewed changes

PR feedback (4)

c7b60a4

This was referenced May 24, 2016

Tuple implementation rejects tuple d-assignment from this #11533

Open

Tuple implementation rejects implicit operators #11530

Closed

jcouv merged commit 6cb6df1 into dotnet:features/tuples May 24, 2016

jcouv deleted the tuple-deconstruction branch May 24, 2016 22:00

Conversation

jcouv commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 24, 2016

Uh oh!

gafter commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented May 24, 2016

Uh oh!

gafter commented May 24, 2016

Uh oh!

AlekseyTs commented May 24, 2016

Uh oh!

VSadov commented May 24, 2016

Uh oh!

jcouv commented May 24, 2016

Uh oh!

VSadov commented May 24, 2016

Uh oh!

jcouv commented May 24, 2016

Uh oh!

AlekseyTs commented May 24, 2016

Uh oh!

gafter commented May 24, 2016

Uh oh!

jaredpar commented May 24, 2016

Uh oh!

Reviewers

Assignees

VSadov May 23, 2016 •

edited

Loading

gafter commented May 24, 2016 •

edited

Loading