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

[Maurizio Cimadamore]

On Wed, 9 Oct 2024 22:04:35 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

> Because `visitForLoop()` itself has a loop and recurses into the parts of the `for()` statement multiple times.

I believe this was the source of the misunderstanding. The `scan(tree.body)` in `AssignAnalyzer` is indeed buried in a loop as we need a couple of passes to make sure to detect bad cases of multiple assignments e.g. to the same blank final variable.

Note that even in the current PR you `letInit` will be called twice, which in principle can be problematic in cases like:


for (int i ; ,..) {
    i = m();
    ...
}


If I understand correctly, the way the code in this PR works for the above is:
* in the first DA/DU pass of the loop body (`flowKind` == `NORMAL`), `i` is DU when it's assigned. So it is *not* added to the list of loop variable reassigned in body
* at the end of the pass, we see that `i` is not reassigned, so we augment it with the `FOR_LOOP_BODY_MAY_CAPTURE` flag
* then we do a second DA/DU pass of the loop body (`flowKind` == `SPECULATIVE`), which calls `letInit` again. This time `i` is not DU when assigned, so `i` ends up in the list of reassigned loop variables.
* at the end of the pass we do nothing (since we see `i` as reassigned). Luckily, the flag set in the first DA/DU pass is still there, and will be used by `CaptuerAnalyzer`.

This begs the question: should `letInit` even bother to deal with reassigned loop variables in the speculative DA/DU pass? Seems to me that pass will generally cause spurious results because of the changed DA/DU status of the variables involved.

Regardless, I think I found an issue with the current implementation when there are nested loops:


    void test() {
        for (int i = 0; ; ) {
            for (; ;) {
                i = 42;
                Runnable r = () -> System.out.println(i);
            }
        }
    }


This compiles fine, even though `i` is reassigned when DA. The problem seems to be that we lose track of the reassigned bit set (e.g. the inner loop will overwrite that), and since the variable is reassigned from the inner loop, that is problematic. Removing the inner loop results in a compile-time error, as expected.

We should either track "active" loop variables in a list when we append/remove from. Or, we should use the various Bits::incl/excl methods. E.g. flow should not create a new bit set on each loop, but augment the existing bit set (the initial could be the "all zero" set) with the bits corresponding to the variables of the loop under consideration. Then, once we have finished inspecting a loop, we "exclude" its variable bits from the sets before returning from the visitor. This way the "outer" loop will be able to "see" reassignments happening in inner loops.

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

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