Deconstruction assignment: basic implementation #11384

jcouv · 2016-05-17T22:52:46Z

Here's a first basic implementation of deconstruction-assignment. I'm wondering if this should go into a feature branch instead as I only did limited IDE testing so far.

In terms of design:

The parsing was fixed and a test harness added for various potentially ambiguous scenarios with parentheses.
- The ScanTupleType method can now return a new ScanTypeFlags type. This only affects two callers: IsPossibleLocalDeclarationStatement (see code snippet at the bottom) and
The binding for assignment forks off when such a tuple literal is found on the left:
- The binding produces a new bound node (BoundDeconstructionAssignmentOperator). This bound node contains the list of variables on the left (bound and checked), the receiver for Deconstruct, the symbol for the Deconstruct method and information to complete the assignment of individual values (including conversions).
  -The Deconstruct method is resolved with restrictive rules at this point (we should find exactly one).
The lowering for this new bound node generates the following sequence:
- lower the variables on the left,
- get their side-effects and keep expressions to access them later without side-effect (like compound assignment does),
- lower the right-hand-side expression (that will be the receiver for Deconstruct below),
- prepare locals as out parameters,
- invoke Deconstruct with those out parameters,
- assign the values from those out parameters back into their proper left-hand-side variable (including conversions).
There is also some small change to the data flow logic, to ensure that variables on the left count as being assigned.

Vlad, the initial approach that we discussed for binding was very difficult (significant refactoring on tricky code). So the bound node holds onto the assignment expressions for each variable, as opposed to a Conversion object. Not only was this much simpler and natural, I think this will actually help later (when dealing with nesting case).

@dotnet/roslyn-compiler for review.

From IsPossibleLocalDeclarationStatement:

                ScanTypeFlags st = this.ScanType();

                // [comment removed]
                if (st == ScanTypeFlags.MustBeType && this.CurrentToken.Kind != SyntaxKind.DotToken)
                {
                    return true;
                }

ScanType used to return MustBeType or NotType, but we need to care about "possibly-tuple-type-but-might-also-be-tuple-expression" (the new TupleType option).

cston · 2016-05-17T23:53:58Z

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

+
+            // bind the variables and check they can be assigned to
+            var checkedVariablesBuilder = ArrayBuilder<BoundExpression>.GetInstance(arguments.Count);
+            for (int i = 0, l = numElements; i < l; i++)


I can't. I need the index i below.

It looks like the index is only used to index into arguments though. i and l seem unnecessary.

Ah, yes. Sorry about that.

cston · 2016-05-18T05:36:18Z

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

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


ValidStatement?

gafter · 2016-05-18T19:16:52Z

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

+            if (numElements < 2)
+            {
+                // this should be a parse error already.
+                BoundExpression[] args = numElements == 1 ?


Actually, it should have parsed as a parenthesized expression, not a tuple expression. I think you can assert that this does not occur.

gafter · 2016-05-18T19:36:25Z

If any of the element types "must be a type", then the whole enclosing thing must be a type. That should be reflected in the return value of this function.

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:6289 in 86999a5. [](commit_id = 86999a5, deletion_comment = False)

VSadov · 2016-05-18T19:54:20Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

+
+    <Field Name="LeftVariables" Type="ImmutableArray&lt;BoundExpression&gt;"/>
+
+    <Field Name="DeconstructReceiver" Type="BoundExpression"/>


I'd just call it "Right"

And you actually call it loweredRight after lowering

AlekseyTs · 2016-05-19T04:47:37Z

docs/features/deconstruction.md

+Treat deconstruction of a tuple into existing variables as a kind of assignment, using the existing AssignmentExpression.
+
+
+###Deconstruction-assignment (deconstruction into into existing variables):


"into into"

AlekseyTs · 2016-05-19T16:54:15Z

AlekseyTs · 2016-05-19T16:54:38Z

LGTM

jcouv · 2016-05-19T16:55:59Z

Thanks all for the review and feedback. I'll merge shortly.

VSadov · 2016-05-19T17:23:00Z

LGTM.

We need to figure what nesting on the left means and what is the type of the whole expression.

gafter · 2016-05-19T17:45:59Z

docs/features/deconstruction.md

+    ;
+
+join_clause // not sure
+    : 'join' type? identifier 'in' expression 'on' expression 'equals' expression


This is an existing production, no? Are you intending to add an alternative with local_variable_combo_declaration_lhs? Here and below two productions.

The list above resulted from my fishing for syntax nodes that look like they declare variables and so would be candidates for deconstruction-declaration.
For those marked "not sure", I wasn't sure whether they are good candidates and what the updated syntax would be.

jcouv added 2 commits May 16, 2016 16:40

Deconstruction assignment: basic implementation

65dad7b

Working on parsing

d824baf

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

jcouv self-assigned this May 17, 2016

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

cston reviewed May 17, 2016
View reviewed changes

Deconstruction-assignment: PR feedback (1)

86999a5

cston reviewed May 18, 2016
View reviewed changes

Deconstruction-assignment: PR feedback (1)

d935dcb

gafter reviewed May 18, 2016
View reviewed changes

VSadov reviewed May 18, 2016
View reviewed changes

Deconstruction-assignment: PR feedback (3)

b3cf56f

jcouv force-pushed the tuple-deconstruction branch from 5438da3 to b3cf56f Compare May 19, 2016 03:45

AlekseyTs reviewed May 19, 2016
View reviewed changes

jcouv mentioned this pull request May 19, 2016

[Umbrella] Deconstruction work items #11299

Closed

Deconstruction-assignment: PR feedback (4)

85e1f4d

jcouv force-pushed the tuple-deconstruction branch from 846ef81 to 85e1f4d Compare May 19, 2016 16:53

gafter reviewed May 19, 2016
View reviewed changes

jcouv merged commit 6e8e079 into dotnet:features/tuples May 19, 2016

jcouv mentioned this pull request Sep 15, 2016

Deconstruction: Properties on the left may not do proper flow analysis #13827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deconstruction assignment: basic implementation #11384

Deconstruction assignment: basic implementation #11384

jcouv commented May 17, 2016

cston May 17, 2016

jcouv May 18, 2016

cston May 18, 2016

jcouv May 18, 2016

cston May 18, 2016

jcouv May 18, 2016

gafter May 18, 2016

jcouv May 18, 2016

gafter commented May 18, 2016

VSadov May 18, 2016

VSadov May 18, 2016

jcouv May 18, 2016

AlekseyTs May 19, 2016

jcouv May 19, 2016

AlekseyTs commented May 19, 2016

AlekseyTs commented May 19, 2016

jcouv commented May 19, 2016

VSadov commented May 19, 2016

gafter May 19, 2016

jcouv May 19, 2016


		<Field Name="LeftVariables" Type="ImmutableArray<BoundExpression>"/>

		<Field Name="DeconstructReceiver" Type="BoundExpression"/>

		Treat deconstruction of a tuple into existing variables as a kind of assignment, using the existing AssignmentExpression.


		###Deconstruction-assignment (deconstruction into into existing variables):

Deconstruction assignment: basic implementation #11384

Deconstruction assignment: basic implementation #11384

Conversation

jcouv commented May 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 19, 2016

AlekseyTs commented May 19, 2016

jcouv commented May 19, 2016

VSadov commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment