[code-reflection] RFR: Simplify code generation for quotable lambdas not to require duplicate capture argument lists

[Paul Sandoz]

On Fri, 4 Oct 2024 16:56:48 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> When translating quotable lambdas/method references, we generate two captured argument lists:
> 
> * the captured arguments to be used by the functional interface implementation;
> * the captured arguments to be used by the quotable implementation
> 
> In most cases these two captured lists are identical, except for cosmetic differences.
> This is even more true after the backend of javac underwent significant rearchitecture, to better support the Flexible Constructor Bodies feature.
> 
> This PR simplifies `ReflectMethods` so that a visitor now computes all the required captured variables upfront.
> The captured variables are then inserted in the quoted block, so that the rest of `BodyScanner` will be able to resolve references to such variables w/o the need for ad-hoc logic.
> 
> One complication is that, under this new regime, we can no longer afford to treat captured variables whose value is constant (e.g. `final int k = 100`) as if they were true captures.
> This is because neither `Lower` nor `LambdaToMethod` capture constant variables. Since we're reusing the same dynamic arguments as `LambdaToMethod` we need to make sure that `ReflectMethods` also does not capture constant arguments.
> Special logic to avoid capture of constant arguments is baked into the new visitor.
> 
> I have also added a check in the visitor, so that if, inside a quotable lambda, we see a creation of a local class that is declared outside the lambda, an unsupported exception is thrown.
> 
> I will followup with a separate fix to deal with these cases.

Nicely done, just a few comments.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java line 154:

> 152:         log = Log.instance(context);
> 153:         lower = Lower.instance(context);
> 154:         lambdaToMethod = LambdaToMethod.instance(context);

Unused?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java line 312:

> 310:     }
> 311: 
> 312:     // @@@: Only used for quoted lambda, not quotable ones. Remove?

Might be worth inlining the method (see also comment related to `// @@@ just make translation happy`)

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java line 536:

> 534: 
> 535:             List<VarSymbol> capturedSymbols = lambdaCaptureScanner.analyzeCaptures();
> 536:             int blockArgOffset = 0;

Suggestion:

            int blockParamOffset = 0;

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java line 556:

> 554:             // add "this" capture (if needed)
> 555:             if (lambdaCaptureScanner.capturesThis) {
> 556:                 top.localToOp.put(currentClassSym, null); // @@@ just make translation happy

I think we can avoid this. For a quotable lambda we know the number of block parameters (captures) before we obtain the captured expressions from `top.localToOp`.

If `funcOp.parameters().size() == bodyScanner.stack.localToOp.size() + 1` then we know we need to first capture `this` expression. This is easier to do if we inline the call to `quotedCapturedArgs`.

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

PR Review: https://git.openjdk.org/babylon/pull/249#pullrequestreview-2349103268
PR Review Comment: https://git.openjdk.org/babylon/pull/249#discussion_r1788301563
PR Review Comment: https://git.openjdk.org/babylon/pull/249#discussion_r1788288722
PR Review Comment: https://git.openjdk.org/babylon/pull/249#discussion_r1788311481
PR Review Comment: https://git.openjdk.org/babylon/pull/249#discussion_r1788299589