-
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
Binding for deconstruction-declaration in 'foreach' statement #12326
Changes from 2 commits
e177f2a
e7cd9f8
d6fe678
52b8bde
b44a59c
f9c5a43
3f48d29
4cc036c
2f22693
7231a69
7d76302
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.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ private SourceLocalSymbol IterationVariable | |
{ | ||
get | ||
{ | ||
return (SourceLocalSymbol)this.Locals[0]; | ||
return _syntax.IsDeconstructionDeclaration ? null : (SourceLocalSymbol)this.Locals[0]; | ||
} | ||
} | ||
|
||
|
@@ -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> | ||
|
@@ -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")); | ||
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; | ||
|
@@ -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; | ||
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. Initialization is not necessary. 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. 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); | ||
|
||
|
@@ -116,6 +174,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics, | |
boundIterationVariableType, | ||
this.IterationVariable, | ||
collectionExpr, | ||
deconstructStep, | ||
body, | ||
CheckOverflowAtRuntime, | ||
this.BreakLabel, | ||
|
@@ -207,6 +266,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics, | |
boundIterationVariableType, | ||
this.IterationVariable, | ||
convertedCollectionExpression, | ||
deconstructStep, | ||
body, | ||
CheckOverflowAtRuntime, | ||
this.BreakLabel, | ||
|
@@ -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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. --> | ||
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. Isn't 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. 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<LocalSymbol>"/> | ||
|
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.
The expression
inferredType ?? CreateErrorType("var")
is used several times inForEachLoopBinder
. Consider extracting a helper method.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.
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.