Fix syntax associated with bound block of foreach

[tmat]

Fixes #1443.

[tmat]

/cc @gafter @VSadov

[VSadov]

LGTM

[gafter]

This is not correct. The scope is the controlled statement only (there is a separate instance of the variable per iteration).

[tmat]

You are right, I'll change the comment. Actually, now I'm wondering how many closures are going to create in case of

foreach (var x in ...) { var y = 1; () => x, () => y; }

[tmat]

And the answer is - we create 2 closures (display classes). So the change is correct - we need to associate this block with a syntax distinct from the body of the statement (which represents a different closure). Just the comment is wrong.

[gafter]

What I mean is that the code is incorrect (the comment correctly describes the incorrect behavior that is implemented here). The extent of the for-each variable on this block is the same as the controlled statement, not the foreach statement itself. Your change attaches a syntax node that has the wrong extent.

This will become a worse issue in C# 7, when we expect to have variables introduced in any statement that has a subexpression (e.g. declaration expressions or pattern matching). At that time I would expect this code to require revision. Better to attach a syntax node that has the right extent to start with.

The display class created for the foreach control variable could be shared with the closure for any variables declared in the controlled statement. But it could not be the same as for any variables declared in the expression of the foreach loop.

It feels to me as if the issue is the assertion, not the code.

[tmat]

First of all, for EnC mapping to work no two BoundBlocks that represent a closure in the lambda rewriter can be associated with the same syntax node. If they were the mapping would be broken.

You are correct that once expression-level declarations are introduced we need to amend the implementation of IsClosureScope (and of other code in EnC infrastructure). And that is true regardless of what node this particular BoundBlock is associated with.
For now we have two options:

1. The small change implemented in this PR.
2. A bigger change of updating the code-gen to not emit a distinct display class and instead share the closure with variables of the embedded statement.

I'm not against implementing [2] for RTM. I believe [1] is better for RC, since it's a less risky change.

BTW, the node associated with the BoundBlock does not need to exactly describe the scope of the variable. It does however need to uniquely represent it, meaning that it should be possible to unambiguously derive the exact span from the associated node. Given scope associated with a node of type ForEachStatementSyntax we know that the exact scope being represented spans the embedded statement.

[gafter]

We should not have assertions about the shape of the language (syntax) in any compiler phases after lowering. The purpose of lowering is to abstract away language-specific shapes into less language-specific shapes.

The bound node itself already uniquely identifies the scope.

[tmat]

Well, that's great theory but doesn't quite help me to implement EnC.

[gafter]

Assertions in the lowering phases won't help you implement EnC.

It is possible you may need to record additional info in the bound trees (and the generated debug info) to implement EnC.

[tmat]

They absolutely help me to implement EnC. IsClosureScope is called from the IDE to find out what syntax corresponds to display classes (BoundBlocks). So the assertion is making sure that the IDE and the compiler are in sync. I agree we might want to consider improving the way how to make this information available to the IDE in future (perhaps exposing some public APIs, semantic trees, etc.). We can revisit this for C# 7. For the current version I'm not gonna change the fundamentals of the EnC infrastructure.

[gafter]

I suggest you just change IsClosureScope to return true for any statement.

[tmat]

@gafter @VSadov Added more detailed explanation to the rewriter and more tests to cover all 4 kinds of foreach loops.

[gafter]

As we discussed offline, this solution is good for now, but we'll need to introduce a nonterminal around the identifier in a foreach loop to which the block and its associated closure can be associated.

[tmat]

After an offline discussion we agreed to leave the change as is for RC, and consider changing the structure of foreach statement syntax for RTM (see #1503).

[gafter]

LGTM