Deconstruction-assignment: nested scenario #11715

jcouv · 2016-06-02T16:48:15Z

Adding support for nested deconstruction-assignments, such as (x, (y, z)) = M(); or (x, (y, z)) = (1, (2, 3));.

The bound node now contains:

all the LHS variables (flat list),
the expression on the right (used as target for deconstruction),
a list of deconstruction steps (which has a deconstruction method and placeholders for input and outputs, with the method being null in tuple cases),
and a list of assignment steps.

Note conversions are still coupled with assignments, but I have a work item (in #11299) to do all conversions first then all assignments.

CC @dotnet/roslyn-compiler for review.

cston · 2016-06-02T17:51:41Z

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

+                        Error(diagnostics, ErrorCode.ERR_DeconstructRequiresExpression, node);
+                        return BadExpression(node, checkedVariables.Concat(boundRHS).ToArray());
+                    }
+                }


Can this if be moved before try/finally and deconstructionSteps declaration?

Yes, indeed. Thanks

cston · 2016-06-02T21:34:01Z

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

+    <Field Name="AssignmentSteps" Type="ImmutableArray&lt;BoundDeconstructionAssignmentOperator.AssignmentStep&gt;" Null="NotApplicable" SkipInVisitor="true"/>
+  </Node>
+
+  <Node Name="BoundDeconstructionVariables" Base="BoundExpression">


Why a BoundExpression rather than BoundNode?

So, this bound node is never output from binding at all. But during binding, I have bound expressions for the LHS, which need to be organized in a tree.
Because all simple variables are BoundExpression, it is convenient that this is also BoundExpression (rather than BoundNode). But I'm open to suggestions.
I agree that having a bound node that never escapes binding is strange.

Where do we use checkedVariables as (array of) BoundExpression rather than BoundNode? Or is it more than just checkedVariables that is the concern?

cston · 2016-06-03T18:25:20Z

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

-            DiagnosticBag bag = new DiagnosticBag();
-            MethodSymbol deconstructMethod = FindDeconstruct(checkedVariables, boundRHS, node, bag);
+            var bag = DiagnosticBag.GetInstance();
+            MethodSymbol deconstructMethod = FindDeconstruct(variables.Length, targetPlaceholder, syntax, bag);
            if (!diagnostics.HasAnyErrors())


Is bag only added if diagnostics is empty to avoid cascading errors? Are we sure existing diagnostics are from the same expression (in other words, are we sure bag contains errors related to those in diagnostics)? Perhaps add bag always.

AlekseyTs · 2016-06-03T22:11:27Z

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

+        /// </summary>
+        static private DeconstructStep MakeNonTupleDeconstructStep(
+                        BoundDeconstructValuePlaceholder targetPlaceholder,
+                        AssignmentExpressionSyntax syntax,


Same comment about associated syntax node.

AlekseyTs · 2016-06-03T23:04:08Z

Done with the review pass

…error

jcouv · 2016-06-04T03:16:23Z

Pushed some commits addressing the feedback.
I'm on the fence about removing BoundDeconstructionVariables as it would make it easier to track portions of syntax on the LHS.

AlekseyTs · 2016-06-04T04:52:46Z

Having BoundDeconstructionVariables as a helper node seems fine as long as it doesn't survive initial binding. It does provide an easy way to track relevant syntax node. Otherwise we would have to use a tuple for this.

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

cston · 2016-06-04T05:41:57Z

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

+        private class DeconstructionVariable
+        {
+            public BoundExpression Single;
+            public ImmutableArray<DeconstructionVariable>? Nested;


Consider using non-nullable ImmutableArray<DeconstructionVariable> where the unset value is default(Immutable<DeconstructionVariable>).

There is a type loader bug that would cause a struct S1 containing ImmutableArray< S1 > be rejected as having a layout cycle (there is not real cycle though since IA wraps a ref array)
CLR knows about this bug, but it is too risky area to fix right now.

jcouv · 2016-06-06T19:17:55Z

@AlekseyTs @cston I pushed a change that uses default instead of a nullable and avoids IEnumerable.
The tracking of more precise locations is deferred to a follow-up work item.

jcouv · 2016-06-06T21:13:06Z

@AlekseyTs @cston Please let me know if ok to merge. Thanks

cston · 2016-06-06T21:23:55Z

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

-            TypeSymbol lhsAsTuple = TupleTypeSymbol.Create(locationOpt: null, elementTypes: lhsTypes, elementLocations: default(ImmutableArray<Location>), elementNames: default(ImmutableArray<string>), compilation: Compilation, diagnostics: diagnostics);
-            var typedRHS = GenerateConversionForAssignment(lhsAsTuple, boundRHS, diagnostics);
+            public BoundExpression Single;
+            public ImmutableArray<DeconstructionVariable> Nested;


readonly for both fields.

cston · 2016-06-06T21:34:50Z

LGTM

AlekseyTs · 2016-06-07T06:05:16Z

LGTM, modulo the fact that lhsTemps are still not renamed to lhsTargets in LocalRewriter.

jcouv · 2016-06-07T19:02:13Z

Merged. Thanks for the review and feedback.

Deconstruction-assignment: nested scenario

c71be15

jcouv added Area-Compilers Feature - Tuples Tuples labels Jun 2, 2016

jcouv self-assigned this Jun 2, 2016

dnfclas added the cla-already-signed label Jun 2, 2016

jcouv mentioned this pull request Jun 2, 2016

Deconstruction-assignment: expression type should be void #11594

Merged

jcouv added 2 commits June 2, 2016 10:08

Flattening variables in bound node

86ca7d3

Fixing warning with OneOrMany

dd3b674

cston reviewed Jun 2, 2016
View reviewed changes

PR feedback (1)

199c6ad

cston reviewed Jun 2, 2016
View reviewed changes

Adding error for tuple cardinality mismatch

461b419

jcouv force-pushed the nesting branch from 2117ae7 to 461b419 Compare June 3, 2016 16:23

cston reviewed Jun 3, 2016
View reviewed changes

AlekseyTs reviewed Jun 3, 2016
View reviewed changes

jcouv added 3 commits June 3, 2016 18:13

PR feedback (3)

6f354a1

Don't use a bound node for nested deconstruction variables

f205d5f

Capture as many deconstruction and assignment steps as possible when …

ba975f7

…error

jcouv mentioned this pull request Jun 4, 2016

[Umbrella] Deconstruction work items #11299

Closed

Adding PROTOTYPE markers

1748df1

cston reviewed Jun 4, 2016
View reviewed changes

PR feedback (4)

4e531c6

cston reviewed Jun 6, 2016
View reviewed changes

PR feedback (5)

87ee4f4

jcouv force-pushed the nesting branch from a523920 to 87ee4f4 Compare June 7, 2016 17:13

jcouv merged commit 326f70b into dotnet:features/tuples Jun 7, 2016

jcouv deleted the nesting branch June 7, 2016 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deconstruction-assignment: nested scenario #11715

Deconstruction-assignment: nested scenario #11715

jcouv commented Jun 2, 2016 •

edited

Loading

cston Jun 2, 2016

jcouv Jun 2, 2016

cston Jun 2, 2016

jcouv Jun 2, 2016

cston Jun 3, 2016

cston Jun 3, 2016

AlekseyTs Jun 3, 2016

AlekseyTs commented Jun 3, 2016

jcouv commented Jun 4, 2016

AlekseyTs commented Jun 4, 2016

cston Jun 4, 2016

VSadov Jun 6, 2016 •

edited

Loading

jcouv Jun 6, 2016

jcouv commented Jun 6, 2016

jcouv commented Jun 6, 2016

cston Jun 6, 2016

jcouv Jun 6, 2016

cston commented Jun 6, 2016

AlekseyTs commented Jun 7, 2016 •

edited

Loading

jcouv commented Jun 7, 2016

Deconstruction-assignment: nested scenario #11715

Deconstruction-assignment: nested scenario #11715

Conversation

jcouv commented Jun 2, 2016 • edited Loading

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

AlekseyTs commented Jun 3, 2016

jcouv commented Jun 4, 2016

AlekseyTs commented Jun 4, 2016

Choose a reason for hiding this comment

VSadov Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 6, 2016

jcouv commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Jun 6, 2016

AlekseyTs commented Jun 7, 2016 • edited Loading

jcouv commented Jun 7, 2016

jcouv commented Jun 2, 2016 •

edited

Loading

VSadov Jun 6, 2016 •

edited

Loading

AlekseyTs commented Jun 7, 2016 •

edited

Loading