JDK 8 RFR, redux, for JDK-8005294 : Consider default methods for additions to AnnotatedElement

[Joe Darcy]

On 10/29/2013 06:37 PM, Joe Darcy wrote:
> Hi Joel,
>
> On 10/29/2013 09:20 AM, Joel Borggrén-Franck wrote:
>> Hi Joe, Peter,
>>
>> On 29 okt 2013, at 07:09, Joe Darcy <joe.darcy at oracle.com> wrote:
>>> Your comments, along with some spec refinements from the other 
>>> OpenJDK list, are reflected in the next iteration of the webrev:
>>>
>>>      http://cr.openjdk.java.net/~darcy/8005294.5/
>>>
>>> Thanks,
>>>
>> 259          if (result.length == 0 && // Neither directly nor 
>> indirectly present
>>   260              this instanceof Class && // the element is a class
>>   261 AnnotationType.getInstance(annotationClass).isInherited()) { // 
>> Inheritable
>>   262              ...
>>
>> j.l.Class is final and has an implementation of getAnnotationsByType 
>> so everything in this branch is dead code. I understand you might 
>> want to document how the lookup is supposed to happen if you have 
>> inheritance semantics for your AnnotatedElement but IMHO this isn't 
>> the way to do it. Just delegate to getDeclaredAnnotationsByType.
>
> Summarizing an off-list discussion, in terms of making this particular 
> method robust in the face of of possible alternative versions of 
> java.lang.Class and friends, I think it is acceptable to introduce 
> what will in practice be dead code with the version of java.lang.Class 
> in JDK 8.
>
>>
>> 291      * @since 1.8
>>   292      */
>>
>>   293     default <T extends Annotation> T 
>> getDeclaredAnnotation(Class<T> annotationClass) {
>>
>> I keep forgetting that this one is new to 8. My reservations about 
>> making this a default was based on (the misconception) that this was 
>> a 5 method. This method looks good.
>>
>> For getDeclaredAnnotationsByType() the idea of having two 
>> implementations of the same logic, one taking arrays as arguments and 
>> the other Maps, seems wrong. I would strongly prefer if we only had 
>> one implementation, calling into AnnotationSupport from this method. 
>> Something like
>>
>>   default <T extends Annotation> T[] 
>> getDeclaredAnnotationsByType(Class<T> annotationClass) {
>>           Objects.requireNonNull(annotationClass);
>>      return 
>> AnnotationSupport.getDirectlyAndIndirectlyPresent(Arrays.stream(getDeclaredAnnotations()).collect(Collectors.toMap(
>>                          (a -> a.getClass()),
>>                          Function.identity())),
>>                   annotationClass);
>>    }
>>
>> I can't see these default methods as the code we should be spending 
>> time optimizing.
>
> Given the improved maintenance situation and the low expecte duty 
> cycle of the default method implementations, despite having worked on 
> what should be a fast-ish getDeclaredAnnotation implementation, I'm 
> willing to replace it by a call to AnnotationSupport wrapped around a 
> call to getDeclaredAnnotations().
>
> This style of implementation is allowed by the current @implSpec for 
> the method.
>
> However, I'm not sure the 
> AnnotationSupport.getDirectlyAndIndirectlyPresent call can work as 
> currently formulated will work since I believe what needs to be called 
> is "a.getAnnotationType()" rather than "a.getClass()", but 
> unfortunately "getAnnotationType()" is not defined on AnnotatedElement 
> so reflection would be needed to call getAnnotationType :-(
>

I retract my previous comments; I was confusing "annotationType" and 
"value()" in terms of where they are defined. This version of 
getDeclaredAnnotationsByType passes the battery of regression tests:

     default <T extends Annotation> T[] 
getDeclaredAnnotationsByType(Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
         return AnnotationSupport.
getDirectlyAndIndirectlyPresent(Arrays.stream(getDeclaredAnnotations()).
collect(Collectors.toMap(
(a -> a.annotationType()),
Function.identity(),
((first,second) -> first),
() -> new LinkedHashMap<>() )),
                                             annotationClass);
     }

I've prepared a new version of the webrev with this change as well as a 
slightly-refactored regression test:

     http://cr.openjdk.java.net/~darcy/8005294.6

Thanks,

-Joe