RFR: 8336492: Regression in lambda serialization [v3]

[Liam Miller-Cushon]

On Sat, 27 Jul 2024 18:42:13 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

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

> @cushon things look stable on our end, maybe you want to give this PR another try?

I will verify with the latest changes, but I have preliminary results from testing before https://github.com/openjdk/jdk/pull/20349/commits/6a5501fa83aef2ee455e7c965a1ebc74d36b97ec. There were no unexpected regressions, and some minor expected compatibility impact:

* A number of tests are asserting on names of synthetic lambda methods that have changed (e.g. `assertThat(Throwables.getStackTraceAsString(...)).contains("foo$0");`)
* I found a single test that was reading in persisted serialized data which no longer deserialized, and had to be updated to match the new output

I also removed the workarounds I'd added for JDK-8336492 and confirmed that they are unnecessary after these changes.

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

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