RFR: 8341782: Allow lambda capture of basic for() loop variables as with enhanced for()

[Archie Cobbs]

On Wed, 9 Oct 2024 21:23:29 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> * We would need a new flag that says "we are currently in the for() loop body" and not, for example, in the step, where we should leave the flag alone.
>> 
>> Well, my idea for doing that was to just set the flag on the loop variables before scanning the loop body (as you seem to do in the patch you show)
>> 
>>> 
>>> * We would also need to ensure we don't get confused with nested `for()` loops. So the aforementioned flag can't just be a boolean (because otherwise which `for()` loop are we talking about?). For example, a symbol address range instead.
>> 
>> This I don't get. If you have a loop nested inside another loop, you want assignments in the inner loop to affect whether the outer loop variable is considered a "loop capturable variable" or not. So I'm not sure - it seems to me `letInit` should happily remove flags for any variable it sees marked with `FOR_LOOP_BODY_MAY_CAPTURE` (as that is surely a variable of a for loop and we must be in that loop's body for the variable to have that flag set?)
>
>> So I'm not sure - it seems to me `letInit` should happily remove flags for any variable it sees marked with `FOR_LOOP_BODY_MAY_CAPTURE` (as that is surely a variable of a for loop and we must be in that loop's body for the variable to have that flag set?)
> 
> Concretely, why not, in `letInit`:
> 
> 
> // In basic for() loop bodies, variables that are reassigned can no longer be captured
> if ((sym.flags_field & FOR_LOOP_BODY_MAY_CAPTURE) != 0 && !uninits.isMember(sym.adr)) {
>     sym.flags_field &= ~FOR_LOOP_BODY_MAY_CAPTURE;
> }

> This I don't get. If you have a loop nested inside another loop, you want assignments in the inner loop to affect whether the outer loop variable is considered a "loop capturable variable" or not. So I'm not sure - it seems to me `letInit` should happily remove flags for any variable it sees marked with `FOR_LOOP_BODY_MAY_CAPTURE` (as that is surely a variable of a for loop and we must be in that loop's body for the variable to have that flag set?)

I think you are correct, but I think we're referring to different flags. I was referring to this flag:

> We would need a new flag that says "we are currently in the for() loop body"

That's would be a flag in `AssignAnalyzer`, not `Symbol`. Sorry for the mixing of terminology.

To be clear, here's the (first) problem I was worried about:

for (int i = 1; i <= 3; i++) {
    for (int j = 1; j <= 3; j++) {
        Runnable r = () -> System.out.println(j);   // false error here
    }
}

Explanation: We set the boolean flag `WeAreCurrentlyInTheForLoopBody` just before entering the outer loop body. Then when entering the inner loop step (which occurs prior entering to the inner loop body), the flag is still set, and so then we would incorrectly mark `j` has being reassigned in the body - but it's the wrong body that the flag is referring to. We're in the outer loop body but not the inner loop body.

This could be addressed by a stash/pop bracketing of the `WeAreCurrentlyInTheForLoopBody` flag around the _entire_ `visitForLoop()` method instead of only around the recursion into the body.

But then you still have a second problem, as shown in this example:


for (int i = 1; i <= 3; i++) {
    for (int j = 1; j <= 3; i++, j++) {
        Runnable r = () -> System.out.println(i);
    }
}

Now the `i++` that is the inner loop step does not invalidate the capture of `i` as it should, because `WeAreCurrentlyInTheForLoopBody` will be false.  So in summary `WeAreCurrentlyInTheForLoopBody` can't just be a boolean flag because: Which loop are we talking about?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21415#discussion_r1794283192