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

[Archie Cobbs]

On Wed, 9 Oct 2024 21:48:47 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>>> 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?

> Concretely, why not, in `letInit`:

Because `visitForLoop()` itself has a loop and recurses into the parts of the `for()` statement multiple times. So when you loop back to visit `step` after visiting `body` for the first time, the flag would get reset.

So then you say, "Let's set a flag that let's this code know that we're in the _body_ of the loop" like this:

// In basic for() loop bodies, variables that are reassigned can no longer be captured
if (inForLoopBody && (sym.flags_field & FOR_LOOP_BODY_MAY_CAPTURE) != 0 && !uninits.isMember(sym.adr)) {
    sym.flags_field &= ~FOR_LOOP_BODY_MAY_CAPTURE;
}

and then you realize a boolean flag is not sufficient, and then ... well, see previous discussion :)

Another probem is that after the first loop, the variable is no longer DU...

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

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