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

[Joseph D. Darcy]

Hello,

A few initial comments, not my final review.

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

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

or creating a subdirectory for conditions, etc..

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.

(As a follow-up refactoring, it might be worthwhile to replace the 
interior of the three parseFooArray methods to a shared method that is 
passed a lambda for the "Object value = parseFooValue(/*args to get 
foo*/...);" logic.)

Thanks,

-Joe

On 2/23/2018 10:06 AM, joe darcy wrote:
> On 2/23/2018 9:39 AM, Alan Bateman wrote:
>> On 22/02/2018 23:20, Liam Miller-Cushon wrote:
>>> Hello,
>>>
>>>
>>> Please consider this fix for JDK-7183985.
>>>
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-7183985
>>>
>>> webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/
>>>
>>>
>>> I started a CSR for the change:
>>> https://bugs.openjdk.java.net/browse/JDK-8198584
>>>
>>> We have been using the fix at Google for about two years, and there has
>>> been no compatibility impact. I found very few places 
>>> ArrayStoreException
>>> was being explicitly handled, and none that were depending on the 
>>> current
>>> behaviour of getAnnotation().
>>>
>>>
>>> There was some previous discussion of the bug on core-libs-dev:
>>>
>> Yes, this one comes up every few years. I'm hoping Joe Darcy will 
>> reply to your review with any background or issues from when this 
>> came up in the past. From a distance then retrofitting 
>> AnnotatedElement getXXX methods to throw TypeNotPresentException 
>> seems reasonable, I'm less sure about the isAnnotationPresent method 
>> as it might be surprising for that to fail.
>>
>
> On my list!
>
> Cheers,
>
> -Joe
>