Deconstruction-assignment for tuples (#11457)

docs/features/deconstruction.md

@@ -30,48 +30,61 @@ Treat deconstruction of a tuple into existing variables as a kind of assignment,
 
 
 ###Deconstruction-assignment (deconstruction into existing variables):
+
 This doesn't introduce any changes to the language grammar. We have an `assignment-expression` (also simply called `assignment` in the C# grammar) where the `unary-expression` (the left-hand-side) is a `tuple-literal`.
-In short, what this does is find a `Deconstruct` method on the expression on the right-hand-side of the assignment, invoke it, collect its `out` parameters and assign them to the variables on the left-hand-side.
+In short, what this does in the general case is find a `Deconstruct` method on the expression on the right-hand-side of the assignment, invoke it, collect its `out` parameters and assign them to the variables on the left-hand-side. And in the special case where the expression on the right-hand-side is a tuple (tuple literal or tuple type), then the elements of the tuple can be assigned to the variables on the left-hand-side without needing to call `Deconstruct`.
 
 The existing assignment binding currently checks if the variable on its left-hand-side can be assigned to and if the two sides are compatible.
 It will be updated to support deconstruction-assignment, ie. when the left-hand-side is a tuple-literal/tuple-expression:
 
-- Each item on the left needs to be assignable and needs to be compatible with corresponding position on the right
+- Needs to break the right-hand-side into items. That step is un-necessary if the right-hand-side is already a tuple though.
+- Each item on the left needs to be assignable and needs to be compatible with corresponding position on the right (resulting from previous step).
 - Needs to handle nesting case such as `(x, (y, z)) = M();`, but note that the second item in the top-level group has no discernable type.
 
-The lowering for deconstruction-assignment would translate: `(expressionX, expressionY, expressionZ) = (expressionA, expressionB, expressionC)` into:
+The evaluation order can be summarized as: (1) all the side-effects on the left-hand-side, (2) all the Deconstruct invocations (if not tuple), (3) conversions (if needed), and (4) assignments.
+
+In the general case, the lowering for deconstruction-assignment would translate: `(expressionX, expressionY, expressionZ) = expressionRight` into:
 
 ```
+// do LHS side-effects
 tempX = &evaluate expressionX
 tempY = &evaluate expressionY
 tempZ = &evaluate expressionZ
 
-tempRight = evaluate right and evaluate Deconstruct
+// do Deconstruct
+evaluate right and evaluate Deconstruct in three parts (tempA, tempB and tempC)
 
-tempX = tempRight.A (including conversions)
-tempY = tempRight.B (including conversions)
-tempZ = tempRight.C (including conversions)
+// do conversions
+tempConvA = convert tempA
+tempConvB = convert tempB
+tempConvC = convert tempC
 
-“return/continue” with newTupleIncludingNames tempRight (so you can do get Item1 from the assignment)?
+// do assignments
+tempX = tempConvA
+tempY = tempConvB
+tempZ = tempConvC
 ```
 
 The evaluation order for nesting `(x, (y, z))` is:
 ```
+// do LHS side-effects
 tempX = &evaluate expressionX
-
-tempRight = evaluate right and evaluate Deconstruct
-
-tempX = tempRight.A (including conversions)
-tempLNested = tempRight.B (no conversions)
-
 tempY = &evaluate expressionY
 tempZ = &evaluate expressionZ
 
-tempRNest = evaluate Deconstruct on tempRight
+// do Deconstruct
+evaluate right and evaluate Deconstruct into two parts (tempA and tempNested)
+evaluate Deconstruct on tempNested intwo two parts (tempB and tempC)
 
-tempY = tempRNest.B (including conversions)
-tempZ = tempRNest.C (including conversions)
+// do conversions
+tempConvA = convert tempA
+tempConvB = convert tempB
+tempConvC = convert tempC
 
+// do assignments
+tempX = tempConvA
+tempY = tempConvB
+tempZ = tempConvC
 ```
 
 The evaluation order for the simplest cases (locals, fields, array indexers, or anything returning ref) without needing conversion:
@@ -80,23 +93,15 @@ evaluate side-effect on the left-hand-side variables
 evaluate Deconstruct passing the references directly in
 ```
 
-Note that the feature is built around the `Deconstruct` mechanism for deconstructing types.
-`ValueTuple` and `System.Tuple` will rely on that same mechanism, except that the compiler may need to synthesize the proper `Deconstruct` methods.
-
+In the case where the expression on the right is a tuple, the evaluation order becomes:
+```
+evaluate side-effect on the left-hand-side variables
+evaluate the right-hand-side and do a tuple conversion (using a fake tuple representing the types in the left-hand-side)
+assign element-wise from the right to the left
+```
 
-**Work items, open issues and assumptions to confirm with LDM:**
+Note that tuples (`System.ValueTuple` and `System.Tuple`) don't need to invoke Deconstruct.
 
-- I assume this should work even if `System.ValueTuple` is not present.
-- How is the Deconstruct method resolved?
-    - I assumed there can be no ambiguity. Only one `Deconstruct` is allowed (in nesting cases we have no type to guide the resolution process).
-    - But we may allow a little bit of ambiguity and preferring an instance over extension method.
-- Do the names matter? `int x, y; (a: x, b: y) = M();`
-- Can we deconstruct into a single out variable? I assume no.
-- I assume no compound assignment `(x, y) += M();`
-- [ ] Provide more details on the semantic of deconstruction-assignment, both static (The LHS of the an assignment-expression used be a L-value, but now it can be L-value -- which uses existing rules -- or tuple_literal. The new rules for tuple_literal on the LHS...) and dynamic.
-- [ ] Discuss with Aleksey about "Deconstruct and flow analysis for nullable ref type"
-- [ ] Validate any target typing or type inference scenarios.
-- The deconstruction-assignment is treated separately from deconstruction-declaration, which means it doesn't allow combinations such as `int x; (x, int y) = M();`.
 
 ###Deconstruction-declaration (deconstruction into new variables):
 
@@ -158,20 +163,11 @@ constant_declarator // not sure
     ;
 ```
 
-Treat deconstruction of a tuple into new variables as a new kind of node (AssignmentExpression).
+Treat deconstruction of a tuple into new variables as a new kind of node (not AssignmentExpression).
 It would pick up the behavior of each contexts where new variables can be declared (TODO: need to list). For instance, in LINQ, new variables go into a transparent identifiers.
 It is seen as deconstructing into separate variables (we don't introduce transparent identifiers in contexts where they didn't exist previously).
 
-Should we allow this?
-`var t = (x: 1, y: 2);    (x: var a, y: var b) = t;`
-or `var (x: a, y: b) = t;`
-(if not, tuple names aren't very useful?)
-
-- [ ] Add example: var (x, y) =
-- [ ] Semantic (cardinality should match, ordering including conversion,
-- [ ] What are the type rules? `(string s, int x) = (null, 3);`
-
-- Deconstruction for `System.ValueTuple`, `System.Tuple` and any other type involves a call to `Deconstruct`.
+Just like deconstruction-assignment, deconstruction-declaration does not need to invoke `Deconstruct` for tuple types.
 
 **References**
 

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

@@ -1715,6 +1715,13 @@ private BoundExpression BindAssignment(AssignmentExpressionSyntax node, Diagnost
             return BindAssignment(node, op1, op2, diagnostics);
         }
 
+        /// <summary>
+        /// There are two kinds of deconstruction-assignments which this binding handles: tuple and non-tuple.
+        ///
+        /// Returns a BoundDeconstructionAssignmentOperator
+        ///     - with all the fields populated except deconstructMember, for the tuple case
+        ///     - with all the fields populated, for a non-tuple case
+        /// </summary>
         private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
         {
             SeparatedSyntaxList<ArgumentSyntax> arguments = ((TupleExpressionSyntax)node.Left).Arguments;
@@ -1727,16 +1734,50 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
 
             // bind the variables and check they can be assigned to
             var checkedVariablesBuilder = ArrayBuilder<BoundExpression>.GetInstance(numElements);
-            for (int i = 0; i < numElements; i++)
+            foreach (var argument in arguments)
             {
-                var boundVariable = BindExpression(arguments[i].Expression, diagnostics, invoked: false, indexed: false);
+                var boundVariable = BindExpression(argument.Expression, diagnostics, invoked: false, indexed: false);
                 var checkedVariable = CheckValue(boundVariable, BindValueKind.Assignment, diagnostics);
 
                 checkedVariablesBuilder.Add(checkedVariable);
             }
 
             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))
+            {
+                return BindDeconstructWithTuple(node, diagnostics, boundRHS, checkedVariables);
+            }
+
+            // expression without type such as `null`
+            if (boundRHS.Type == null)
+            {
+                Error(diagnostics, ErrorCode.ERR_DeconstructRequiresExpression, node);
+                return BadExpression(node, checkedVariables.Concat(boundRHS).ToArray());
+            }
+
+            return BindDeconstructWithDeconstruct(node, diagnostics, boundRHS, checkedVariables);
+        }
+
+        private BoundExpression BindDeconstructWithTuple(AssignmentExpressionSyntax node, DiagnosticBag diagnostics, BoundExpression boundRHS, ImmutableArray<BoundExpression> checkedVariables)
+        {
+            // make a conversion for the tuple based on the LHS information (this also takes care of tuple literals without a natural type)
+            var lhsTypes = checkedVariables.SelectAsArray(v => v.Type);
+            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);
+
+            ImmutableArray<TypeSymbol> tupleTypes = typedRHS.Type.TupleElementTypes;
+
+            // figure out the pairwise conversions
+            var assignments = checkedVariables.SelectAsArray((variable, index, types) => MakeAssignmentInfo(variable, types[index], node, diagnostics), tupleTypes);
+
+            TypeSymbol lastType = tupleTypes.Last();
+            return new BoundDeconstructionAssignmentOperator(node, checkedVariables, typedRHS, deconstructMemberOpt: null, assignments: assignments, type: lastType);
+        }
+
+        private BoundExpression BindDeconstructWithDeconstruct(AssignmentExpressionSyntax node, DiagnosticBag diagnostics, BoundExpression boundRHS, ImmutableArray<BoundExpression> checkedVariables)
+        {
             // symbol and parameters for Deconstruct
             DiagnosticBag bag = new DiagnosticBag();
             MethodSymbol deconstructMethod = FindDeconstruct(checkedVariables, boundRHS, node, bag);
@@ -1754,24 +1795,27 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
             }
 
             // figure out the pairwise conversions
-            var assignmentsBuilder = ArrayBuilder<BoundDeconstructionAssignmentOperator.AssignmentInfo>.GetInstance(numElements);
             var deconstructParameters = deconstructMethod.Parameters;
-            for (int i = 0; i < checkedVariables.Length; i++)
-            {
-                var leftPlaceholder = new BoundLValuePlaceholder(checkedVariables[i].Syntax, checkedVariables[i].Type) { WasCompilerGenerated = true };
-                var rightPlaceholder = new BoundRValuePlaceholder(node.Right, deconstructParameters[i].Type) { WasCompilerGenerated = true };
-
-                // each assignment has a placeholder for a receiver and another for the source
-                BoundAssignmentOperator op = BindAssignment(node, leftPlaceholder, rightPlaceholder, diagnostics);
-                assignmentsBuilder.Add(new BoundDeconstructionAssignmentOperator.AssignmentInfo() { Assignment = op, LValuePlaceholder = leftPlaceholder, RValuePlaceholder = rightPlaceholder });
-            }
-
-            var assignments = assignmentsBuilder.ToImmutableAndFree();
+            var assignments = checkedVariables.SelectAsArray((variable, index, parameters) => MakeAssignmentInfo(variable, parameters[index].Type, node, diagnostics), deconstructParameters);
 
             TypeSymbol lastType = deconstructParameters.Last().Type;
             return new BoundDeconstructionAssignmentOperator(node, checkedVariables, boundRHS, deconstructMethod, assignments, lastType);
         }
 
+        /// <summary>
+        /// Figures out how to assign from sourceType into receivingVariable and bundles the information (leaving holes for the actual source and receiver) into an AssignmentInfo.
+        /// </summary>
+        private BoundDeconstructionAssignmentOperator.AssignmentInfo MakeAssignmentInfo(BoundExpression receivingVariable, TypeSymbol sourceType, AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
+        {
+            var leftPlaceholder = new BoundLValuePlaceholder(receivingVariable.Syntax, receivingVariable.Type) { WasCompilerGenerated = true };
+            var rightPlaceholder = new BoundRValuePlaceholder(node.Right, sourceType) { WasCompilerGenerated = true };
+
+            // each assignment has a placeholder for a receiver and another for the source
+            BoundAssignmentOperator op = BindAssignment(node, leftPlaceholder, rightPlaceholder, diagnostics);
+
+            return new BoundDeconstructionAssignmentOperator.AssignmentInfo() { Assignment = op, LValuePlaceholder = leftPlaceholder, RValuePlaceholder = rightPlaceholder };
+        }
+
         /// <summary>
         /// Find the Deconstruct method for the expression on the right, that will fit the assignable bound expressions on the left.
         /// Returns true if the Deconstruct method is found.

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

@@ -397,7 +397,7 @@
     <Field Name="LeftVariables" Type="ImmutableArray&lt;BoundExpression&gt;"/>
 
     <Field Name="Right" Type="BoundExpression"/>
-    <Field Name="DeconstructMember" Type="MethodSymbol" Null="disallow"/>
+    <Field Name="DeconstructMemberOpt" Type="MethodSymbol" Null="allow"/>
 
     <!-- The assignments have placeholders for the left and right part of the assignment -->
     <Field Name="Assignments" Type="ImmutableArray&lt;BoundDeconstructionAssignmentOperator.AssignmentInfo&gt;" Null="NotApplicable" SkipInVisitor="true"/>

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -4884,4 +4884,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_MissingDeconstruct" xml:space="preserve">
     <value>No Deconstruct instance or extension method was found for type '{0}'.</value>
   </data>
+  <data name="ERR_DeconstructRequiresExpression" xml:space="preserve">
+    <value>Deconstruct assignment requires an expression with a type on the right-hand-side.</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1395,5 +1395,6 @@ internal enum ErrorCode
         ERR_AmbiguousDeconstruct = 8207,
         ERR_DeconstructRequiresOutParams = 8208,
         ERR_DeconstructWrongParams = 8209,
+        ERR_DeconstructRequiresExpression = 8210,
     }
 }