-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 11 commits
c71be15
86ca7d3
dd3b674
199c6ad
461b419
6e68b8d
3b6d012
6f354a1
f205d5f
ba975f7
1748df1
4e531c6
87ee4f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
@@ -287,6 +282,17 @@ private void RemovePlaceholderReplacement(BoundValuePlaceholderBase placeholder) | |
Debug.Assert(removed); | ||
} | ||
|
||
/// <summary> | ||
/// Remove all the listed placeholders. | ||
/// </summary> | ||
private void RemovePlaceholderReplacements(IEnumerable<BoundValuePlaceholderBase> placeholders) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use more specific type rather than IEnumerable? ArrayBuilder/ImmutableArray There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps inline the method since it is only used in one place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect in the future it will be handy again to be able to remove multiple placeholders. I'm not sure what is the downside of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AlekseyTs Why not |
||
{ | ||
foreach (var placeholder in placeholders) | ||
{ | ||
RemovePlaceholderReplacement(placeholder); | ||
} | ||
} | ||
|
||
public override BoundNode VisitBadExpression(BoundBadExpression node) | ||
{ | ||
// Cannot recurse into BadExpression children since the BadExpression | ||
|
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; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand the question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mistake, I didn't realize |
||
|
||
// 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
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++) | ||
|
@@ -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, | ||
|
@@ -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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).