RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Joseph D. Darcy]

Hi Liam,

On 2/24/2018 5:14 PM, Liam Miller-Cushon wrote:
> Hi, thanks for the comments.
>
> The updated webrev is at: 
> http://cr.openjdk.java.net/~cushon/7183985/webrev.02/ 
> <http://cr.openjdk.java.net/%7Ecushon/7183985/webrev.02/>
>
> On Fri, Feb 23, 2018 at 4:29 PM, Joseph D. Darcy <joe.darcy at oracle.com 
> <mailto:joe.darcy at oracle.com>> wrote:
>
>     Objects implementing the AnnotatedElement interface are also
>     created in javac during annotation processing. As a secondary
>     concern, it would be good to be have behavior between both javac
>     and runtime annotations consistent when possible. (My own to-do
>     list includes at least once such alignment JDK-8164819: "Make
>     javac's toString() on annotation objects consistent with core
>     reflection".)
>
>
> Do you have a pointer to where that happens? There's 
> javax.lang.model.AnnotatedConstruct#getAnnotation, which isn't 
> affected by this issue--it throws MirroredTypesExceptionProxy when 
> accessing an annotation value that is an array of class literals, 
> regardless of whether the elements' classes are missing. I'm not 
> seeing implementations of AnnotatedElement in langtools.

Sorry, I misremembered the situation in javac. For annotation processing 
in javac, AnnotatedElement is not a factor, but the class

src/jdk.compiler/share/classes/com/sun/tools/javac/model/AnnotationProxyMaker.java

in javac does create annotation proxies, which is the same general 
technique used to create the annotation objects at runtime for core 
reflection.  These annotation objects in javac for javax.lang.model and 
annotation processing, do not follow the full contract of 
java.lang.reflect.AnnotatedElement. In particular, the annotations 
returned are not necessarily serializable. The intersection of methods 
on AnnotatedConstruct and AnnotatedElement is only a proper subset of 
the methods on AnnotatedElement; however, getAnnotation​(Class<T> 
annotationClass) is in the intersection.

>     Even in javac we've moved away from test and directory names based
>     on bugid. I'd recommend incorporating these regression tests into
>     the existing tests in
>         test/jdk/java/lang/annotation/Missing
>
>
> Thanks for the reminder, done.


I believe the new tests could reuse some of the existing types in 
test/jdk/java/lang/annotation/Missing. For example, the new 
MissingAnnotation.java is an alpha-rename of the existing Missing.java. 
If such sharing is not practical, then I'd recommend putting the new 
files into a subdirectory under underneath test/.../annotation/Missing 
(otherwise it will be confusing to edit these tests in the future since 
too many file names will start with "Missing".)

In MissingClassArrayElementTest.java:

   71     public static void main(String[] args) throws Exception {
   72         ClassArrayAnnotation[] outer = 
Test.class.getAnnotation(AnnotationAnnotation.class).value();
   73         // The second entry in the values array can be loaded 
successfully
   74         assertEquals(Arrays.toString(outer[1].value()), "[class 
java.lang.String]");

Something like

     assertEquals(Arrays.equals(outer[1].value(), {String.class})

would be more robust against any future changes in Class.toString. 
Likewise for the analogous comparisons.

>     It would be worth verifying whether or not this change also covers
>     java.lang.reflect.Executable.getParameterAnnotations(), more
>     specifically the implementation of that method in Method and
>     Constructor. The method getParameterAnnotations() is much less
>     used than getAnnotations and the other methods on the
>     AnnotatedElement interface, but still part of the annotations feature.
>
>
> Done.

Thanks.

Cheers,

-Joe