RFR: JDK-8267936: PreserveAllAnnotations option isn't tested

[David Holmes]

On 3/06/2021 1:38 am, Peter Levart wrote:
> On Wed, 2 Jun 2021 13:24:10 GMT, David Holmes <david.holmes at oracle.com> wrote:
> 
>> Sorry now I see what happens. We aren't combining two arrays of
>> annotations we're concatenating two streams of byes, each of which
>> represents a set of annotations, starting with the length.
>>
>> The code that receives this on the JDK side doesn't seem to understand
>> that this is a possibility.
>>
>> Though maybe this isn't a bug, maybe the AnnotationParser is
>> deliberately ignoring the second byte stream. (Though if it were
>> deliberate there should be some commentary to that affect!)
>>
>> David
> 
> I think this is not deliberate. Since `rawAnnotations` may end up having any of the following:
> - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation usages compiled into the class or `-XX+PreserveAllAnnotations` was not used at runtime)
> - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime)
> - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there were `RUNTIME` and `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime)
> 
> So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not in 2nd case?

Well in the second case there is no way to know it is looking only at 
invisible annotations, so it has to read them and then discard them as 
they were in fact CLASS retention. The skipping could be seen as an 
optimization.

The code is confusing because it gives no indication that it is aware 
that runtime invisible annotations could be present:

/**
      * Parses the annotations described by the specified byte array.
      * resolving constant references in the specified constant pool.
      * The array must contain an array of annotations as described
      * in the RuntimeVisibleAnnotations_attribute:

but at the same time it checks for the RUNTIME retention, which means it 
must have recognised the possibility there could be something else 
there. That check is day 2 code though, on day 1 there was this comment:

/* XXX Uncomment when this meta-attribute on meta-attributes (Neal's 
putback)
              if (type.retention() == VisibilityLevel.RUNTIME)
  XXX  */

But the end result is that the code in AnnotationParser correctly 
filters out all non-RUNTIME annotations, either directly, or by skipping 
them in the stream in the first place. So in that sense there is no bug, 
but the code could do with some additional comments.

Cheers,
David
-----


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