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: nested scenario #11715

Merged
merged 13 commits into from
Jun 7, 2016
285 changes: 232 additions & 53 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

Large diffs are not rendered by default.

10 changes: 0 additions & 10 deletions src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -278,16 +278,6 @@ 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
Expand Down
21 changes: 16 additions & 5 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@
This node is used to represent an expression returning value of a certain type.
It is used to perform intermediate binding, and will not survive the local rewriting.
-->
<Node Name="BoundLValuePlaceholder" Base="BoundValuePlaceholderBase" HasValidate="true">
</Node>
<Node Name="BoundRValuePlaceholder" Base="BoundValuePlaceholderBase" HasValidate="true">
<Node Name="BoundDeconstructValuePlaceholder" Base="BoundValuePlaceholderBase" HasValidate="true">
</Node>

<!-- only used by codegen -->
Expand Down Expand Up @@ -394,13 +392,26 @@
<!-- Non-null type is required for this node kind -->
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

<!-- Various assignable expressions, including some BoundDeconstructionVariables for nested variables -->
<Field Name="LeftVariables" Type="ImmutableArray&lt;BoundExpression&gt;"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need BoundDeconstructionVariables in this list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presence of these node can potentially confuse SemanticModel, etc.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, given the current implementation, it looks like LeftVariable is always a flat list of targets, BoundDeconstructionVariables is never among them. Please adjust the comment accordingly.


In reply to: 65778993 [](ancestors = 65778993,65776149)

Copy link
Member Author

@jcouv jcouv Jun 4, 2016

Choose a reason for hiding this comment

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

@AlekseyTs, from discussion with Chuck I will remove the BoundDeconstructionVariables node and store the tree of variables some other way (without creating new kind of bound node).


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

<Field Name="DeconstructSteps" Type="ImmutableArray&lt;BoundDeconstructionDeconstructStep&gt;" Null="NotApplicable" SkipInVisitor="true" />

<Field Name="AssignmentSteps" Type="ImmutableArray&lt;BoundDeconstructionAssignmentStep&gt;" Null="NotApplicable" SkipInVisitor="true"/>
</Node>

<Node Name="BoundDeconstructionDeconstructStep" Base="BoundNode">
<Field Name="DeconstructMemberOpt" Type="MethodSymbol" Null="allow"/>
<Field Name="TargetPlaceholder" Type="BoundDeconstructValuePlaceholder" Null="disallow"/>
<Field Name="OutputPlaceholders" Type="ImmutableArray&lt;BoundDeconstructValuePlaceholder&gt;"/>
</Node>

<!-- 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"/>
<Node Name="BoundDeconstructionAssignmentStep" Base="BoundNode">
<Field Name="Assignment" Type="BoundAssignmentOperator" Null="disallow"/>
<Field Name="TargetPlaceholder" Type="BoundDeconstructValuePlaceholder" Null="disallow"/>
<Field Name="OutputPlaceholder" Type="BoundDeconstructValuePlaceholder" Null="disallow"/>
</Node>

<Node Name="BoundVoid" Base="BoundExpression">
Expand Down
17 changes: 1 addition & 16 deletions src/Compilers/CSharp/Portable/BoundTree/Expression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,7 @@ Optional<object> IOperation.ConstantValue
public abstract TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument);
}

internal sealed partial class BoundLValuePlaceholder : BoundValuePlaceholderBase, IPlaceholderExpression
{
protected override OperationKind ExpressionKind => OperationKind.PlaceholderExpression;

public override void Accept(OperationVisitor visitor)
{
visitor.VisitPlaceholderExpression(this);
}

public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument)
{
return visitor.VisitPlaceholderExpression(this, argument);
}
}

internal sealed partial class BoundRValuePlaceholder : BoundValuePlaceholderBase, IPlaceholderExpression
internal sealed partial class BoundDeconstructValuePlaceholder : BoundValuePlaceholderBase, IPlaceholderExpression
{
protected override OperationKind ExpressionKind => OperationKind.PlaceholderExpression;

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.

5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -4899,4 +4899,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_OptionMustBeAbsolutePath" xml:space="preserve">
<value>Option '{0}' must be an absolute path.</value>
</data>
</root>
<data name="ERR_DeconstructWrongCardinality" xml:space="preserve">
<value>Cannot deconstruct a tuple of '{0}' elements into '{1}' variables.</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 @@ -1399,5 +1399,6 @@ internal enum ErrorCode
ERR_DeconstructRequiresOutParams = 8208,
ERR_DeconstructWrongParams = 8209,
ERR_DeconstructRequiresExpression = 8210,
ERR_DeconstructWrongCardinality = 8211,
}
}
9 changes: 5 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1485,7 +1485,7 @@ public override BoundNode VisitFixedStatement(BoundFixedStatement node)
{
foreach (LocalSymbol local in node.Locals)
{
switch(local.DeclarationKind)
switch (local.DeclarationKind)
{
case LocalDeclarationKind.RegularVariable:
case LocalDeclarationKind.PatternVariable:
Expand Down Expand Up @@ -1721,14 +1721,15 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
Assign(node.Left, node.Right, refKind: node.RefKind);
return null;
}

public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstructionAssignmentOperator node)
{
base.VisitDeconstructionAssignmentOperator(node);

foreach(BoundExpression variable in node.LeftVariables)
foreach (BoundExpression variable in node.LeftVariables)
{
Assign(variable, null, refKind: RefKind.None);
// PROTOTYPE(tuples) value should not be set to null
Assign(variable, value: null, refKind: RefKind.None);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,12 +230,7 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen
}
}

public override BoundNode VisitLValuePlaceholder(BoundLValuePlaceholder node)
{
return PlaceholderReplacement(node);
}

public override BoundNode VisitRValuePlaceholder(BoundRValuePlaceholder node)
public override BoundNode VisitDeconstructValuePlaceholder(BoundDeconstructValuePlaceholder node)
{
return PlaceholderReplacement(node);
}
Expand Down Expand Up @@ -287,6 +282,17 @@ private void RemovePlaceholderReplacement(BoundValuePlaceholderBase placeholder)
Debug.Assert(removed);
}

/// <summary>
/// Remove all the listed placeholders.
/// </summary>
private void RemovePlaceholderReplacements(ArrayBuilder<BoundValuePlaceholderBase> placeholders)
{
foreach (var placeholder in placeholders)
{
RemovePlaceholderReplacement(placeholder);
}
}

public override BoundNode VisitBadExpression(BoundBadExpression node)
{
// Cannot recurse into BadExpression children since the BadExpression
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// 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;
Expand All @@ -12,76 +11,92 @@ internal sealed partial class LocalRewriter
{
public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstructionAssignmentOperator node)
{
Debug.Assert(node.DeconstructSteps != null);
Debug.Assert(node.AssignmentSteps != null);

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

var lhsReceivers = ArrayBuilder<BoundExpression>.GetInstance();
foreach (var variable in node.LeftVariables)
{
// This will be filled in with the LHS that uses temporaries to prevent
// double-evaluation of side effects.
lhsReceivers.Add(TransformCompoundAssignmentLHS(variable, stores, temps, isDynamicAssignment: false));
}
// evaluate left-hand-side side-effects
var lhsTemps = LeftHandSideSideEffects(node.LeftVariables, temps, stores);

// get or make right-hand-side values
BoundExpression loweredRight = VisitExpression(node.Right);
ImmutableArray<BoundExpression> rhsValues;
AddPlaceholderReplacement(node.DeconstructSteps[0].TargetPlaceholder, loweredRight);
placeholders.Add(node.DeconstructSteps[0].TargetPlaceholder);
Copy link
Member

Choose a reason for hiding this comment

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

Is this AddPlaceholderReplacement case also handled by the foreach loop below?

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 I understand the question.
Here we bootstrap the first placeholder for the RHS value. After the RHS value is deconstructed, it will populate proper placeholders for subsequence deconstructs, and so on. Those same placeholders are also referenced by the assignment step.
At the very end, we clear all those "carried-through" placeholders.

Copy link
Member

Choose a reason for hiding this comment

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

My mistake, I didn't realize AccessTupleFields and CallDeconstruct drilled in to fields or parameters of each step.


// get or make right-hand-side values
if (node.Right.Type.IsTupleType)
{
rhsValues = AccessTupleFields(node, loweredRight, temps, stores);
}
else
foreach (var deconstruction in node.DeconstructSteps)
{
rhsValues = CallDeconstruct(node, loweredRight, temps, stores);
if (deconstruction.DeconstructMemberOpt == null)
{
// tuple case
AccessTupleFields(node, deconstruction, temps, stores, placeholders);
}
else
{
CallDeconstruct(node, deconstruction, temps, stores, placeholders);
}
}

// assign from rhs values to lhs receivers
int numAssignments = node.Assignments.Length;
int numAssignments = node.AssignmentSteps.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, rhsValues[i]);
// lower the assignment and replace the placeholders for its outputs in the process
var assignmentInfo = node.AssignmentSteps[i];
AddPlaceholderReplacement(assignmentInfo.OutputPlaceholder, lhsTemps[i]);

var assignment = VisitExpression(assignmentInfo.Assignment);

RemovePlaceholderReplacement(assignmentInfo.LValuePlaceholder);
RemovePlaceholderReplacement(assignmentInfo.RValuePlaceholder);
RemovePlaceholderReplacement(assignmentInfo.OutputPlaceholder);

stores.Add(assignment);
}

stores.Add(new BoundVoid(node.Syntax, node.Type));
var result = _factory.Sequence(temps.ToImmutable(), stores.ToArray());
var result = _factory.Sequence(temps.ToImmutable(), stores.ToImmutable(), new BoundVoid(node.Syntax, node.Type));

RemovePlaceholderReplacements(placeholders);
placeholders.Free();

temps.Free();
stores.Free();
lhsReceivers.Free();

return result;
}

private ImmutableArray<BoundExpression> AccessTupleFields(BoundDeconstructionAssignmentOperator node, BoundExpression loweredRight, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores)
/// <summary>
/// Adds the side effects to stores and returns temporaries (as a flat list) to access them.
/// </summary>
private ImmutableArray<BoundExpression> LeftHandSideSideEffects(ImmutableArray<BoundExpression> variables, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores)
{
var lhsReceivers = ArrayBuilder<BoundExpression>.GetInstance(variables.Length);

foreach (var variable in variables)
{
// PROTOTYPE(tuples) should the dynamic flag always be false?
lhsReceivers.Add(TransformCompoundAssignmentLHS(variable, stores, temps, isDynamicAssignment: false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this change, but are we certain that isDynamicAssignment should always be false?

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. Added a follow-up item "Verify correct handling of dynamic" to #11299 and a PROTOTYPE marker.

}

return lhsReceivers.ToImmutableAndFree();
}

private void AccessTupleFields(BoundDeconstructionAssignmentOperator node, BoundDeconstructionDeconstructStep deconstruction, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores, ArrayBuilder<BoundValuePlaceholderBase> placeholders)
{
var tupleType = loweredRight.Type.IsTupleType ? loweredRight.Type : TupleTypeSymbol.Create((NamedTypeSymbol)loweredRight.Type);
var target = PlaceholderReplacement(deconstruction.TargetPlaceholder);
var tupleType = target.Type.IsTupleType ? target.Type : TupleTypeSymbol.Create((NamedTypeSymbol)target.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
// save the target as we need to access it multiple times
BoundAssignmentOperator assignmentToTemp;
var savedTuple = _factory.StoreToTemp(loweredRight, out assignmentToTemp);
var savedTuple = _factory.StoreToTemp(target, out assignmentToTemp);
stores.Add(assignmentToTemp);
temps.Add(savedTuple.LocalSymbol);

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

for (int i = 0; i < numElements; i++)
Expand All @@ -94,30 +109,30 @@ private ImmutableArray<BoundExpression> AccessTupleFields(BoundDeconstructionAss
Symbol.ReportUseSiteDiagnostic(useSiteInfo, _diagnostics, syntax.Location);
}
var fieldAccess = MakeTupleFieldAccess(syntax, field, savedTuple, null, LookupResultKind.Empty, tupleElementTypes[i]);
fieldAccessorsBuilder.Add(fieldAccess);
}

return fieldAccessorsBuilder.ToImmutableAndFree();
AddPlaceholderReplacement(deconstruction.OutputPlaceholders[i], fieldAccess);
placeholders.Add(deconstruction.OutputPlaceholders[i]);
}
}

/// <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)
private void CallDeconstruct(BoundDeconstructionAssignmentOperator node, BoundDeconstructionDeconstructStep deconstruction, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> stores, ArrayBuilder<BoundValuePlaceholderBase> placeholders)
{
Debug.Assert((object)node.DeconstructMemberOpt != null);
Debug.Assert((object)deconstruction.DeconstructMemberOpt != null);

CSharpSyntaxNode syntax = node.Syntax;

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

foreach (var deconstructParameter in deconstructParameters)
for (var i = 0; i < deconstructParameters.Length; i++)
{
var deconstructParameter = deconstructParameters[i];
var localSymbol = new SynthesizedLocal(_factory.CurrentMethod, deconstructParameter.Type, SynthesizedLocalKind.LoweringTemp);

var localBound = new BoundLocal(syntax,
Expand All @@ -129,15 +144,16 @@ private ImmutableArray<BoundExpression> CallDeconstruct(BoundDeconstructionAssig

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

AddPlaceholderReplacement(deconstruction.OutputPlaceholders[i], localBound);
placeholders.Add(deconstruction.OutputPlaceholders[i]);
}

var outParameters = outParametersBuilder.ToImmutableAndFree();

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

return outParameters;
}
}
}
Loading