Binding for local deconstruction declaration statement #12224
Conversation
This approach does not match the design guidelines for initial bound nodes. The shape of the initial bound tree is intended to be fairly closely isomorphic to the shape of the syntax. We have separate bound nodes for statements and expressions, and the local declarations remain in the bound tree after initial binding. The deconstruction declaration is not an expression form in the language. It is represented in the syntax as something more akin to a local variable declaration (a statement). It could be translated "via" an expression form during lowering, but it is not syntactically or semantically an expression statement. |
|
@gafter Thanks for pointing this out. I will introduce a new bound node for |
| SeparatedSyntaxList<ArgumentSyntax> arguments = ((TupleExpressionSyntax)node.Left).Arguments; | ||
| ImmutableArray<DeconstructionVariable> checkedVariables = BindDeconstructionVariables(arguments, diagnostics); | ||
| ArrayBuilder<DeconstructionVariable> checkedVariables = BindDeconstructionVariables(arguments, diagnostics); |
There was a problem hiding this comment.
Is BindDeconstructionAssignment method below freeing checkedVariables? Please consider having the same method allocate and free.
There was a problem hiding this comment.
Thanks for pointing this. I think when I switched to ArrayBuilder I removed the freeing and did not put it back. I'll fix.
If you add a field to the new node containing the list of declared variables, I think that is a good solution. |
|
I think all tests for various forms of deconstruction declaration should also test behavior of SemanticModel.GetDeclaredSymbol API for all declared locals. |
|
You will want to test the region analysis APIs for these new variables, but doing that in a separate PR makes sense. |
|
Added semantic model API tests and the initial feedback. |
| return result; | ||
| } | ||
|
|
||
| private void FreeDeconstructionVariables(ArrayBuilder<DeconstructionVariable> variables) |
|
I pushed a commit with most feedback addressed, except: I need to add tests for errors from |
| Debug.Assert(((VariableDeclarationSyntax)topLevelVariableDeclaration).Deconstruction != null); | ||
| Debug.Assert(((VariableDeclarationSyntax)topLevelVariableDeclaration).Deconstruction.Value != null); | ||
|
|
||
| var statement = topLevelVariableDeclaration.Parent; |
There was a problem hiding this comment.
Perhaps extract a GetLocalDeclarationStatement(IdentifierToken) helper method that is used here and in the .ctor above and have that helper method assert there at least one pair of { VariableDeconstructionDeclarator, VariableDeclaration }.
There was a problem hiding this comment.
Refactored all this code to minimize duplication.
| if (((VariableDeclarationSyntax)parent).Deconstruction != null) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
return ((VariableDeclarationSyntax)parent).Deconstruction != null;
|
LGTM |
| /// </summary> | ||
| internal partial class OutVarLocalPendingInference | ||
| { | ||
| public override void Accept(OperationVisitor visitor) | ||
| { | ||
| visitor.VisitNoneOperation(this); | ||
| throw ExceptionUtilities.Unreachable; |
|
Done with review pass. |
|
Pushed a change with addresses the feedback and suggestions. I will add more tests on semantic model (including aliases) as well as the |
|
The last commit makes |
This change adds binding for local deconstruction declaration statements. The for and foreach statements will be added later.
The binding produces a
BoundExpressionStatementwith aBoundDeconstructionAssignmentOperator. The main difference compared to the binding for deconstruction-assignment is that now, variables in theBoundDeconstructionAssignmentOperatorareBoundLocal.Much of the binding still relies on the existing
BindDeconstructionAssignment, except how the variables on the left are prepared. Going intoBindDeconstructionAssignment, the variables are a new type,DeconstructionLocalPendingInference, which during the process of binding the assignment (once the types on the right-hand-side are figured out) get converted toBoundLocalwith the inferred type.Like
OutVarLocalPendingInference,DeconstructionLocalPendingInferencedoesn't survive the early binding process.In order to allow the tree of variables to be updated by inference, I changed the existing
DeconstructionVariableto hold anArrayBuilderinstead ofImmutableArrayfor nested elements.@dotnet/roslyn-compiler for review.