Binding for deconstruction-declaration in 'foreach' statement #12326

jcouv · 2016-07-02T01:15:55Z

Adds support for for (var (x, y) in ...) { ... }.

Overview of the design:

Initial binding
Add more information into the BoundForEachStatement describing the deconstruction step. That information is a bound deconstruction assignment (BoundDeconstructionAssignmentOperator) with a placeholder for the expression to be deconstructed.
The ForEachLoopBinder declares more locals from its BuildLocals method. Some of them may need inference, which InferTypeOfVarVariable in SourceLocalSymbol.cs can handle.
Local rewriter
There are four cases for foreach: IEnumerable, SZ array, MD array and string. In each case, if the BoundForEachStatement has a deconstruction step, the current value for the iteration variable is deconstructed to produce multiple locals.

I ran into some issues with the GetDeclaredSymbol API on those deconstruction locals, because the syntax for the deconstruction variables fall into the PatternVariableBinder but not the ForEachLoopBinder which builds those locals.
I will chat with @gafter next week to confirm my approach was ok.

I also made some minor updates to the parts of syntax that are attached to deconstruction diagnostics, and cleaned up some PROTOTYPE markers.

@dotnet/roslyn-compiler for review.
Deconstruction work items: #11299

gafter · 2016-07-02T04:31:32Z

It would be nice to have a test that confirms that these foreach loop iteration variables remain readonly, like the ones introduced the old way.

jcouv · 2016-07-02T04:50:06Z

@gafter I didn't know about that behavior. Thanks for pointing that out.

gafter · 2016-07-02T05:04:56Z

I will chat with @gafter next week to confirm my approach was ok.

I will be working from home, but I'm happy to chat with you whenever.

gafter · 2016-07-02T05:07:02Z

@jcouv The readonly behavior would be my suggestion for these (since foreach variables are readonly) but I can imagine the LDM deciding either way. So don't necessarily rush to change it from whatever it is naturally in this implementation. Rather, note it as an issue.

cston · 2016-07-02T06:38:32Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

@@ -645,6 +649,11 @@ protected override TypeSymbol InferTypeOfVarVariable(DiagnosticBag diagnostics)
                        loopBinder.BindDeconstructionDeclaration(forStatement.Declaration, forStatement.Declaration, diagnostics);
                        break;

+                    case SyntaxKind.ForEachStatement:
+                        var foreachBinder = this.binder.GetBinder((ForEachStatementSyntax)statement);
+                        foreachBinder.BindForEachDeconstruction(diagnostics, foreachBinder);


Isn't this.binder the ForEachBinder? If so, ForEachBinder.BindForEachDeconstruction can be called directly and the method does not need to be virtual.

I'm not entirely sure, but Aleksey warned against taking such shortcuts in this comment on my last PR.
In terms of binders, the above is staying as close as possible to what happens with BindForEach (before it calls into BindForEachDeconstruction).

jcouv · 2016-07-03T02:46:30Z

The last commit fixes the circularity problem, using ImplicitlyTypedLocalBinder (the same way it's done elsewhere in SourceLocalSymbol code.

jcouv · 2016-07-05T17:35:47Z

@CyrusNajmabadi @dotnet/roslyn-ide The last commit contains a fix for formatting deconstruction-declaration syntax, if you want to review.
The proper formatting is var (x, y) = ... (with space after "var") rather than var(x, y) = ... (without space). This is independent of settings.

VSadov · 2016-07-05T19:40:26Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+                Assert.Null(model.GetSymbolInfo(x12Var.Type).Symbol); // The var in `var (x1, x2)` has no symbol
+            };
+
+            var comp = CompileAndVerify(source, expectedOutput: "1 2 - 3 4 -", additionalRefs: new[] { ValueTupleRef, SystemRuntimeFacadeRef }, sourceSymbolValidator: validator);


we need to check IL here to make sure we are using array indexing directly.

Yes, I'll add that. I did verify manually in debugger though.

VSadov · 2016-07-05T20:02:38Z

LGTM

jcouv · 2016-07-05T22:35:44Z

@cston When you have some time, a second review would be great.

cston · 2016-07-05T23:14:35Z

src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs

+            bool hasErrors = !GetEnumeratorInfoAndInferCollectionElementType(ref builder, ref collectionExpr, diagnostics, out inferredType);
+
+            VariableDeclarationSyntax variables = _syntax.DeconstructionVariables;
+            var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, inferredType ?? CreateErrorType("var"));


The expression inferredType ?? CreateErrorType("var") is used several times in ForEachLoopBinder. Consider extracting a helper method.

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.

cston · 2016-07-06T00:11:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+        }
+
+        [Fact]
+        public void ForeachIEnumerableDeclarationWithImplicitVarType()


This is more frequently ForEach in the CSharp projects. Consider changing the new instances.

Fixed the casing

cston · 2016-07-06T04:29:11Z

LGTM

jcouv · 2016-07-06T16:54:39Z

@Pilchie Could I get an IDE code review on this small commit above? 3f48d29
It fixes a spacing issue with formatting deconstruction declarations.

Pilchie · 2016-07-06T17:06:28Z

src/Workspaces/CSharp/Portable/Formatting/Rules/SpacingFormattingRule.cs

@@ -321,6 +321,12 @@ public override AdjustSpacesOperation GetAdjustSpacesOperation(SyntaxToken previ
                return CreateAdjustSpacesOperation(0, AdjustSpacesOption.ForceSpaces);
            }

+            // Always put a space in the var form of deconstruction-declaration
+            if (currentKind == SyntaxKind.OpenParenToken && currentToken.Parent.Kind() == SyntaxKind.VariableDeconstructionDeclarator && previousToken.Kind() == SyntaxKind.IdentifierToken && previousToken.ValueText == "var")


I'm slightly concerned that Parent can be null here, if the formatting API is asked to format just a random stream of tokens.

@heejaechang does that sound possible to you? @jcouv if so, you might consider using currentToken.IsParentKind(SyntaxKind.VariableDeconstructionDeclarator) instead.

I don't think you need to check the valuetext of previous token. I would just check if the parent is a deconstruction, then see if the deconstruction has a non-null type.

Thanks!
I didn't find an IsParentKind extension that takes a SyntaxToken (they only take SyntaxNode), so I inlined a null check instead.

@CyrusNajmabadi I pushed a new commit which doesn't rely on the ValueText of the previous token as you suggested. Also adding a test for nested formatting.

Binding for deconstruction-declaration in 'foreach' statement

e177f2a

jcouv added Area-Compilers Feature - Tuples Tuples labels Jul 2, 2016

jcouv self-assigned this Jul 2, 2016

Test that foreach deconstruction variables are readonly

e7cd9f8

Adding tests

d6fe678

cston reviewed Jul 2, 2016
View reviewed changes

Fixing circularity problem

52b8bde

jcouv added 3 commits July 5, 2016 09:21

Adding formatting test

b44a59c

Adding IL test

f9c5a43

Fix space formatting issue with var(x, y)

3f48d29

VSadov reviewed Jul 5, 2016
View reviewed changes

PR feedback (1)

4cc036c

cston reviewed Jul 5, 2016
View reviewed changes

cston reviewed Jul 6, 2016
View reviewed changes

PR feedback (2)

2f22693

Pilchie reviewed Jul 6, 2016
View reviewed changes

jcouv added 2 commits July 6, 2016 10:44

Adding null check in formatting code

7231a69

Refining formatting logic

7d76302

jcouv merged commit 269bde0 into dotnet:features/tuples Jul 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binding for deconstruction-declaration in 'foreach' statement #12326

Binding for deconstruction-declaration in 'foreach' statement #12326

jcouv commented Jul 2, 2016 •

edited

Loading

gafter commented Jul 2, 2016

jcouv commented Jul 2, 2016

gafter commented Jul 2, 2016

gafter commented Jul 2, 2016

cston Jul 2, 2016

jcouv Jul 3, 2016

jcouv commented Jul 3, 2016

jcouv commented Jul 5, 2016

VSadov Jul 5, 2016

jcouv Jul 5, 2016

VSadov commented Jul 5, 2016

jcouv commented Jul 5, 2016

cston Jul 5, 2016

jcouv Jul 6, 2016

cston Jul 6, 2016

jcouv Jul 6, 2016

cston commented Jul 6, 2016

jcouv commented Jul 6, 2016

Pilchie Jul 6, 2016

CyrusNajmabadi Jul 6, 2016

jcouv Jul 6, 2016

jcouv Jul 6, 2016

Binding for deconstruction-declaration in 'foreach' statement #12326

Binding for deconstruction-declaration in 'foreach' statement #12326

Conversation

jcouv commented Jul 2, 2016 • edited Loading

gafter commented Jul 2, 2016

jcouv commented Jul 2, 2016

gafter commented Jul 2, 2016

gafter commented Jul 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jul 3, 2016

jcouv commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Jul 5, 2016

jcouv commented Jul 5, 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

cston commented Jul 6, 2016

jcouv commented Jul 6, 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

jcouv commented Jul 2, 2016 •

edited

Loading