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

[Peter Levart]

On Wed, 2 Jun 2021 22:44:10 GMT, David Holmes <david.holmes at oracle.com> wrote:

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

Not all annotations in the 2nd case need to be CLASS retention. They were CLASS retention when the class that uses them was compiled, but at runtime, the retention may be different (separate compilation). So they are actually returned by the reflection in that case.

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

Yes, there could be a CLASS retention annotation there (even though `-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations contains the content of `RuntimeVisibleAnnotations` only). Again, such annotation was RUNTIME retention when its use was compiled into a class, but at runtime such annotation may be updated to have CLASS or even SOURCE retention. Such annotation use is filtered out.

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

I guess this was just code in creation before release. At some point, the `RetentionPolicy` enum was added, but above comment refers to it by the name `VisibilityLevel`....

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

The problem is that it sometimes skips RUNTIME annotations too. I consider this a bug.

> 
> Cheers,
> David

Regard, Peter

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

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