-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Don't require ValueTuple type in deconstruction if return value is not used #20295
Changes from 8 commits
86e2971
8b0e679
91d300a
5a0b7fb
5ca6259
996fae2
11f2783
127f65a
a694550
43cf584
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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( | ||
|
@@ -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); | ||
|
@@ -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; | ||
|
@@ -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()) | ||
|
@@ -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); | ||
} | ||
|
@@ -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( | ||
|
@@ -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> | ||
|
@@ -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) | ||
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 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. We could pass |
||
{ | ||
int count = variables.Count; | ||
var valuesBuilder = ArrayBuilder<BoundExpression>.GetInstance(count); | ||
|
@@ -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); | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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.
Would it make sense to put this helper as an extension method into src\Compilers\CSharp\Portable\BoundTree\BoundExpressionExtensions.cs ? #Closed 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. It's used only here, so I left as-is. Let met know if ok. In reply to: 123901529 [](ancestors = 123901529) 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 think that is fine. Thanks. #Closed |
||
{ | ||
return node.Kind == BoundKind.DeconstructionAssignmentOperator && !((BoundDeconstructionAssignmentOperator)node).IsUsed; | ||
} | ||
|
||
public override BoundNode VisitLambda(BoundLambda node) | ||
{ | ||
_sawLambdas = true; | ||
|
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.
Are we building with compiler that performs efficient code gen for this condition? #Closed
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.
Not yet. The
master
branch is using2.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