RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code

[David Holmes]

On 31/05/2021 4:44 am, Alan Bateman wrote:
> On Fri, 28 May 2021 12:56:39 GMT, Jaroslav Tulach <github.com+26887752+JaroslavTulach at openjdk.org> wrote:
> 
>> This PR exposes runtime invisible annotations via `Class.getAnnotation` when `-XX:+PreserveAllAnnotations` option is passed to the JVM.
>>
>> Existing `-XX:+PreserveAllAnnotations` option can be very useful for code that needs to access annotations with `RetentionPolicy.CLASS` without the need to parse the .class files manually. While the RuntimeInvisibleAnnotations are kept in the runtime, they are not visible via java.lang.reflect API. I assume that's just an omission.
>>
>> This PR provides a new test and a fix to make `Class.getAnnotation(...)` useful when `-XX:+PreserveAllAnnotations` option is on.
> 
> I checked the pre-OpenJDK history and `-XX:+PreserveAllAnnotations` dates from when JSR-175 support was added in JDK 5. It may have been useful during the development but I don't see any tests using it now. I don't think we want to create an attractive nuisance here and maybe this XX option should be deprecated so that it can be obsoleted and eventually removed.

I was a bit confused by this PR until I realized that annotations with 
CLASS retention policy must be represented in the classfile as 
RuntimeInvisible annotations - as the only affect of 
PreserveAllAnnotations is to make invisible annotations remain present 
in the runtime representation of the classfile, as allowed for by JVMS 
4.7.17 (and related).

As Alan notes we don't seem to have any tests (current or historical) 
for the operation of this flag (which is quite surprising) but its 
affect in the VM code is quite simple and obvious - and was recently 
extended to handle record attributes.

I don't know what the design intent on the core reflection side was in 
relation to this. I could imagine a design that always asks the VM for 
any CLASS or RUNTIME retention annotation, knowing that the VM may have 
been instructed to preserve CLASS annotations. In that way the core 
reflection code would be oblivious to the means by which the VM was 
instructed to preserve said annotations.

If core reflection has never exposed CLASS retention annotations even 
when PreserveAllAnnotations is set, then I would not support starting 
now. But if it is the case that only some recently added annotations are 
not conforming to pre-existing behaviour then I would support fixing 
those cases. However, given we have not previously needed to tell core 
reflection about the PreserveAllAnnotations flag, I would not want to 
start doing so now, as it should not be necessary.

If PreserveAllAnnotations has hitherto been working exactly as one would 
expect given JVMS 4.7.17 etc, then I would certainly not support 
deprecating and removing it. But we should add in some tests.

Cheers,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/4245
>