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.

Sign up for GitHub

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

Jump to bottom

Don't require ValueTuple type in deconstruction if return value is not used #20295

Merged
merged 10 commits into from
Jun 29, 2017
Merged

Don't require ValueTuple type in deconstruction if return value is not used #20295

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
86e2971
Don't require ValueTuple type in deconstruction if return value is no…
jcouv Jun 17, 2017
8b0e679
Fix EE scenarios
jcouv Jun 20, 2017
91d300a
Address PR feedback from Chuck
jcouv Jun 20, 2017
5a0b7fb
Address more PR feedback from Chuck
jcouv Jun 20, 2017
5ca6259
Address PR feedback from Aleksey
jcouv Jun 22, 2017
996fae2
Address more PR feedback
jcouv Jun 22, 2017
11f2783
Use the last effect as the result of the sequence, when lowering
jcouv Jun 24, 2017
127f65a
Tweak comment and fix two tests
jcouv Jun 27, 2017
a694550
Fix leak of array builders
jcouv Jun 27, 2017
43cf584
Tweak PopLast on deconstruction effects
jcouv Jun 27, 2017
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
60 changes: 49 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal partial class Binder
{
private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
internal BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, DiagnosticBag diagnostics, bool resultIsUsedOverride = false)
{
var left = node.Left;
var right = node.Right;
DeclarationExpressionSyntax declaration = null;
ExpressionSyntax expression = null;
var result = BindDeconstruction(node, left, right, diagnostics, ref declaration, ref expression);
var result = BindDeconstruction(node, left, right, diagnostics, ref declaration, ref expression, resultIsUsedOverride);
if (declaration != null)
{
// only allowed at the top level, or in a for loop
Expand Down Expand Up @@ -78,6 +78,8 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
/// <param name="diagnostics">Where to report diagnostics</param>
/// <param name="declaration">A variable set to the first variable declaration found in the left</param>
/// <param name="expression">A variable set to the first expression in the left that isn't a declaration or discard</param>
/// <param name="resultIsUsedOverride">The expression evaluator needs to bind deconstructions (both assignments and declarations) as expression-statements
/// and still access the returned value</param>
/// <param name="rightPlaceholder"></param>
/// <returns></returns>
internal BoundDeconstructionAssignmentOperator BindDeconstruction(
Expand All @@ -87,6 +89,7 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
DiagnosticBag diagnostics,
ref DeclarationExpressionSyntax declaration,
ref ExpressionSyntax expression,
bool resultIsUsedOverride = false,
BoundDeconstructValuePlaceholder rightPlaceholder = null)
{
DeconstructionVariable locals = BindDeconstructionVariables(left, diagnostics, ref declaration, ref expression);
Expand All @@ -95,7 +98,8 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
BoundExpression boundRight = rightPlaceholder ?? BindValue(right, diagnostics, BindValueKind.RValue);
boundRight = FixTupleLiteral(locals.NestedVariables, boundRight, deconstruction, diagnostics);

var assignment = BindDeconstructionAssignment(deconstruction, left, boundRight, locals.NestedVariables, diagnostics);
bool resultIsUsed = resultIsUsedOverride || IsDeconstructionResultUsed(left);
var assignment = BindDeconstructionAssignment(deconstruction, left, boundRight, locals.NestedVariables, resultIsUsed, diagnostics);
DeconstructionVariable.FreeDeconstructionVariables(locals.NestedVariables);

return assignment;
Expand All @@ -106,6 +110,7 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
ExpressionSyntax left,
BoundExpression boundRHS,
ArrayBuilder<DeconstructionVariable> checkedVariables,
bool resultIsUsed,
DiagnosticBag diagnostics)
{
if ((object)boundRHS.Type == null || boundRHS.Type.IsErrorType())
Expand All @@ -117,9 +122,10 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
var type = boundRHS.Type ?? voidType;
return new BoundDeconstructionAssignmentOperator(
node,
DeconstructionVariablesAsTuple(node, checkedVariables, diagnostics, hasErrors: true),
DeconstructionVariablesAsTuple(node, checkedVariables, diagnostics, ignoreDiagnosticsFromTuple: true),
new BoundConversion(boundRHS.Syntax, boundRHS, Conversion.Deconstruction, @checked: false, explicitCastInCode: false,
constantValueOpt: null, type: type, hasErrors: true),
resultIsUsed,
voidType,
hasErrors: true);
}
Expand All @@ -135,7 +141,7 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag

FailRemainingInferences(checkedVariables, diagnostics);

var lhsTuple = DeconstructionVariablesAsTuple(left, checkedVariables, diagnostics, hasErrors);
var lhsTuple = DeconstructionVariablesAsTuple(left, checkedVariables, diagnostics, ignoreDiagnosticsFromTuple: hasErrors || !resultIsUsed);
TypeSymbol returnType = hasErrors ? CreateErrorType() : lhsTuple.Type;

var boundConversion = new BoundConversion(
Expand All @@ -149,7 +155,38 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
hasErrors: hasErrors)
{ WasCompilerGenerated = true };

return new BoundDeconstructionAssignmentOperator(node, lhsTuple, boundConversion, returnType);
return new BoundDeconstructionAssignmentOperator(node, lhsTuple, boundConversion, resultIsUsed, returnType);
}

private static bool IsDeconstructionResultUsed(ExpressionSyntax left)
{
var parent = left.Parent;
if (parent is null || parent.Kind() == SyntaxKind.ForEachVariableStatement)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

parent is null [](start = 16, length = 14)

Are we building with compiler that performs efficient code gen for this condition? #Closed

Copy link
Member Author

@jcouv jcouv Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not yet. The master branch is using 2.3.0-beta2-61716-09 (from May 16th), but this optimization went into on May 30th. We will definitely refresh with RTM drop (if not sooner), and this PR is for 15.6. #Resolved

{
return false;
}

Debug.Assert(parent.Kind() == SyntaxKind.SimpleAssignmentExpression);

var grandParent = parent.Parent;
if (grandParent is null)
{
return false;
}

switch (grandParent.Kind())
{
case SyntaxKind.ExpressionStatement:
return ((ExpressionStatementSyntax)grandParent).Expression != parent;

case SyntaxKind.ForStatement:
// Incrementors and Initializers don't have to produce a value
var loop = (ForStatementSyntax)grandParent;
return !loop.Incrementors.Contains(parent) && !loop.Initializers.Contains(parent);

default:
return true;
}
}

/// <summary>When boundRHS is a tuple literal, fix it up by inferring its types.</summary>
Expand Down Expand Up @@ -467,7 +504,8 @@ private static TypeSymbol MakeMergedTupleType(ArrayBuilder<DeconstructionVariabl
errorPositions: default(ImmutableArray<bool>));
}

private BoundTupleLiteral DeconstructionVariablesAsTuple(CSharpSyntaxNode syntax, ArrayBuilder<DeconstructionVariable> variables, DiagnosticBag diagnostics, bool hasErrors)
private BoundTupleLiteral DeconstructionVariablesAsTuple(CSharpSyntaxNode syntax, ArrayBuilder<DeconstructionVariable> variables,
DiagnosticBag diagnostics, bool ignoreDiagnosticsFromTuple)
Copy link
Member

Choose a reason for hiding this comment

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

Is ignoreDiagnosticsFromTuple needed or could the caller pass diagnostics: null in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could pass null diagnostics, but the feedback from Aleksey was that pushing that logic down (being more explicit) is safer as the code changes in the future.

{
int count = variables.Count;
var valuesBuilder = ArrayBuilder<BoundExpression>.GetInstance(count);
Expand All @@ -478,7 +516,7 @@ private BoundTupleLiteral DeconstructionVariablesAsTuple(CSharpSyntaxNode syntax
{
if (variable.HasNestedVariables)
{
var nestedTuple = DeconstructionVariablesAsTuple(variable.Syntax, variable.NestedVariables, diagnostics, hasErrors);
var nestedTuple = DeconstructionVariablesAsTuple(variable.Syntax, variable.NestedVariables, diagnostics, ignoreDiagnosticsFromTuple);
valuesBuilder.Add(nestedTuple);
typesBuilder.Add(nestedTuple.Type);
namesBuilder.Add(null);
Expand All @@ -504,9 +542,9 @@ private BoundTupleLiteral DeconstructionVariablesAsTuple(CSharpSyntaxNode syntax
var type = TupleTypeSymbol.Create(syntax.Location,
typesBuilder.ToImmutableAndFree(), locationsBuilder.ToImmutableAndFree(),
tupleNames, this.Compilation,
shouldCheckConstraints: !hasErrors,
errorPositions: disallowInferredNames ? inferredPositions : default(ImmutableArray<bool>),
syntax: syntax, diagnostics: hasErrors ? null : diagnostics);
shouldCheckConstraints: !ignoreDiagnosticsFromTuple,
errorPositions: disallowInferredNames ? inferredPositions : default,
syntax: syntax, diagnostics: ignoreDiagnosticsFromTuple ? null : diagnostics);

// Always track the inferred positions in the bound node, so that conversions don't produce a warning
// for "dropped names" on tuple literal when the name was inferred.
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@

<Field Name="Left" Type="BoundTupleExpression" Null="disallow"/>
<Field Name="Right" Type="BoundConversion" Null="disallow"/>
<Field Name="IsUsed" Type="Boolean" Null="NotApplicable"/>
</Node>

<Node Name="BoundNullCoalescingOperator" Base="BoundExpression">
Expand Down
14 changes: 9 additions & 5 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ public BoundAssignmentOperator Update(BoundExpression left, BoundExpression righ

internal sealed partial class BoundDeconstructionAssignmentOperator : BoundExpression
{
public BoundDeconstructionAssignmentOperator(SyntaxNode syntax, BoundTupleExpression left, BoundConversion right, TypeSymbol type, bool hasErrors = false)
public BoundDeconstructionAssignmentOperator(SyntaxNode syntax, BoundTupleExpression left, BoundConversion right, Boolean isUsed, TypeSymbol type, bool hasErrors = false)
: base(BoundKind.DeconstructionAssignmentOperator, syntax, type, hasErrors || left.HasErrors() || right.HasErrors())
{

Expand All @@ -1170,23 +1170,26 @@ public BoundDeconstructionAssignmentOperator(SyntaxNode syntax, BoundTupleExpres

this.Left = left;
this.Right = right;
this.IsUsed = isUsed;
}


public BoundTupleExpression Left { get; }

public BoundConversion Right { get; }

public Boolean IsUsed { get; }

public override BoundNode Accept(BoundTreeVisitor visitor)
{
return visitor.VisitDeconstructionAssignmentOperator(this);
}

public BoundDeconstructionAssignmentOperator Update(BoundTupleExpression left, BoundConversion right, TypeSymbol type)
public BoundDeconstructionAssignmentOperator Update(BoundTupleExpression left, BoundConversion right, Boolean isUsed, TypeSymbol type)
{
if (left != this.Left || right != this.Right || type != this.Type)
if (left != this.Left || right != this.Right || isUsed != this.IsUsed || type != this.Type)
{
var result = new BoundDeconstructionAssignmentOperator(this.Syntax, left, right, type, this.HasErrors);
var result = new BoundDeconstructionAssignmentOperator(this.Syntax, left, right, isUsed, type, this.HasErrors);
result.WasCompilerGenerated = this.WasCompilerGenerated;
return result;
}
Expand Down Expand Up @@ -8509,7 +8512,7 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct
BoundTupleExpression left = (BoundTupleExpression)this.Visit(node.Left);
BoundConversion right = (BoundConversion)this.Visit(node.Right);
TypeSymbol type = this.VisitType(node.Type);
return node.Update(left, right, type);
return node.Update(left, right, node.IsUsed, type);
}
public override BoundNode VisitNullCoalescingOperator(BoundNullCoalescingOperator node)
{
Expand Down Expand Up @@ -9503,6 +9506,7 @@ public override TreeDumperNode VisitDeconstructionAssignmentOperator(BoundDecons
{
new TreeDumperNode("left", null, new TreeDumperNode[] { Visit(node.Left, null) }),
new TreeDumperNode("right", null, new TreeDumperNode[] { Visit(node.Right, null) }),
new TreeDumperNode("isUsed", node.IsUsed, null),
new TreeDumperNode("type", node.Type, null)
}
);
Expand Down
9 changes: 8 additions & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,19 @@ private BoundExpression VisitExpressionImpl(BoundExpression node)
// like compound assignment does (extra flag only passed when it is an expression
// statement means that this constraint is not violated).
// Dynamic type will be erased in emit phase. It is considered equivalent to Object in lowered bound trees.
// Unused deconstructions are lowered to produce a return value that isn't a tuple type.
Debug.Assert(visited == null || visited.HasErrors || ReferenceEquals(visited.Type, node.Type) ||
visited.Type.Equals(node.Type, TypeCompareKind.IgnoreDynamicAndTupleNames));
visited.Type.Equals(node.Type, TypeCompareKind.IgnoreDynamicAndTupleNames) ||
IsUnusedDeconstruction(node));

return visited;
}

private static bool IsUnusedDeconstruction(BoundExpression node)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IsUnusedDeconstruction [](start = 28, length = 22)

Would it make sense to put this helper as an extension method into src\Compilers\CSharp\Portable\BoundTree\BoundExpressionExtensions.cs ? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used only here, so I left as-is. Let met know if ok.

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

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's used only here, so I left as-is. Let met know if ok.

I think that is fine. Thanks. #Closed

{
return node.Kind == BoundKind.DeconstructionAssignmentOperator && !((BoundDeconstructionAssignmentOperator)node).IsUsed;
}

public override BoundNode VisitLambda(BoundLambda node)
{
_sawLambdas = true;
Expand Down
Loading