Skip to content

Commit

Permalink
Don't require ValueTuple type in deconstruction if return value is no…
Browse files Browse the repository at this point in the history 
…t used (#20295)
  • Loading branch information
@jcouv
jcouv committed Jun 29, 2017
1 parent beacb39 commit 239af49
Show file tree
Hide file tree
Showing 13 changed files with 818 additions and 384 deletions.
60 changes: 49 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
View file
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)
{
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)
{
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
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
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
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)
{
return node.Kind == BoundKind.DeconstructionAssignmentOperator && !((BoundDeconstructionAssignmentOperator)node).IsUsed;
}

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

0 comments on commit 239af49

Please sign in to comment.