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 'for' statement #12302

Merged
merged 2 commits into from
Jun 30, 2016

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jun 30, 2016

Allows for(var(x, y) = M(); ..; ...) { ... }

The change has two main parts:

  1. BindForParts
    If the VariableDeclarationSyntax for the initialization part of the "for" is a deconstruction-declaration, the initializer bound node will be a BoundLocalDeconstructionDeclaration (which is the statement introduced in last PR).
  2. MakeLocals
    When scanning the for syntax for locals, if the VariableDeclaration is a deconstruction-declaration, then collect locals from it.
    Some of those may have an implicit type, which PossiblyImplicitlyTypedDeconstructionLocalSymbol.InferTypeOfVarVariable in SourceLocalSymbols.cs will figure out.

@dotnet/roslyn-compiler for review.
Issue #11299


var result = new BoundLocalDeconstructionDeclaration(node, BindDeconstructionAssignment(node.Declaration.Deconstruction.Value, node.Declaration.Deconstruction.Value, variables, diagnostics));
var result = new BoundLocalDeconstructionDeclaration(statement, BindDeconstructionAssignment(declaration.Deconstruction.Value, declaration.Deconstruction.Value, variables, diagnostics));
Copy link
Member

Choose a reason for hiding this comment

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

Why associate the deconstruction with the entire for statement rather than the Declaration syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a check in BindStatement (in Binder_Statement.cs line 132) that all bound statements should be associated with a statement syntax.
In the for statement, the VariableDeclaration used to initialize the loop is not a statement (it's just an expression).

Copy link
Member

Choose a reason for hiding this comment

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

But isn't that check for the BoundForStatement in this case rather than any BoundLocalDeconstructionDeclaration within that?

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'm looking into this more. The binding for for (int i = 0; ...; ...) { ...} avoids this problem, so I should be able to do the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found a solution :-)

@jaredpar
Copy link
Member

:shipit:

@@ -616,7 +617,7 @@ private class PossiblyImplicitlyTypedDeconstructionLocalSymbol : SourceLocalSymb
#if DEBUG
SyntaxNode parent;
Debug.Assert(SyntaxFacts.IsDeconstructionIdentifier(identifierToken, out parent));
Debug.Assert(parent.Parent?.Kind() == SyntaxKind.LocalDeclarationStatement);
Debug.Assert(parent.Parent?.Kind() == SyntaxKind.LocalDeclarationStatement || parent.Parent?.Kind() == SyntaxKind.ForStatement);
Copy link
Member

Choose a reason for hiding this comment

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

Consider asserting parent.Parent != null separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jcouv
Copy link
Member Author

jcouv commented Jun 30, 2016

Pushed a change with the PR feedback so far and also some circularity tests to be fixed in a later PR (thanks Chuck for those suggestions!).

@cston
Copy link
Member

cston commented Jun 30, 2016

LGTM

@jcouv
Copy link
Member Author

jcouv commented Jun 30, 2016

Thanks!
I'll make sure to test the nested circularity case when I investigate the problem in details.

@jcouv jcouv merged commit 1c53b7e into dotnet:features/tuples Jun 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants