D-declaration should use declaration expressions #14871

jcouv · 2016-11-01T23:12:13Z

This implements the syntax changes (not the wildcard part) of #14832.
The first commit has the changes to the parser and parsing tests. The second commit has the rest (binding and semantic model).

From our discussion we could keep the following restrictions for now:

only parse the declaration form of deconstruction when it is a standalone statement (as today), which means we wouldn't parse M((int x, int y) = e); for now (I will do that next)
no support for mixing declarations and assignments in one deconstruction, such as (x, var y) = e;

On the other hand, deconstruction declaration is now allowed as an embedded statement.

Some follow-ups:

merge the parsing logic (and some of the binding logic) between d-assignment and d-declaration (which will allow mixing at some point)
the logic for parsing d-assignment and d-declaration is still separate (we can merge later). Some of the binding logic could also be factored further.
GetTypeInfo on tuple expression in the left-hand-side of d-assignment and d-declaration.

gafter · 2016-11-02T17:21:58Z

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

@@ -1894,23 +1869,6 @@
  </Node>


Could you please remove the commented definition of WildcardVariableDesignationSyntax, since we no longer expect to do things that way? #Resolved

gafter · 2016-11-02T17:25:29Z

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

    <Kind Name="ForEachComponentStatement"/>
    <Field Name="ForEachKeyword" Type="SyntaxToken" Override="true">
      <Kind Name="ForEachKeyword"/>
    </Field>
    <Field Name="OpenParenToken" Type="SyntaxToken" Override="true">
      <Kind Name="OpenParenToken"/>
    </Field>
-    <Field Name="VariableComponent" Type="VariableComponentSyntax"/>
+    <Field Name="VariableComponent" Type="ExpressionSyntax"/> <!-- Should this be something more specific? TODO REVIEW -->


Since it could either be a tuple literal or a declaration expression, I don't think there is anything else more specific it could be. We probably will want to allow a simple wildcard here too (e.g. foreach (_ in e) {}), perhaps later if not now, so better not to be too restrictive on the type of this field.

Probably rename to Variable? #Resolved

gafter · 2016-11-02T17:25:58Z

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

@@ -2153,15 +2110,15 @@
  <!-- We name this "DeclarationForEachStatementSyntax" because it can express existing foreach
       loops. We may elect to represent all foreach loops using this node and deprecate (stop parsing
       into) the old one. -->
-  <Node Name="ForEachComponentStatementSyntax" Base="CommonForEachStatementSyntax">
+  <Node Name="ForEachComponentStatementSyntax" Base="CommonForEachStatementSyntax"> <!-- Change name TODO REVIEW -->


Possibly ForEachVariableStatementSyntax? #Resolved

gafter · 2016-11-02T17:32:23Z

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

@@ -1131,7 +1131,8 @@
  </Node>
  <Node Name="DeclarationExpressionSyntax" Base="ExpressionSyntax">
    <Kind Name="DeclarationExpression"/>
-    <Field Name="VariableComponent" Type="VariableComponentSyntax">
+    <Field Name="Type" Type="TypeSyntax" Optional="true"/>
+    <Field Name="Designation" Type="VariableDesignationSyntax">
      <PropertyComment>
        <summary>Declaration representing the variable declared in an out parameter.</summary>


Probably want to make this comment slightly more expansive:

<summary>Declaration representing the variable declared in an out parameter or deconstruction.</summary>

#Resolved

gafter · 2016-11-02T18:30:17Z

Looks good so far. #Resolved

jcouv · 2016-11-03T16:50:45Z

With this second commit, the compiler tests are passing.
I'm still working on editor features (refactoring, formatting, and EnC). #Resolved

jcouv · 2016-11-04T18:31:42Z

...udio/CSharp/Impl/Debugging/CSharpProximityExpressionsService.RelevantExpressionsCollector.cs

-                            foreach (var v in t.Variables) AddVariableExpressions(component, expressions);
+                            var t = (TupleExpressionSyntax)component;
+                            foreach (ArgumentSyntax a in t.Arguments) AddVariableExpressions(a.Expression, expressions);
+                            // TODO REVIEW I think there was a bug there. Are we missing tests?


@gafter I'll stop by to discuss. #Closed

jcouv · 2016-11-04T18:32:04Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

+            //    issueSpan = candidateIssueSpan;
+            //    return true;
+            //}
+            // TODO REVIEW Are we lacking tests? It seems this code should be needed for DeclarationExpression


Still investigating how to hit this code. #Resolved

Please turn your TODO comments into PROTOTYPE(wildcard) comments so we can check this in and treat it as an open issue.

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

jcouv · 2016-11-04T18:32:53Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

+                if (operatorToken.Kind() == SyntaxKind.EqualsToken &&
+                    (left.Kind() == SyntaxKind.TupleExpression || left.Kind() == SyntaxKind.DeclarationExpression))
+                {
+                    // TODO REVIEW Once GetTypeInfo works on the left-hand-side expression in a deconstruction declaration,


I'll replace this with reference to github issue once we decide on semantic model behavior. #Resolved

jcouv · 2016-11-04T18:34:25Z

@dotnet/roslyn-compiler This is ready for review. There is one failing test (related to EE) which I'm still investigating. Thanks #Resolved

AlekseyTs · 2016-11-08T06:23:45Z

src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs

@@ -171,7 +171,7 @@ private void CheckOutVarDeclaration(BoundLocal node)
        {
            if (IsInside &&
                !node.WasCompilerGenerated && node.Syntax.Kind() == SyntaxKind.DeclarationExpression &&
-                ((DeclarationExpressionSyntax)node.Syntax).Identifier() == node.LocalSymbol.IdentifierToken)
+                ((SingleVariableDesignationSyntax)((DeclarationExpressionSyntax)node.Syntax).Designation).Identifier == node.LocalSymbol.IdentifierToken)


((SingleVariableDesignationSyntax)((DeclarationExpressionSyntax)node.Syntax).Designation).Identifier == node.LocalSymbol.IdentifierToken [](start = 16, length = 136)

Should probably strengthen the check by making sure it is an out argument. #Closed

AlekseyTs · 2016-11-08T06:36:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/DeconstructionTests.cs

            comp.VerifyDiagnostics(
-                // (7,13): error CS1023: Embedded statement cannot be a declaration or labeled statement
-                //             var (x, y) = (1, 2);
-                Diagnostic(ErrorCode.ERR_BadEmbeddedStmt, "var (x, y) = (1, 2);").WithLocation(7, 13)


Diagnostic(ErrorCode.ERR_BadEmbeddedStmt, "var (x, y) = (1, 2);").WithLocation(7, 13) [](start = 16, length = 85)

I think we might still want to report this error. #Closed

cston · 2016-11-08T18:39:36Z

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

+                    if (expression.Kind == SyntaxKind.SimpleAssignmentExpression)
+                    {
+                        var assignment = (AssignmentExpressionSyntax)expression;
+                        if (assignment.Left.EnumerateNodes().Any(x => x.RawKind == (int)SyntaxKind.DeclarationExpression))


Could assignment.Left contain a lambda with a declaration?

while (true) F(() => { (int x, int y) = (1, 2); }).x = null; ``` #Resolved

Can we simplify the check by simply checking if the left is a tuple or a declaration expression? #Closed

Probably not, tuple expression can mean deconstruction assignment without declaration. #Closed

AlekseyTs · 2016-11-08T18:41:56Z

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

@@ -1131,13 +1131,14 @@
  </Node>
  <Node Name="DeclarationExpressionSyntax" Base="ExpressionSyntax">
    <Kind Name="DeclarationExpression"/>
-    <Field Name="VariableComponent" Type="VariableComponentSyntax">
+    <Field Name="Type" Type="TypeSyntax" Optional="true"/>


Optional="true" [](start = 40, length = 16)

I thought the type is not optional. #Closed

AlekseyTs · 2016-11-08T18:44:44Z

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

+        /// <summary>
+        /// Checks if we can find at least one type in the deconstruction variables
+        /// </summary>
+        private static bool TypeFoundInDeconstructionDeclarationVariables(ExpressionSyntax node)


TypeFoundInDeconstructionDeclarationVariables [](start = 28, length = 45)

I am not sure if this check is relevant now. Shouldn't we always require Tuples Feature when deconstruction is used? #Closed

This is not related to the tuples features. There are two heuristics to be confident that the user typed a deconstruction declaration:

we must find the expected following token (typically '=' but could be 'in' for in foreach context),

the variables must include at least one specified type. That's the TypeFoundInDeconstructionDeclarationVariables method. #Resolved

AlekseyTs · 2016-11-08T18:45:32Z

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

                    {
-                        var syntax = (TypedVariableComponentSyntax)node;
+                        var syntax = (DeclarationExpressionSyntax)node;
                        if (syntax.Type.IsMissing || syntax.Designation.IsMissing) return false;


(syntax.Type.IsMissing || syntax.Designation.IsMissing) [](start = 27, length = 55)

Can this ever be true now? #Closed

Yes, if we try to parse (x, var y) = e; as a d-declaration, then the type on x will be missing.
Similarly, in (int, var y) = e;, the designation will be missing. #Resolved

AlekseyTs · 2016-11-08T19:09:15Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+                {
+                    if (parent.Parent?.Kind() == SyntaxKind.TupleExpression)
+                    {
+                        expr = (TupleExpressionSyntax)parent.Parent;


(TupleExpressionSyntax) [](start = 31, length = 23)

It looks like the cast is not necessary. #Closed

A cast is needed, at least to ExpressionSyntax (the type of expr). #Resolved

Right, never mind then. #Closed

AlekseyTs · 2016-11-08T19:13:46Z

src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs

-                _variablesDeclared.Add(node.LocalSymbol);
+                var declaration = (DeclarationExpressionSyntax)node.Syntax;
+                if (((SingleVariableDesignationSyntax)declaration.Designation).Identifier == node.LocalSymbol.IdentifierToken &&
+                    ((ArgumentSyntax)declaration.Parent).RefOrOutKeyword.Kind() == SyntaxKind.OutKeyword)


declaration.Parent [](start = 37, length = 18)

Probably should ensure that the cast is safe (check the kind) and that the Parent is not null. #Closed

AlekseyTs · 2016-11-08T19:27:41Z

        var (x, y) = (1, 2);

Consider adding a test with deconstruction assignment without declaration. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DeconstructionTests.cs:2445 in 7400a9f. [](commit_id = 7400a9f, deletion_comment = False)

gafter · 2016-11-08T19:29:23Z

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

                    statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt);
                    break;
+                case SyntaxKind.ExpressionStatement:


I think we should get guidance from Mads as to whether or not an error is deserved in this situation. I thought that it isn't considered a declaration any longer (from the spec point of view) due to the syntax change.

Whichever way you leave this code, please open a separate issue to resolve this. #Resolved

Yes. I will leave the error for now and added a note in #15049 (which tracks allow d-deconstruction in expression context).
My understanding is that we do want to allow d-deconstruction in expression context, so this error would naturally disappear then. #Resolved

jcouv · 2016-11-08T20:01:05Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

@@ -2161,14 +2161,6 @@ private static bool IsThisOrTypeOrNamespace(MemberAccessExpressionSyntax memberA
                return true;
            }

-            //if (simpleName.IsParentKind(SyntaxKind.TypedVariableComponent))


Filed follow-up issue: #15094

jcouv · 2016-11-08T20:03:38Z

...udio/CSharp/Impl/Debugging/CSharpProximityExpressionsService.RelevantExpressionsCollector.cs

@@ -136,7 +136,6 @@ public override void VisitSwitchStatement(SwitchStatementSyntax node)
                        {
                            var t = (TupleExpressionSyntax)component;
                            foreach (ArgumentSyntax a in t.Arguments) AddVariableExpressions(a.Expression, expressions);
-                            // TODO REVIEW I think there was a bug there. Are we missing tests?


Confirmed with Neal there was a bug.
Filed bug #15095 to improve test coverage

AlekseyTs · 2016-11-08T20:07:24Z

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

+        /// <summary>
+        /// Returns true if the expression is composed only of nested tuple and declaration expressions.
+        /// </summary>
+        private static bool IsDeconstructionDeclarationLeft(ExpressionSyntax node)


IsDeconstructionDeclarationLeft [](start = 28, length = 31)

Is this a dupe of a method from SyntaxExtensions? Consider reusing it instead. #Closed

The logic is the same, but one is on internal nodes, the other on public nodes.

AlekseyTs · 2016-11-08T20:11:18Z

Compiler changes LGTM

CyrusNajmabadi · 2016-11-08T21:16:00Z

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs

+                    }
+                    else
+                    {
+                        goto default;


please do not use gotos in IDE code :)

CyrusNajmabadi · 2016-11-08T21:17:11Z

...udio/CSharp/Impl/Debugging/CSharpProximityExpressionsService.RelevantExpressionsCollector.cs

-                            var t = (ParenthesizedVariableComponentSyntax)component;
-                            foreach (var v in t.Variables) AddVariableExpressions(component, expressions);
+                            var t = (TupleExpressionSyntax)component;
+                            foreach (ArgumentSyntax a in t.Arguments) AddVariableExpressions(a.Expression, expressions);


always use braces.

CyrusNajmabadi · 2016-11-08T21:17:34Z

src/Workspaces/CSharp/Portable/Extensions/SemanticModelExtensions.cs

@@ -235,8 +235,7 @@ private static bool CanBindToken(SyntaxToken token)
                else if (current is DeclarationExpressionSyntax)
                {
                    var decl = (DeclarationExpressionSyntax)current;
-                    var component = decl.VariableComponent as TypedVariableComponentSyntax;
-                    var name = component?.Designation as SingleVariableDesignationSyntax;
+                    var name = decl.Designation as SingleVariableDesignationSyntax;
                    if (name == null) break;


always use braces.

CyrusNajmabadi · 2016-11-08T21:18:02Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

@@ -876,6 +875,13 @@ private IEnumerable<TypeInferenceInfo> InferTypeInBinaryOrAssignmentExpression(E
                    return SpecializedCollections.SingletonEnumerable(new TypeInferenceInfo(this.Compilation.GetSpecialType(SpecialType.System_Boolean)));
                }

+                // Infer type for deconstruction declaration
+                if (operatorToken.Kind() == SyntaxKind.EqualsToken &&
+                    (left.Kind() == SyntaxKind.TupleExpression || left.Kind() == SyntaxKind.DeclarationExpression))


didn't you introduce an IsDeconstructoin helper? Shouldn't that be used here?

Factored the code into an extension in CSharpWorkspace project.

jcouv · 2016-11-08T22:56:12Z

@CyrusNajmabadi I went ahead and merged to unblock Neal. I'll make sure to address any other feedback that you have.

WIP on syntax

dc2fe06

jcouv added Area-Compilers Feature - Tuples Tuples labels Nov 1, 2016

jcouv added this to the 2.0 (RC.2) milestone Nov 1, 2016

jcouv self-assigned this Nov 1, 2016

dnfclas added the cla-already-signed label Nov 1, 2016

jcouv changed the base branch from master to features/wildcard November 1, 2016 23:12

gafter self-assigned this Nov 2, 2016

gafter reviewed Nov 2, 2016

View reviewed changes

gafter removed their assignment Nov 2, 2016

jcouv force-pushed the decon-syntax branch 8 times, most recently from c369441 to 0d95857 Compare November 3, 2016 16:10

Updating binding and semantic model to new syntax

d0f94c3

jcouv force-pushed the decon-syntax branch 3 times, most recently from 3bae047 to 158826d Compare November 4, 2016 18:26

jcouv commented Nov 4, 2016

View reviewed changes

Updating editor features to adopt new syntax model

ecce74d

jcouv force-pushed the decon-syntax branch from 158826d to ecce74d Compare November 4, 2016 18:33

AlekseyTs reviewed Nov 8, 2016

View reviewed changes

jcouv force-pushed the decon-syntax branch 2 times, most recently from 9279a6b to 0ea0754 Compare November 8, 2016 18:18

PR feedback (1)

7400a9f

jcouv force-pushed the decon-syntax branch from 0ea0754 to 7400a9f Compare November 8, 2016 18:36

cston reviewed Nov 8, 2016

View reviewed changes

AlekseyTs reviewed Nov 8, 2016

View reviewed changes

gafter reviewed Nov 8, 2016

View reviewed changes

PR feedback (2)

d5bede6

jcouv commented Nov 8, 2016

View reviewed changes

AlekseyTs reviewed Nov 8, 2016

View reviewed changes

Moving IsDeconstructionDeclarationLeft method

7e4ea6e

CyrusNajmabadi reviewed Nov 8, 2016

View reviewed changes

IDE feedback (1)

d39e75e

jcouv force-pushed the decon-syntax branch from d13a4c8 to d39e75e Compare November 8, 2016 22:17

jcouv merged commit fb5d0b2 into dotnet:master Nov 8, 2016

jcouv deleted the decon-syntax branch November 8, 2016 22:56

jcouv mentioned this pull request Jan 8, 2017

Confirm dead code removal in IsReplaceableByVar was ok #15094

Closed

D-declaration should use declaration expressions #14871

D-declaration should use declaration expressions #14871

Conversation

jcouv commented Nov 1, 2016 • edited Loading

gafter Nov 2, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Nov 2, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Nov 2, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Nov 2, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter commented Nov 2, 2016 • edited by jcouv Loading

jcouv commented Nov 3, 2016 • edited Loading

jcouv Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv commented Nov 4, 2016 • edited Loading

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

cston Nov 8, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 2016 • edited Loading

gafter Nov 8, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 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

Choose a reason for hiding this comment

jcouv commented Nov 8, 2016

jcouv commented Nov 1, 2016 •

edited

Loading

gafter Nov 2, 2016 •

edited by jcouv

Loading

gafter Nov 2, 2016 •

edited by jcouv

Loading

gafter Nov 2, 2016 •

edited by jcouv

Loading

gafter Nov 2, 2016 •

edited by jcouv

Loading

gafter commented Nov 2, 2016 •

edited by jcouv

Loading

jcouv commented Nov 3, 2016 •

edited

Loading

jcouv Nov 4, 2016 •

edited

Loading

jcouv Nov 4, 2016 •

edited

Loading

jcouv Nov 4, 2016 •

edited

Loading

jcouv commented Nov 4, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

cston Nov 8, 2016 •

edited by jcouv

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

jcouv Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

jcouv Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

jcouv Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading

AlekseyTs commented Nov 8, 2016 •

edited

Loading

gafter Nov 8, 2016 •

edited by jcouv

Loading

jcouv Nov 8, 2016 •

edited

Loading

AlekseyTs Nov 8, 2016 •

edited

Loading