RFR: 8336492: Regression in lambda serialization [v3]

[Liam Miller-Cushon]

On Fri, 26 Jul 2024 16:49:11 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR consolidates the code for dealing with local captures in both `Lower` and `LambdaToMethod`. It does so by adding a new tree scanner, namely `CaptureScanner`, which is then subclassed by `Lower.FreeVarCollector` and also by the new `LambdaToMethod.LambdaCaptureScanner`.
>> 
>> The main idea behind the new visitor is that we can compute the set of (most) captured locals w/o relying on complex state from `Lower`, and make the logic general enough to work on *any* tree. This can be done by keeping track of all local variable declarations occurring in a given subtree `T`. Then, if `T` contains a reference to a local variable that has not been seen, we can assume that this variable is a captured variable. This simple logic works rather well, and doesn't rely on checking e.g. that the accessed variable has the same owner of the method that owns a local class (which then caused other artifacts such as `Lower::ownerToCopyFreeVarsFrom`, now removed).
>> 
>> The bigger rewrite is the one in `LambdaToMethod`. That class featured a custom visitor called `LambdaAnalyzerPreprocessor` (now removed) which maintains a stack of frames encountered during a visit, and tries to establish which references to local variables needs to be captured by the lambda, as well as whether `this` needs to be referenced by the lambda. Thanks to the new `CaptureScanner` visitor, all this logic can be achieved in a very small subclass of `CaptureScanner`, namely `LambdaCaptureScanner`.
>> 
>> This removes the need to keep track of frames, and other ancillary data structures. `LambdaTranslationContext` is now significantly simpler, and its most important field is a `Map<VarSymbol, VarSymbol>` which maps logical lambda symbols to translated ones (in the generated lambda method). We no longer need to maintain different maps for different kinds of captured symbols.
>> 
>> The code for patching identifiers in a lambda expression also became a lot simpler: we just check if we have pending lambda translation context and, if so, we ask if the identifier symbol is contained in the translation map. Otherwise, we leave the identifier as is.
>> 
>> Fixing the issue referenced by this PR is now very simple: inside `LambdaCaptureScanner`, we just need to make sure that any identifier referring to a captured local field generated by `Lower` is re-captured, and the rest just works. To make the code more robust I've added a new variable flag to denote synthetic captured fields generated by `Lower`.
>> 
>> #### C...
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add more docs to CaptureScanner, and make fields private/final

Some initial testing shows a regression in type annotation handling with this change.

Given an example like the following, the type annotation in the lambda is now being incorrectly propagated to the constructor. A `RuntimeInvisibleTypeAnnotations` attribute for the annotation is emitted on both the synthetic lambda method and the constructor. I noticed this because the `length` for the `RuntimeInvisibleTypeAnnotations` is longer than the constructor, and both ASM and javap report an error for the class file.


import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import java.util.ArrayList;
import java.util.List;

class T {
  @Target(ElementType.TYPE_USE)
  @interface A {}

  Runnable r =
      () -> {
        List<@A Object> xs = new ArrayList<>();
        xs.add(1);
        xs.add(2);
        xs.add(3);
        xs.add(5);
        xs.add(6);
        xs.add(7);
        xs.add(8);
      };

  T() {}
}


javap reports an error because the type annotation's length is longer than the constructor:


$ javap -c T
Compiled from "T.java"
class T {
  java.lang.Runnable r;

  T();
    Code:
Error: error at or after byte 0
Error: Fatal error: Bytecode offset out of range; bci=89, codeLength=14


Using the JDK 11 javap shows


  T();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=2, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: aload_0
         5: invokedynamic #7,  0              // InvokeDynamic #0:run:()Ljava/lang/Runnable;
        10: putfield      #11                 // Field r:Ljava/lang/Runnable;
        13: return
      LineNumberTable:
        line 22: 0
        line 10: 4
        line 22: 13
      RuntimeInvisibleTypeAnnotations:
        0: #35(): LOCAL_VARIABLE, {start_pc=8, length=81, index=0}, location=[TYPE_ARGUMENT(0)]
          T$A

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

PR Comment: https://git.openjdk.org/jdk/pull/20349#issuecomment-2254224629