Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deconstruction-assignment for tuples #11457

Merged
merged 10 commits into from
May 24, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -1737,6 +1744,52 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using foreach above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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, new[] { boundRHS });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using overload BadExpression(CSharpSyntaxNode, BoundNode).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Done

}

return BindDeconstructWithDeconstruct(node, diagnostics, boundRHS, checkedVariables);
}

private BoundExpression BindDeconstructWithTuple(AssignmentExpressionSyntax node, DiagnosticBag diagnostics, BoundExpression boundRHS, ImmutableArray<BoundExpression> checkedVariables)
{
// patch up the tuple literal so that it has a type, based on the LHS information
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should elementLocations be set from checkedVariables?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. The left-hand-side really isn't a tuple (it's a set of individual variables), although it has tuple-looking syntax...
My goal here was to do the minimum faking of the left-hand-side-as-a-tuple so that I can give the expression on the right a proper type (the conversion logic).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think locations should not be set here because this is not a declaration location for the tuple type.


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

var typedRHS = GenerateConversionForAssignment(lhsAsTuple, boundRHS, diagnostics);

ImmutableArray<TypeSymbol> tupleTypes = typedRHS.Type.TupleElementTypes;

// figure out the pairwise conversions
var assignmentsBuilder = ArrayBuilder<BoundDeconstructionAssignmentOperator.AssignmentInfo>.GetInstance(checkedVariables.Length);

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, tupleTypes[i]) { 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation of assignments looks similar to the calculation in BindDeconstructWithDeconstruct. Can the code be shared?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :-)
Initially the code for those two paths diverged, but now it's back to being very similar. I'll factor.


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);
Expand All @@ -1754,7 +1807,7 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
}

// figure out the pairwise conversions
var assignmentsBuilder = ArrayBuilder<BoundDeconstructionAssignmentOperator.AssignmentInfo>.GetInstance(numElements);
var assignmentsBuilder = ArrayBuilder<BoundDeconstructionAssignmentOperator.AssignmentInfo>.GetInstance(checkedVariables.Length);
var deconstructParameters = deconstructMethod.Parameters;
for (int i = 0; i < checkedVariables.Length; i++)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1395,5 +1395,6 @@ internal enum ErrorCode
ERR_AmbiguousDeconstruct = 8207,
ERR_DeconstructRequiresOutParams = 8208,
ERR_DeconstructWrongParams = 8209,
ERR_DeconstructRequiresExpression = 8210,
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;

namespace Microsoft.CodeAnalysis.CSharp
Expand All @@ -9,9 +12,6 @@ internal sealed partial class LocalRewriter
{
public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstructionAssignmentOperator node)
{
CSharpSyntaxNode syntax = node.Syntax;
int numVariables = node.LeftVariables.Length;

var temps = ArrayBuilder<LocalSymbol>.GetInstance();
var stores = ArrayBuilder<BoundExpression>.GetInstance();

Expand All @@ -24,40 +24,27 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct
}

BoundExpression loweredRight = VisitExpression(node.Right);
ImmutableArray<BoundExpression> rhsValues;

// prepare out parameters for Deconstruct
var deconstructParameters = node.DeconstructMember.Parameters;
var outParametersBuilder = ArrayBuilder<BoundExpression>.GetInstance(deconstructParameters.Length);
Debug.Assert(deconstructParameters.Length == node.LeftVariables.Length);

for (int i = 0; i < numVariables; i++)
// get or make right-hand-side values
if (node.Right.Type.IsTupleType)
{
var localSymbol = new SynthesizedLocal(_factory.CurrentMethod, deconstructParameters[i].Type, SynthesizedLocalKind.LoweringTemp);

var localBound = new BoundLocal(syntax,
localSymbol,
null,
deconstructParameters[i].Type
) { WasCompilerGenerated = true };

temps.Add(localSymbol);
outParametersBuilder.Add(localBound);
rhsValues = AccessTupleFields(node, loweredRight, temps, stores);
}
else
{
rhsValues = CallDeconstruct(node, loweredRight, temps, stores);
}

var outParameters = outParametersBuilder.ToImmutableAndFree();

// invoke Deconstruct
var invokeDeconstruct = MakeCall(syntax, loweredRight, node.DeconstructMember, outParameters, node.DeconstructMember.ReturnType);
stores.Add(invokeDeconstruct);

// assign from out temps to lhs receivers
for (int i = 0; i < numVariables; i++)
// assign from rhs values to lhs receivers
int numAssignments = node.Assignments.Length;
for (int i = 0; i < numAssignments; i++)
{
// lower the assignment and replace the placeholders for source and target in the process
var assignmentInfo = node.Assignments[i];

AddPlaceholderReplacement(assignmentInfo.LValuePlaceholder, lhsReceivers[i]);
AddPlaceholderReplacement(assignmentInfo.RValuePlaceholder, outParameters[i]);
AddPlaceholderReplacement(assignmentInfo.RValuePlaceholder, rhsValues[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this loop interleaves LHS accesses and RHS reads. That does not seem right.
It should be something like:

  1. go through all LHS expressions, lower them, and make temps if they are sideeffecting in their lowered form.
  2. go through all RHS while lowering and making assignments to the lowered LHS directly (if does not need temp) or to a corresponding temp made at step1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the code above needs to be updated after LDM decision to not bundle conversion with assignment, I think it is otherwise correct and matches your description.

All the LHS side-effects have already been stacked on line 23: lhsReceivers.Add(TransformCompoundAssignmentLHS(variable, stores, temps, isDynamicAssignment: false));. That is step (1) in your description.

The loop that you commented on only takes care of conversion+assigment (step 2 from your description).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Then it should be all good.


var assignment = VisitExpression(assignmentInfo.Assignment);

Expand All @@ -75,5 +62,76 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct

return result;
}

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

var tupleElementTypes = tupleType.TupleElementTypes;

var numElements = tupleElementTypes.Length;
Debug.Assert(numElements == node.LeftVariables.Length);

CSharpSyntaxNode syntax = node.Syntax;

// save the loweredRight as we need to access it multiple times
BoundAssignmentOperator assignmentToTemp;
var savedTuple = _factory.StoreToTemp(loweredRight, out assignmentToTemp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably optimize for a case when it is already a local or parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Added a work item in #11299.
One thing that surprised me is that we had discussed some possible other optimization, which turned out to already fall in place (see the tests that check IL). I'm not sure how that is already happening.

Copy link
Member

@VSadov VSadov May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in general not a trivial optimization. RHS could be captured in a closure and changed between reads, could be used on the left, could be aliased via a byref local...

Simple redundant temp cases should be already handled in the codegen.

stores.Add(assignmentToTemp);
temps.Add(savedTuple.LocalSymbol);

// list the tuple fields
var fieldAccessorsBuilder = ArrayBuilder<BoundExpression>.GetInstance(numElements);

for (int i = 0; i < numElements; i++)
{
var fields = tupleType.GetMembers(TupleTypeSymbol.TupleMemberName(i + 1)).OfType<FieldSymbol>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a reliable way to lookup the field. We should probably add an API GetTupleElementField(int elementIndex). Let's talk offline in more details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I was a bit unsure about this, as I had to pick one of multiple members (although I never expect multiple). We used to have a way to enumerate field symbols on the tuple symbol, but I don't think it's there anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

var field = fields.Single();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot make this assumption. This is related to the previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with new method of getting fields.

var fieldAccess = MakeTupleFieldAccess(syntax, field, savedTuple, null, LookupResultKind.Empty, tupleElementTypes[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation of MakeTupleFieldAccess assumes use-site errors for the field are reported prior to calling it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand this comment. I'll stop by in the morning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See what happens when underlying type doesn't define the field for the element.


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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added such test and fixed. Thanks

fieldAccessorsBuilder.Add(fieldAccess);
}

return fieldAccessorsBuilder.ToImmutableAndFree();
}

/// <summary>
/// Prepares local variables to be used in Deconstruct call
/// Adds a invocation of Deconstruct with those as out parameters onto the 'stores' sequence
/// Returns the expressions for those out parameters
/// </summary>
private ImmutableArray<BoundExpression> CallDeconstruct(BoundDeconstructionAssignmentOperator node, BoundExpression loweredRight, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores)
{
Debug.Assert((object)node.DeconstructMemberOpt != null);

int numVariables = node.LeftVariables.Length;
CSharpSyntaxNode syntax = node.Syntax;

// prepare out parameters for Deconstruct
var deconstructParameters = node.DeconstructMemberOpt.Parameters;
var outParametersBuilder = ArrayBuilder<BoundExpression>.GetInstance(deconstructParameters.Length);
Debug.Assert(deconstructParameters.Length == node.LeftVariables.Length);

for (int i = 0; i < numVariables; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreach

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

{
var localSymbol = new SynthesizedLocal(_factory.CurrentMethod, deconstructParameters[i].Type, SynthesizedLocalKind.LoweringTemp);

var localBound = new BoundLocal(syntax,
localSymbol,
null,
deconstructParameters[i].Type
)
{ WasCompilerGenerated = true };

temps.Add(localSymbol);
outParametersBuilder.Add(localBound);
}

var outParameters = outParametersBuilder.ToImmutableAndFree();

// invoke Deconstruct
var invokeDeconstruct = MakeCall(syntax, loweredRight, node.DeconstructMemberOpt, outParameters, node.DeconstructMemberOpt.ReturnType);
stores.Add(invokeDeconstruct);

return outParameters;
}
}
}
Loading