Deconstruction-assignment: PR feedback (4)

docs/features/deconstruction.md

@@ -29,7 +29,7 @@ class C
 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 (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.
 

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

@@ -1738,7 +1738,13 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
             var checkedVariables = checkedVariablesBuilder.ToImmutableAndFree();
 
             // symbol and parameters for Deconstruct
-            MethodSymbol deconstructMethod = FindDeconstruct(checkedVariables, boundRHS, node, diagnostics);
+            DiagnosticBag bag = new DiagnosticBag();
+            MethodSymbol deconstructMethod = FindDeconstruct(checkedVariables, boundRHS, node, bag);
+            if (!diagnostics.HasAnyErrors())
+            {
+                diagnostics.AddRange(bag);
+            }
+
             if ((object)deconstructMethod == null)
             {
                 return new BoundDeconstructionAssignmentOperator(node, checkedVariables, boundRHS, ErrorMethodSymbol.UnknownMethod,
@@ -1747,14 +1753,13 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
                                                                 hasErrors: true);
             }
 
-            var parameterTypes = deconstructMethod.Parameters.SelectAsArray(p => p.Type);
-
             // 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, parameterTypes[i]) { 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);
@@ -1763,7 +1768,7 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
 
             var assignments = assignmentsBuilder.ToImmutableAndFree();
 
-            TypeSymbol lastType = parameterTypes[parameterTypes.Length - 1];
+            TypeSymbol lastType = deconstructParameters.Last().Type;
             return new BoundDeconstructionAssignmentOperator(node, checkedVariables, boundRHS, deconstructMethod, assignments, lastType);
         }
 
@@ -1806,13 +1811,13 @@ private static MethodSymbol FindDeconstruct(ImmutableArray<BoundExpression> chec
 
             if (deconstructMethod.ParameterCount != checkedVariables.Length)
             {
-                Error(diagnostics, ErrorCode.ERR_DeconstructWrongParams, boundRHS.Syntax, boundRHS.Type, checkedVariables.Length);
+                Error(diagnostics, ErrorCode.ERR_DeconstructWrongParams, boundRHS.Syntax, deconstructMethod, checkedVariables.Length);
                 return null;
             }
 
             if (deconstructMethod.Parameters.Any(p => p.RefKind != RefKind.Out))
             {
-                Error(diagnostics, ErrorCode.ERR_DeconstructRequiresOutParams, boundRHS.Syntax, boundRHS.Type);
+                Error(diagnostics, ErrorCode.ERR_DeconstructRequiresOutParams, boundRHS.Syntax, deconstructMethod);
                 return null;
             }
 

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs

@@ -278,6 +278,16 @@ public override Symbol ExpressionSymbol
         public ImmutableArray<MethodSymbol> OriginalUserDefinedOperatorsOpt { get; }
     }
 
+    internal sealed partial class BoundDeconstructionAssignmentOperator : BoundExpression
+    {
+        internal class AssignmentInfo
+        {
+            public BoundAssignmentOperator Assignment;
+            public BoundLValuePlaceholder LValuePlaceholder;
+            public BoundRValuePlaceholder RValuePlaceholder;
+        }
+    }
+
     internal partial class BoundCompoundAssignmentOperator
     {
         public override Symbol ExpressionSymbol

src/Compilers/CSharp/Portable/BoundTree/Expression.cs

@@ -1047,13 +1047,6 @@ public override void Accept(OperationVisitor visitor)
         {
             return visitor.VisitNoneOperation(this, argument);
         }
-
-        internal class AssignmentInfo
-        {
-            public BoundAssignmentOperator Assignment;
-            public BoundLValuePlaceholder LValuePlaceholder;
-            public BoundRValuePlaceholder RValuePlaceholder;
-        }
     }
 
     internal partial class BoundCompoundAssignmentOperator : ICompoundAssignmentExpression

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

@@ -255,7 +255,7 @@ private BoundExpression PlaceholderReplacement(BoundValuePlaceholderBase placeho
         [Conditional("DEBUG")]
         private static void AssertPlaceholderReplacement(BoundValuePlaceholderBase placeholder, BoundExpression value)
         {
-            Debug.Assert(value.Type.Equals(placeholder.Type, ignoreCustomModifiersAndArraySizesAndLowerBounds: true));
+            Debug.Assert(value.Type.Equals(placeholder.Type, ignoreCustomModifiersAndArraySizesAndLowerBounds: true, ignoreDynamic: true));
         }
 
         /// <summary>

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

@@ -110,9 +110,9 @@ public void Deconstruct(out int a)
 }";
             var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular.WithTuplesFeature());
             comp.VerifyDiagnostics(
-                // (8,18): error CS8209: The Deconstruct method for type 'C' doesn't have the number of parameters (2) needed for this deconstruction.
+                // (8,18): error CS8209: The Deconstruct method for type 'C.Deconstruct(out int)' doesn't have the number of parameters (2) needed for this deconstruction.
                 //         (x, y) = new C();
-                Diagnostic(ErrorCode.ERR_DeconstructWrongParams, "new C()").WithArguments("C", "2").WithLocation(8, 18)
+                Diagnostic(ErrorCode.ERR_DeconstructWrongParams, "new C()").WithArguments("C.Deconstruct(out int)", "2").WithLocation(8, 18)
                 );
         }
 
@@ -138,10 +138,7 @@ static void Main()
                 Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "f").WithArguments("long", "f").WithLocation(8, 12),
                 // (8,17): error CS1061: 'string' does not contain a definition for 'g' and no extension method 'g' accepting a first argument of type 'string' could be found (are you missing a using directive or an assembly reference?)
                 //         (x.f, y.g) = new C();
-                Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "g").WithArguments("string", "g").WithLocation(8, 17),
-                // (8,22): error CS8209: The Deconstruct method for type 'C' doesn't have the number of parameters (2) needed for this deconstruction.
-                //         (x.f, y.g) = new C();
-                Diagnostic(ErrorCode.ERR_DeconstructWrongParams, "new C()").WithArguments("C", "2").WithLocation(8, 22)
+                Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "g").WithArguments("string", "g").WithLocation(8, 17)
                 );
         }
 
@@ -162,9 +159,9 @@ static void Main()
 ";
             var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular.WithTuplesFeature());
             comp.VerifyDiagnostics(
-                // (8,18): error CS8208: The Deconstruct method for type 'C' must have only out parameters.
+                // (8,18): error CS8208: The Deconstruct method for type 'C.Deconstruct(out int, int)' must have only out parameters.
                 //         (x, y) = new C();
-                Diagnostic(ErrorCode.ERR_DeconstructRequiresOutParams, "new C()").WithArguments("C").WithLocation(8, 18)
+                Diagnostic(ErrorCode.ERR_DeconstructRequiresOutParams, "new C()").WithArguments("C.Deconstruct(out int, int)").WithLocation(8, 18)
                 );
         }
 
@@ -185,9 +182,9 @@ static void Main()
 ";
             var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular.WithTuplesFeature());
             comp.VerifyDiagnostics(
-                // (8,18): error CS8208: The Deconstruct method for type 'C' must have only out parameters.
+                // (8,18): error CS8208: The Deconstruct method for type 'C.Deconstruct(ref int, out int)' must have only out parameters.
                 //         (x, y) = new C();
-                Diagnostic(ErrorCode.ERR_DeconstructRequiresOutParams, "new C()").WithArguments("C").WithLocation(8, 18)
+                Diagnostic(ErrorCode.ERR_DeconstructRequiresOutParams, "new C()").WithArguments("C.Deconstruct(ref int, out int)").WithLocation(8, 18)
                 );
         }