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

Binding for deconstruction-declaration in 'foreach' statement #12326

Merged
merged 11 commits into from
Jul 7, 2016
77 changes: 43 additions & 34 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

Large diffs are not rendered by default.

10 changes: 9 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1723,7 +1723,7 @@ private BoundExpression BindAssignment(AssignmentExpressionSyntax node, Diagnost
return BindAssignment(node, op1, op2, diagnostics);
}

private BoundAssignmentOperator BindAssignment(ExpressionSyntax node, BoundExpression op1, BoundExpression op2, DiagnosticBag diagnostics)
private BoundAssignmentOperator BindAssignment(CSharpSyntaxNode node, BoundExpression op1, BoundExpression op2, DiagnosticBag diagnostics)
{
Debug.Assert(op1 != null);
Debug.Assert(op2 != null);
Expand Down Expand Up @@ -3086,6 +3086,14 @@ internal virtual BoundStatement BindForEachParts(DiagnosticBag diagnostics, Bind
return this.Next.BindForEachParts(diagnostics, originalBinder);
}

/// <summary>
/// Like BindForEachParts, but only bind the deconstruction part of the foreach, for purpose of inferring the types of the declared locals.
/// </summary>
internal virtual void BindForEachDeconstruction(DiagnosticBag diagnostics, Binder originalBinder)
{
this.Next.BindForEachDeconstruction(diagnostics, originalBinder);
}

private BoundStatement BindBreak(BreakStatementSyntax node, DiagnosticBag diagnostics)
{
var target = this.BreakLabel;
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/BuckStopsHereBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,12 @@ internal override BoundStatement BindForEachParts(DiagnosticBag diagnostics, Bin
throw ExceptionUtilities.Unreachable;
}

internal override void BindForEachDeconstruction(DiagnosticBag diagnostics, Binder originalBinder)
{
// There's supposed to be a ForEachLoopBinder (or other overrider of this method) in the chain.
throw ExceptionUtilities.Unreachable;
}

internal override BoundWhileStatement BindWhileParts(DiagnosticBag diagnostics, Binder originalBinder)
{
// There's supposed to be a WhileBinder (or other overrider of this method) in the chain.
Expand Down
120 changes: 90 additions & 30 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ private SourceLocalSymbol IterationVariable
{
get
{
return (SourceLocalSymbol)this.Locals[0];
return _syntax.IsDeconstructionDeclaration ? null : (SourceLocalSymbol)this.Locals[0];
}
}

Expand All @@ -40,14 +40,29 @@ public ForEachLoopBinder(Binder enclosing, ForEachStatementSyntax syntax)

protected override ImmutableArray<LocalSymbol> BuildLocals()
{
var iterationVariable = SourceLocalSymbol.MakeForeachLocal(
(MethodSymbol)this.ContainingMemberOrLambda,
this,
_syntax.Type,
_syntax.Identifier,
_syntax.Expression);

return ImmutableArray.Create<LocalSymbol>(iterationVariable);
if (_syntax.IsDeconstructionDeclaration)
{
var locals = ArrayBuilder<LocalSymbol>.GetInstance();

CollectLocalsFromDeconstruction(
_syntax.DeconstructionVariables,
_syntax.DeconstructionVariables.Type,
LocalDeclarationKind.ForEachIterationVariable,
locals);

return locals.ToImmutableAndFree();
}
else
{
var iterationVariable = SourceLocalSymbol.MakeForeachLocal(
(MethodSymbol)this.ContainingMemberOrLambda,
this,
_syntax.Type,
_syntax.Identifier,
_syntax.Expression);

return ImmutableArray.Create<LocalSymbol>(iterationVariable);
}
}

/// <summary>
Expand All @@ -59,10 +74,32 @@ internal override BoundStatement BindForEachParts(DiagnosticBag diagnostics, Bin
return result;
}

/// <summary>
/// Like BindForEachParts, but only bind the deconstruction part of the foreach, for purpose of inferring the types of the declared locals.
/// </summary>
internal override void BindForEachDeconstruction(DiagnosticBag diagnostics, Binder originalBinder)
{
// Use the right binder to avoid seeing iteration variable
BoundExpression collectionExpr = originalBinder.GetBinder(_syntax.Expression).BindValue(_syntax.Expression, diagnostics, BindValueKind.RValue);

ForEachEnumeratorInfo.Builder builder = new ForEachEnumeratorInfo.Builder();
TypeSymbol inferredType;
bool hasErrors = !GetEnumeratorInfoAndInferCollectionElementType(ref builder, ref collectionExpr, diagnostics, out inferredType);

VariableDeclarationSyntax variables = _syntax.DeconstructionVariables;
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, inferredType ?? CreateErrorType("var"));
Copy link
Member

Choose a reason for hiding this comment

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

The expression inferredType ?? CreateErrorType("var") is used several times in ForEachLoopBinder. Consider extracting a helper method.

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 looked at a few ways to improve this. Moving that logic into the method that produces inferredType (but that didn't work due to some other consumers). I also tried extracting a method, but I didn't feel it helped.
So I'm leaving as-is, if that's ok.

BoundDeconstructionAssignmentOperator deconstruction = BindDeconstructionDeclaration(
variables,
variables,
right: null,
diagnostics: diagnostics,
rightPlaceholder: valuePlaceholder);
}

private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics, Binder originalBinder)
{
// Use the right binder to avoid seeing iteration variable
BoundExpression collectionExpr = originalBinder.GetBinder(_syntax.Expression).BindValue(_syntax.Expression, diagnostics, BindValueKind.RValue);
BoundExpression collectionExpr = originalBinder.GetBinder(_syntax.Expression).BindValue(_syntax.Expression, diagnostics, BindValueKind.RValue);

ForEachEnumeratorInfo.Builder builder = new ForEachEnumeratorInfo.Builder();
TypeSymbol inferredType;
Expand All @@ -74,33 +111,54 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
(object)builder.MoveNextMethod == null ||
(object)builder.CurrentPropertyGetter == null;

// Check for local variable conflicts in the *enclosing* binder; obviously the *current*
// binder has a local that matches!
var hasNameConflicts = originalBinder.ValidateDeclarationNameConflictsInScope(IterationVariable, diagnostics);

// If the type in syntax is "var", then the type should be set explicitly so that the
// Type property doesn't fail.

TypeSyntax typeSyntax = _syntax.Type;

bool isVar;
AliasSymbol alias;
TypeSymbol declType = BindType(typeSyntax, diagnostics, out isVar, out alias);

TypeSymbol iterationVariableType;
if (isVar)
BoundTypeExpression boundIterationVariableType = null;
Copy link
Member

Choose a reason for hiding this comment

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

Initialization is not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. Thankss

bool hasNameConflicts = false;
BoundForEachDeconstructStep deconstructStep = null;
if (_syntax.IsDeconstructionDeclaration)
{
iterationVariableType = inferredType ?? CreateErrorType("var");

VariableDeclarationSyntax variables = _syntax.DeconstructionVariables;
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, iterationVariableType);
BoundDeconstructionAssignmentOperator deconstruction = BindDeconstructionDeclaration(
variables,
variables,
right: null,
diagnostics: diagnostics,
rightPlaceholder: valuePlaceholder);

deconstructStep = new BoundForEachDeconstructStep(_syntax.DeconstructionVariables, deconstruction, valuePlaceholder);
boundIterationVariableType = new BoundTypeExpression(variables, aliasOpt: null, type: iterationVariableType);
}
else
{
Debug.Assert((object)declType != null);
iterationVariableType = declType;
}
// Check for local variable conflicts in the *enclosing* binder; obviously the *current*
// binder has a local that matches!
hasNameConflicts = originalBinder.ValidateDeclarationNameConflictsInScope(IterationVariable, diagnostics);

// If the type in syntax is "var", then the type should be set explicitly so that the
// Type property doesn't fail.

TypeSyntax typeSyntax = _syntax.Type;

BoundTypeExpression boundIterationVariableType = new BoundTypeExpression(typeSyntax, alias, iterationVariableType);
this.IterationVariable.SetTypeSymbol(iterationVariableType);
bool isVar;
AliasSymbol alias;
TypeSymbol declType = BindType(typeSyntax, diagnostics, out isVar, out alias);

if (isVar)
{
iterationVariableType = inferredType ?? CreateErrorType("var");
}
else
{
Debug.Assert((object)declType != null);
iterationVariableType = declType;
}

boundIterationVariableType = new BoundTypeExpression(typeSyntax, alias, iterationVariableType);
this.IterationVariable.SetTypeSymbol(iterationVariableType);
}

BoundStatement body = originalBinder.BindPossibleEmbeddedStatement(_syntax.Statement, diagnostics);

Expand All @@ -116,6 +174,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
boundIterationVariableType,
this.IterationVariable,
collectionExpr,
deconstructStep,
body,
CheckOverflowAtRuntime,
this.BreakLabel,
Expand Down Expand Up @@ -207,6 +266,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
boundIterationVariableType,
this.IterationVariable,
convertedCollectionExpression,
deconstructStep,
body,
CheckOverflowAtRuntime,
this.BreakLabel,
Expand Down Expand Up @@ -475,7 +535,7 @@ private ForEachEnumeratorInfo.Builder GetDefaultEnumeratorInfo(ForEachEnumerator
}
else
{
builder.ElementType = collectionExprType.SpecialType == SpecialType.System_String?
builder.ElementType = collectionExprType.SpecialType == SpecialType.System_String ?
GetSpecialType(SpecialType.System_Char, diagnostics, _syntax) :
((ArrayTypeSymbol)collectionExprType).ElementType;
}
Expand Down
14 changes: 11 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -816,19 +816,27 @@

<!-- Pieces corresponding to the syntax -->
<!-- This is so the binding API can find produce semantic info if the type is "var". -->
<Field Name="IterationVariableType" Type="BoundTypeExpression"/>
<Field Name="IterationVariable" Type="LocalSymbol"/>
<!-- If this node does not have errors, then this is the foreach expression wrapped
<!-- Information about the iteration variable is only applicable if there is no deconstruction. -->
Copy link
Member

Choose a reason for hiding this comment

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

Isn't IterationVariableType used in the deconstruction 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.

Fixed the comment. Thanks

<Field Name="IterationVariableType" Type="BoundTypeExpression"/>
<Field Name="IterationVariableOpt" Type="LocalSymbol" Null="allow"/>
<!-- If this node does not have errors, then this is the foreach expression wrapped
in a conversion to the collection type used by the foreach loop. The conversion
is here so that the binding API can return the correct ConvertedType in semantic
info. It will be stripped off in the rewriter if it is redundant or causes extra
boxing. If this node has errors, then the conversion may not be present.-->
<Field Name="Expression" Type="BoundExpression"/>
<Field Name="DeconstructionOpt" Type="BoundForEachDeconstructStep" Null="allow" SkipInVisitor="true"/>
<Field Name="Body" Type="BoundStatement"/>

<Field Name="Checked" Type="Boolean"/>
</Node>

<!-- All the information need to apply a deconstruction at each iteration of a foreach loop involving a deconstruction-declaration. -->
<Node Name="BoundForEachDeconstructStep" Base="BoundNode">
<Field Name="DeconstructionAssignment" Type="BoundDeconstructionAssignmentOperator" Null="disallow"/>
<Field Name="TargetPlaceholder" Type="BoundDeconstructValuePlaceholder" Null="disallow"/>
</Node>

<Node Name="BoundUsingStatement" Base="BoundStatement">
<!-- DeclarationsOpt and ExpressionOpt cannot both be non-null. -->
<Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;"/>
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/Statement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public override void Accept(OperationVisitor visitor)

internal partial class BoundForEachStatement : IForEachLoopStatement
{
ILocalSymbol IForEachLoopStatement.IterationVariable => this.IterationVariable;
ILocalSymbol IForEachLoopStatement.IterationVariable => this.IterationVariableOpt;

IOperation IForEachLoopStatement.Collection => this.Expression;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ private static Binder GetEnclosingBinder(CSharpSyntaxNode node, int position, Bi
{
binder = rootBinder.GetBinder(current);
}
else if (current.Kind() == SyntaxKind.VariableDeclaration &&
(current.Parent as ForEachStatementSyntax)?.DeconstructionVariables == current)
{
binder = rootBinder.GetBinder(current.Parent);
}
else
{
// If this ever breaks, make sure that all callers of
Expand Down
5 changes: 2 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1075,7 +1075,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, RefKind

case BoundKind.ForEachStatement:
{
var iterationVariable = ((BoundForEachStatement)node).IterationVariable;
var iterationVariable = ((BoundForEachStatement)node).IterationVariableOpt;
Debug.Assert((object)iterationVariable != null);
int slot = GetOrCreateSlot(iterationVariable);
if (slot > 0) SetSlotState(slot, written);
Expand Down Expand Up @@ -1728,7 +1728,6 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct

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

Expand Down Expand Up @@ -2055,7 +2054,7 @@ public override BoundNode VisitEventAccess(BoundEventAccess node)

public override void VisitForEachIterationVariable(BoundForEachStatement node)
{
var local = node.IterationVariable;
var local = node.IterationVariableOpt;
if ((object)local != null)
{
GetOrCreateSlot(local);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ private Symbol GetNodeSymbol(BoundNode node)

case BoundKind.ForEachStatement:
{
return ((BoundForEachStatement)node).IterationVariable;
return ((BoundForEachStatement)node).IterationVariableOpt;
}

case BoundKind.RangeVariable:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public override BoundNode VisitUnboundLambda(UnboundLambda node)

public override void VisitForEachIterationVariable(BoundForEachStatement node)
{
var local = node.IterationVariable;
var local = node.IterationVariableOpt;
if ((object)local != null)
{
GetOrCreateSlot(local);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public override BoundNode VisitForEachStatement(BoundForEachStatement node)
{
if (IsInside)
{
_variablesDeclared.Add(node.IterationVariable);
_variablesDeclared.Add(node.IterationVariableOpt);
}

return base.VisitForEachStatement(node);
Expand Down
Loading