Deconstruction-declaration parsing for local declaration, for and foreach #12104

jcouv · 2016-06-20T17:59:37Z

Adds parsing for three deconstruction-declaration contexts. Other contexts (such as let or from will be added later).

@dotnet/roslyn-compiler for review. Thanks

// All those are parsed as a LocalDeclarationStatement, except that its VariableDeclaration is extended to hold a DeconstructionDeclaration
// DeconstructionDeclaration is DeconstructionVariables + equalsToken + expression
(Int32 a, Int64 b) = foo;
((Int32 a, Int64 b), Int32 c) = foo;
var (a, b) = foo;
var ((a, b), c) = foo;
(Int32 x, var (y, z)) = foo; // var syntax inside list syntax

// This is still a method call
var(a, b);

// ForStatement with the VariableDeclaration holding a DeconstructionDeclaration
for ((Int32 x, Int64 y) = foo; ; ) { }
for (var (x, y) = foo; ; ) { }

// Foreach statement which is extended to hold DeconstructionVariables
foreach ((int x, var y) in foo) { }
foreach (var (x, y) in foo) { }

Note 1: because my VS is busted, I expect some public API errors. I will fix that before checking in.

Note 2: this change includes a copy of Aleksey's recent change in outvar branch to support Internal="true" in Syntax.xml, which will help maintain a backwards compatible public API.

Issue #11299

foreach statements

jaredpar · 2016-06-20T20:28:00Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

            //  foreach ( <type> <identifier> in <expr> ) <embedded-statement>
+            // or
+            //  foreach ( <deconstruction-variables> in <expr> ) <embedded-statement>


The name implies to me that it can only be variables. I assume we can have declarations here as well.

Fixed the comment.

jcouv · 2016-06-20T21:33:56Z

Hold off any reviews. From discussion with Cyrus and Aleksey, I'm going to remove the terminals I introduced (to represent "type id" or "id") and use VariableDeclaration instead.

jcouv · 2016-06-21T16:29:30Z

@dotnet/roslyn-compiler This is ready to review again.

Two examples:

var (x, (y, z)) = expression gets parsed as a VariableDeclaration with type=var, no regular declarators, and the new deconstruction declarator holding the rest.
That deconstruction declarator has two variables, each which is a VariableDeclaration where type is null.
For the first one, the x is inside the regular declarator.
For the second one, the two variables are inside the deconstruction declarator.
(int x, int y) = expression gets parsed as a VariableDeclaration with no type, no regular declarators, and the deconstruction declarator holding everything.
That deconstruction declarator has two variables, each which are held as a VariableDeclaration with type=int and the name of the variable inside the regular declarator.

To recap, VariableDeclaration is used in four ways:

type is var, declarator=null and deconstruction is set. Example: var (...) = ...
type is null, declarator=null and deconstruction is set. Example: (int x, int y)
type is set, declarator is set with one element with just a name and deconstruction is null, as before. Example: int x in (int x, int y) = ...
type is null, declarator is set with one element with just a name and deconstruction is null. Example: y in var (x, y) = ...

cston · 2016-06-21T17:23:50Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs


-                    this.Reset(ref resetPoint);
+                    if ((object)decl == null)


Minor point: CSharpSyntaxNode does not define == so the cast to object is unnecessary.

Fixed here and other similar instances below. Thanks

jcouv · 2016-06-21T21:34:01Z

Just pushed a PR addressing Chuck and Cyrus's feedback.
It removes the new SyntaxKinds, factors ParseDeconstructionListForm and ParseDeconstructionIdentifiers, removes the Internal="true" (and supporting source generation code), and some smaller things.

cston · 2016-06-21T21:44:18Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+
+                    if (this.CurrentToken.Kind != SyntaxKind.CloseParenToken)
+                    {
+                        VariableDeclarationSyntax variable;


variable declaration can be moved inside while.

AlekseyTs · 2016-06-23T17:52:14Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                return null;
+            }
+
+            return CheckFeatureAvailability(result, MessageID.IDS_FeatureTuples);


Please add tests for this check, positive and negative. #Resolved

Added CSharp6 test. Positive tests will be easier once I have some binding, so they will be added in subsequent PRs.

jcouv · 2016-06-23T21:30:31Z

This change to public API is not ready yet. I have more changes coming.

jcouv · 2016-06-23T23:52:53Z

Ok, the latest commit I pushed provides a more reasonable public API. If anything, some APIs could be added for convenience.

cston · 2016-06-24T16:18:51Z

docs/features/deconstruction.md

+    ;
+
+for_statement
+    : 'for' '(' for_initializer? ';' for_condition? ';' for_iterator? ')' embedded_statement
    ;


Has for_statement changed or is the same as without deconstruction variables?

In the grammar, I kept for_statement as before. The difference appears below, in for_initializer.

cston · 2016-06-24T17:35:05Z

What kind of parse errors do we report for contexts where deconstruct is not allowed? For instance using ((x, y) = F()) { ... }. Is it necessary to report good errors in these cases?

cston · 2016-06-24T17:35:11Z

LGTM

AlekseyTs · 2016-06-24T18:04:57Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

@@ -1809,11 +1809,29 @@
      <Kind Name="SemicolonToken"/>
    </Field>
  </Node>
-  <Node Name="VariableDeclarationSyntax" Base="CSharpSyntaxNode">
+
+  <Node Name="VariableDeconstructionDeclaratorSyntax" Base="CSharpSyntaxNode" AvoidAutoCreation="true" InternalConstructor="true">


Should this say InternalFactory rather than constructor? #Resolved

Sure. Renamed.

jcouv · 2016-06-24T19:00:16Z

@cston, I will take a note on your question (should we report errors for un-supported contexts?).

AlekseyTs · 2016-06-24T19:36:04Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -117,6 +119,20 @@ Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.Update(Microsoft.CodeAnalys
 Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithCloseParenToken(Microsoft.CodeAnalysis.SyntaxToken closeParenToken) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax
 Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithElements(Microsoft.CodeAnalysis.SeparatedSyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax> elements) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax
 Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax.WithOpenParenToken(Microsoft.CodeAnalysis.SyntaxToken openParenToken) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleTypeSyntax
+Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeclarationSyntax.Deconstruction.get -> Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeconstructionDeclaratorSyntax
+Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeclarationSyntax.WithDeconstruction(Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeconstructionDeclaratorSyntax deconstruction) -> Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeclarationSyntax


I would expect this method to remove regular variables and even type when it has an incompatible value. Does it?

Null input should probably be disallowed as well.

In reply to: 68451172 [](ancestors = 68451172)

An existing VariableDeclarationSyntax.WithVariables should probably be also adjusted accordingly.

In reply to: 68451298 [](ancestors = 68451298,68451172)

I added two work items to track this:

Add validation to factory methods (only allow creating valid combinations with reasonable-looking children)

Add validation logic or erasure to WithXYZ methods

Thanks for raising the questions.

AlekseyTs · 2016-06-24T21:32:01Z

LGTM

jcouv · 2016-06-24T21:55:33Z

Thanks both for the review and feedback. I'll go ahead with merge.

Deconstruction-declaration parsing for local declaration, for and

62d0b88

foreach statements

jcouv added Area-Compilers New Language Feature - Tuples Tuples labels Jun 20, 2016

jcouv self-assigned this Jun 20, 2016

dnfclas added the cla-already-signed label Jun 20, 2016

jaredpar reviewed Jun 20, 2016
View reviewed changes

jcouv added 2 commits June 20, 2016 22:22

Switching to new syntax model

219bc7c

Minor improvements

1160bc7

Improving comments

2f53b73

cston reviewed Jun 21, 2016
View reviewed changes

PR feedback (1)

13736b8

cston reviewed Jun 21, 2016
View reviewed changes

AlekseyTs reviewed Jun 23, 2016
View reviewed changes

PR feedback (4)

e96585a

jcouv force-pushed the d-declaration branch from 9c7805f to b4e7107 Compare June 23, 2016 21:19

Refining public API

9ae475a

jcouv force-pushed the d-declaration branch from b4e7107 to 9ae475a Compare June 23, 2016 23:48

Removing accidentally-added files

de691fe

cston reviewed Jun 24, 2016
View reviewed changes

AlekseyTs reviewed Jun 24, 2016
View reviewed changes

PR feedback (5)

8c627ba

AlekseyTs reviewed Jun 24, 2016
View reviewed changes

jcouv merged commit 78cb91d into dotnet:features/tuples Jun 24, 2016

jcouv deleted the d-declaration branch June 24, 2016 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deconstruction-declaration parsing for local declaration, for and foreach #12104

Deconstruction-declaration parsing for local declaration, for and foreach #12104

jcouv commented Jun 20, 2016 •

edited

Loading

jaredpar Jun 20, 2016

jcouv Jun 21, 2016

jcouv commented Jun 20, 2016

jcouv commented Jun 21, 2016

cston Jun 21, 2016

jcouv Jun 21, 2016

jcouv commented Jun 21, 2016

cston Jun 21, 2016

jcouv Jun 22, 2016

AlekseyTs Jun 23, 2016 •

edited

Loading

jcouv Jun 23, 2016 •

edited

Loading

jcouv commented Jun 23, 2016

jcouv commented Jun 23, 2016

cston Jun 24, 2016

jcouv Jun 24, 2016

cston commented Jun 24, 2016

cston commented Jun 24, 2016

AlekseyTs Jun 24, 2016 •

edited

Loading

jcouv Jun 24, 2016

jcouv commented Jun 24, 2016

AlekseyTs Jun 24, 2016

AlekseyTs Jun 24, 2016

AlekseyTs Jun 24, 2016

jcouv Jun 24, 2016

AlekseyTs commented Jun 24, 2016

jcouv commented Jun 24, 2016

Deconstruction-declaration parsing for local declaration, for and foreach #12104

Deconstruction-declaration parsing for local declaration, for and foreach #12104

Conversation

jcouv commented Jun 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 20, 2016

jcouv commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jun 23, 2016

jcouv commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Jun 24, 2016

cston commented Jun 24, 2016

AlekseyTs Jun 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 24, 2016

jcouv commented Jun 24, 2016

jcouv commented Jun 20, 2016 •

edited

Loading

AlekseyTs Jun 23, 2016 •

edited

Loading

jcouv Jun 23, 2016 •

edited

Loading

AlekseyTs Jun 24, 2016 •

edited

Loading