8004912: Repeating annotations - getAnnotationsByType(Class<T>) is not working...

8004912: Repeating annotations - getAnnotationsByType(Class<T>) is not working as expected for few inheritance scenarios
8019420: Repeatable non-inheritable annotation types are mishandled by Core Reflection
Reviewed-by: jfranck

src/share/classes/java/lang/Class.java

@@ -3314,7 +3314,10 @@ public final class Class<T> implements java.io.Serializable,
     public <A extends Annotation> A[] getAnnotationsByType(Class<A> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
-        return AnnotationSupport.getMultipleAnnotations(annotationData().annotations, annotationClass);
+        AnnotationData annotationData = annotationData();
+        return AnnotationSupport.getAssociatedAnnotations(annotationData.declaredAnnotations,
+                                                          annotationData.annotations,
+                                                          annotationClass);
     }
 
     /**
@@ -3344,7 +3347,8 @@ public final class Class<T> implements java.io.Serializable,
     public <A extends Annotation> A[] getDeclaredAnnotationsByType(Class<A> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
-        return AnnotationSupport.getMultipleAnnotations(annotationData().declaredAnnotations, annotationClass);
+        return AnnotationSupport.getDirectlyAndIndirectlyPresent(annotationData().declaredAnnotations,
+                                                                 annotationClass);
     }
 
     /**

src/share/classes/java/lang/reflect/Executable.java

@@ -527,7 +527,7 @@ public abstract class Executable extends AccessibleObject
     public <T extends Annotation> T[] getAnnotationsByType(Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
-        return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass);
+        return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass);
     }
 
     /**

src/share/classes/java/lang/reflect/Field.java

@@ -1123,7 +1123,7 @@ class Field extends AccessibleObject implements Member {
     public <T extends Annotation> T[] getAnnotationsByType(Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
-        return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass);
+        return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass);
     }
 
     /**

src/share/classes/java/lang/reflect/Parameter.java

@@ -295,7 +295,7 @@ public final class Parameter implements AnnotatedElement {
     public <T extends Annotation> T[] getAnnotationsByType(Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
 
-        return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass);
+        return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass);
     }
 
     /**

src/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java

@@ -166,7 +166,7 @@ public class AnnotatedTypeFactory {
 
         @Override
         public <T extends Annotation> T[] getDeclaredAnnotationsByType(Class<T> annotation) {
-            return AnnotationSupport.getMultipleAnnotations(annotations, annotation);
+            return AnnotationSupport.getDirectlyAndIndirectlyPresent(annotations, annotation);
         }
 
         // AnnotatedType

src/share/classes/sun/reflect/annotation/AnnotationSupport.java

@@ -28,109 +28,224 @@ package sun.reflect.annotation;
 import java.lang.annotation.*;
 import java.lang.reflect.*;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
 public final class AnnotationSupport {
+
     /**
-     * Finds and returns all annotation of the type indicated by
-     * {@code annotationClass} from the {@code Map} {@code
-     * annotationMap}. Looks into containers of the {@code
-     * annotationClass} (as specified by an the {@code
-     * annotationClass} type being meta-annotated with an {@code
-     * Repeatable} annotation).
+     * Finds and returns all annotations in {@code annotations} matching
+     * the given {@code annoClass}.
+     *
+     * Apart from annotations directly present in {@code annotations} this
+     * method searches for annotations inside containers i.e. indirectly
+     * present annotations.
      *
-     * @param annotationMap the {@code Map} used to store annotations indexed by their type
-     * @param annotationClass the type of annotation to search for
+     * The order of the elements in the array returned depends on the iteration
+     * order of the provided map. Specifically, the directly present annotations
+     * come before the indirectly present annotations if and only if the
+     * directly present annotations come before the indirectly present
+     * annotations in the map.
      *
-     * @return an array of instances of {@code annotationClass} or an empty array if none were found
+     * @param annotations the {@code Map} in which to search for annotations
+     * @param annoClass the type of annotation to search for
+     * @param includeNonInheritedContainees if false, the annoClass must be
+     *        inheritable for the containers to be searched
+     *
+     * @return an array of instances of {@code annoClass} or an empty
+     *         array if none were found
      */
-    public static  <A extends Annotation> A[] getMultipleAnnotations(
-            final Map<Class<? extends Annotation>, Annotation> annotationMap,
-            final Class<A> annotationClass) {
-        final List<A> res = new ArrayList<A>();
+    private static <A extends Annotation> A[] getDirectlyAndIndirectlyPresent(
+            Map<Class<? extends Annotation>, Annotation> annotations,
+            Class<A> annoClass,
+            boolean includeNonInheritedContainees) {
+
+        List<A> result = new ArrayList<A>();
 
         @SuppressWarnings("unchecked")
-        final A candidate = (A)annotationMap.get(annotationClass);
-        if (candidate != null) {
-            res.add(candidate);
+        A direct = (A) annotations.get(annoClass);
+        if (direct != null)
+            result.add(direct);
+
+        if (includeNonInheritedContainees ||
+                AnnotationType.getInstance(annoClass).isInherited()) {
+            A[] indirect = getIndirectlyPresent(annotations, annoClass);
+
+            if (indirect != null) {
+
+                boolean indirectFirst = direct == null ||
+                                        containerBeforeContainee(annotations, annoClass);
+
+                result.addAll((indirectFirst ? 0 : 1), Arrays.asList(indirect));
+            }
         }
 
-        final Class<? extends Annotation> containerClass = getContainer(annotationClass);
-        if (containerClass != null) {
-            res.addAll(unpackAll(annotationMap.get(containerClass), annotationClass));
+        @SuppressWarnings("unchecked")
+        A[] arr = (A[]) Array.newInstance(annoClass, result.size());
+        return result.toArray(arr);
+    }
+
+
+    /**
+     * Equivalent to calling {@code getDirectlyAndIndirectlyPresentAnnotations(
+     * annotations, annoClass, true)}.
+     */
+    public static <A extends Annotation> A[] getDirectlyAndIndirectlyPresent(
+            Map<Class<? extends Annotation>, Annotation> annotations,
+            Class<A> annoClass) {
+
+        return getDirectlyAndIndirectlyPresent(annotations, annoClass, true);
+    }
+
+
+    /**
+     * Finds and returns all annotations matching the given {@code annoClass}
+     * indirectly present in {@code annotations}.
+     *
+     * @param annotations annotations to search indexed by their types
+     * @param annoClass the type of annotation to search for
+     *
+     * @return an array of instances of {@code annoClass} or an empty array if no
+     *         indirectly present annotations were found
+     */
+    private static <A extends Annotation> A[] getIndirectlyPresent(
+            Map<Class<? extends Annotation>, Annotation> annotations,
+            Class<A> annoClass) {
+
+        Repeatable repeatable = annoClass.getDeclaredAnnotation(Repeatable.class);
+        if (repeatable == null)
+            return null;  // Not repeatable -> no indirectly present annotations
+
+        Class<? extends Annotation> containerClass = repeatable.value();
+
+        Annotation container = annotations.get(containerClass);
+        if (container == null)
+            return null;
+
+        // Unpack container
+        A[] valueArray = getValueArray(container);
+        checkTypes(valueArray, container, annoClass);
+
+        return valueArray;
+    }
+
+
+    /**
+     * Figures out if conatiner class comes before containee class among the
+     * keys of the given map.
+     *
+     * @return true if container class is found before containee class when
+     *         iterating over annotations.keySet().
+     */
+    private static <A extends Annotation> boolean containerBeforeContainee(
+            Map<Class<? extends Annotation>, Annotation> annotations,
+            Class<A> annoClass) {
+
+        Class<? extends Annotation> containerClass =
+                annoClass.getDeclaredAnnotation(Repeatable.class).value();
+
+        for (Class<? extends Annotation> c : annotations.keySet()) {
+            if (c == containerClass) return true;
+            if (c == annoClass) return false;
         }
 
-        @SuppressWarnings("unchecked") // should be safe annotationClass is a token for A
-        final A[] emptyTemplateArray = (A[])Array.newInstance(annotationClass, 0);
-        return res.isEmpty() ? emptyTemplateArray : res.toArray(emptyTemplateArray);
+        // Neither containee nor container present
+        return false;
     }
 
-    /** Helper to get the container, or null if none, of an annotation. */
-    private static <A extends Annotation> Class<? extends Annotation> getContainer(Class<A> annotationClass) {
-        Repeatable containingAnnotation = annotationClass.getDeclaredAnnotation(Repeatable.class);
-        return (containingAnnotation == null) ? null : containingAnnotation.value();
+
+    /**
+     * Finds and returns all associated annotations matching the given class.
+     *
+     * The order of the elements in the array returned depends on the iteration
+     * order of the provided maps. Specifically, the directly present annotations
+     * come before the indirectly present annotations if and only if the
+     * directly present annotations come before the indirectly present
+     * annotations in the relevant map.
+     *
+     * @param declaredAnnotations the declared annotations indexed by their types
+     * @param allAnnotations declared and inherited annotations indexed by their types
+     * @param annoClass the type of annotation to search for
+     *
+     * @return an array of instances of {@code annoClass} or an empty array if none were found.
+     */
+    public static <A extends Annotation> A[] getAssociatedAnnotations(
+            Map<Class<? extends Annotation>, Annotation> declaredAnnotations,
+            Map<Class<? extends Annotation>, Annotation> allAnnotations,
+            Class<A> annoClass) {
+
+        // Search declared
+        A[] result = getDirectlyAndIndirectlyPresent(declaredAnnotations, annoClass);
+
+        // Search inherited
+        if (result.length == 0)
+            result = getDirectlyAndIndirectlyPresent(allAnnotations, annoClass, false);
+
+        return result;
     }
 
-    /** Reflectively look up and get the returned array from the the
-     * invocation of the value() element on an instance of an
-     * Annotation.
+
+    /* Reflectively invoke the values-method of the given annotation
+     * (container), cast it to an array of annotations and return the result.
      */
-    private static  <A extends Annotation> A[] getValueArray(Annotation containerInstance) {
+    private static <A extends Annotation> A[] getValueArray(Annotation container) {
         try {
-            // the spec tells us the container must have an array-valued
-            // value element. Get the AnnotationType, get the "value" element
-            // and invoke it to get the contents.
+            // According to JLS the container must have an array-valued value
+            // method. Get the AnnotationType, get the "value" method and invoke
+            // it to get the content.
 
-            Class<? extends Annotation> containerClass = containerInstance.annotationType();
+            Class<? extends Annotation> containerClass = container.annotationType();
             AnnotationType annoType = AnnotationType.getInstance(containerClass);
             if (annoType == null)
-                throw new AnnotationFormatError(containerInstance + " is an invalid container for repeating annotations");
+                throw invalidContainerException(container, null);
 
             Method m = annoType.members().get("value");
             if (m == null)
-                throw new AnnotationFormatError(containerInstance +
-                                                          " is an invalid container for repeating annotations");
+                throw invalidContainerException(container, null);
+
             m.setAccessible(true);
 
-            @SuppressWarnings("unchecked") // not provably safe, but we catch the ClassCastException
-            A[] a = (A[])m.invoke(containerInstance); // this will erase to (Annotation[]) but we
-                                                      // do a runtime cast on the return-value
-                                                      // in the methods that call this method
-            return a;
-        } catch (IllegalAccessException | // couldnt loosen security
-                 IllegalArgumentException | // parameters doesn't match
+            // This will erase to (Annotation[]) but we do a runtime cast on the
+            // return-value in the method that call this method.
+            @SuppressWarnings("unchecked")
+            A[] values = (A[]) m.invoke(container);
+
+            return values;
+
+        } catch (IllegalAccessException    | // couldn't loosen security
+                 IllegalArgumentException  | // parameters doesn't match
                  InvocationTargetException | // the value method threw an exception
-                 ClassCastException e) { // well, a cast failed ...
-            throw new AnnotationFormatError(
-                    containerInstance + " is an invalid container for repeating annotations",
-                    e);
+                 ClassCastException e) {
+
+            throw invalidContainerException(container, e);
+
         }
     }
 
-    /* Sanity check type of and return a list of all the annotation
-     * instances of type {@code annotationClass} from {@code
-     * containerInstance}.
-     */
-    private static <A extends Annotation> List<A> unpackAll(Annotation containerInstance,
-                                                            Class<A> annotationClass) {
-        if (containerInstance == null) {
-            return Collections.emptyList(); // container not present
-        }
 
-        try {
-            A[] a = getValueArray(containerInstance);
-            List<A> l = new ArrayList<>(a.length);
-            for (int i  = 0; i < a.length; i++)
-                l.add(annotationClass.cast(a[i]));
-            return l;
-        } catch (ClassCastException |
-                 NullPointerException e) {
-            throw new AnnotationFormatError(
-                    String.format("%s is an invalid container for repeating annotations of type: %s",
-                        containerInstance, annotationClass),
-                    e);
+    private static AnnotationFormatError invalidContainerException(Annotation anno,
+                                                                   Throwable cause) {
+        return new AnnotationFormatError(
+                anno + " is an invalid container for repeating annotations",
+                cause);
+    }
+
+
+    /* Sanity check type of all the annotation instances of type {@code annoClass}
+     * from {@code container}.
+     */
+    private static <A extends Annotation> void checkTypes(A[] annotations,
+                                                          Annotation container,
+                                                          Class<A> annoClass) {
+        for (A a : annotations) {
+            if (!annoClass.isInstance(a)) {
+                throw new AnnotationFormatError(
+                        String.format("%s is an invalid container for " +
+                                      "repeating annotations of type: %s",
+                                      container, annoClass));
+            }
         }
     }
 }

src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java

@@ -198,7 +198,7 @@ public class TypeVariableImpl<D extends GenericDeclaration>
     @Override
     public <T extends Annotation> T[] getAnnotationsByType(Class<T> annotationClass) {
         Objects.requireNonNull(annotationClass);
-        return AnnotationSupport.getMultipleAnnotations(mapAnnotations(getAnnotations()), annotationClass);
+        return AnnotationSupport.getDirectlyAndIndirectlyPresent(mapAnnotations(getAnnotations()), annotationClass);
     }
 
     @Override

test/java/lang/annotation/repeatingAnnotations/NonInheritableContainee.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8019420
+ * @summary Repeatable non-inheritable annotation types are mishandled by Core Reflection
+ */
+
+import java.util.*;
+import java.lang.annotation.*;
+import java.lang.reflect.*;
+
+public class NonInheritableContainee {
+
+    @Retention(RetentionPolicy.RUNTIME)
+    @Repeatable(InheritedAnnotationContainer.class)
+    @interface NonInheritedAnnotationRepeated {
+        String name();
+    }
+
+    @Inherited
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface InheritedAnnotationContainer {
+        NonInheritedAnnotationRepeated[] value();
+    }
+
+    @NonInheritedAnnotationRepeated(name="A")
+    @NonInheritedAnnotationRepeated(name="B")
+    class Parent {}
+    class Sample extends Parent {}
+
+
+    public static void main(String[] args) {
+
+        Annotation[] anns = Sample.class.getAnnotationsByType(
+                NonInheritedAnnotationRepeated.class);
+
+        if (anns.length != 0)
+            throw new RuntimeException("Non-@Inherited containees should not " +
+                    "be inherited even though its container is @Inherited.");
+    }
+}

test/java/lang/annotation/repeatingAnnotations/OrderUnitTest.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug     8004912
+ * @summary Unit test for order of annotations returned by get[Declared]AnnotationsByType.
+ *
+ * @run main OrderUnitTest
+ */
+
+import java.lang.annotation.*;
+import java.lang.reflect.*;
+
+public class OrderUnitTest {
+
+    public static void main(String[] args) {
+        testOrder(Case1.class);
+        testOrder(Case2.class);
+    }
+
+    private static void testOrder(AnnotatedElement e) {
+        Annotation[] decl = e.getDeclaredAnnotations();
+        Foo[] declByType = e.getDeclaredAnnotationsByType(Foo.class);
+
+        if (decl[0] instanceof Foo != declByType[0].isDirect() ||
+            decl[1] instanceof Foo != declByType[1].isDirect()) {
+            throw new RuntimeException("Order of directly / indirectly present " +
+                    "annotations from getDeclaredAnnotationsByType does not " +
+                    "match order from getDeclaredAnnotations.");
+        }
+    }
+}
+
+@Retention(RetentionPolicy.RUNTIME)
+@interface FooContainer {
+    Foo[] value();
+}
+
+@Retention(RetentionPolicy.RUNTIME)
+@Repeatable(FooContainer.class)
+@interface Foo {
+    boolean isDirect();
+}
+
+
+@Foo(isDirect = true) @FooContainer({@Foo(isDirect = false)})
+class Case1 {
+}
+
+
+@FooContainer({@Foo(isDirect = false)}) @Foo(isDirect = true)
+class Case2 {
+}

test/java/lang/annotation/repeatingAnnotations/RepeatedUnitTest.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug     7154390 8005712 8007278
+ * @bug     7154390 8005712 8007278 8004912
  * @summary Unit test for repeated annotation reflection
  *
  * @compile RepeatedUnitTest.java subpackage/package-info.java subpackage/Container.java subpackage/Containee.java subpackage/NonRepeated.java subpackage/InheritedContainee.java subpackage/InheritedContainer.java subpackage/InheritedNonRepeated.java
@@ -51,6 +51,12 @@ public class RepeatedUnitTest {
         inheritedMe3();
         inheritedMe4();
 
+        inheritedMe5();    // ContainerOnSuperSingleOnSub
+        inheritedMe6();    // RepeatableOnSuperSingleOnSub
+        inheritedMe7();    // SingleAnnoOnSuperContainerOnSub
+        inheritedMe8();    // SingleOnSuperRepeatableOnSub
+
+
         // CONSTRUCTOR
         checkMultiplier(Me1.class.getConstructor(new Class[0]), 10);
 
@@ -159,6 +165,30 @@ public class RepeatedUnitTest {
         check(e.getAnnotationsByType(NonRepeated.class)[0].value() == 1000);
     }
 
+    static void inheritedMe5() {
+        AnnotatedElement e = Me5.class;
+        check(2 == e.getAnnotations().length);
+        check(1 == countAnnotation(e, InheritedContainee.class));
+    }
+
+    static void inheritedMe6() {
+        AnnotatedElement e = Me6.class;
+        check(2 == e.getAnnotations().length);
+        check(1 == countAnnotation(e, InheritedContainee.class));
+    }
+
+    static void inheritedMe7() {
+        AnnotatedElement e = Me7.class;
+        check(2 == e.getAnnotations().length);
+        check(2 == countAnnotation(e, InheritedContainee.class));
+    }
+
+    static void inheritedMe8() {
+        AnnotatedElement e = Me8.class;
+        check(2 == e.getAnnotations().length);
+        check(2 == countAnnotation(e, InheritedContainee.class));
+    }
+
     static void checkMultiplier(AnnotatedElement e, int m) {
         // Basic sanity of non-repeating getAnnotation(Class)
         check(e.getAnnotation(NonRepeated.class).value() == 5 * m);
@@ -252,3 +282,31 @@ class Me3 extends Father {}
 @InheritedContainee(1000) @InheritedContainee(2000) @InheritedContainee(3000) @InheritedContainee(4000)
 @Containee(1000) @Containee(2000) @Containee(3000) @Containee(4000)
 class Me4 extends Father {}
+
+
+@InheritedContainer({@InheritedContainee(1), @InheritedContainee(2)})
+class SuperOf5 {}
+
+@InheritedContainee(3)
+class Me5 extends SuperOf5{}
+
+
+@InheritedContainee(1) @InheritedContainee(2)
+class SuperOf6 {}
+
+@InheritedContainee(3)
+class Me6 extends SuperOf6 {}
+
+
+@InheritedContainee(1)
+class SuperOf7 {}
+
+@InheritedContainer({@InheritedContainee(2), @InheritedContainee(3)})
+class Me7 extends SuperOf7 {}
+
+
+@InheritedContainee(1)
+class SuperOf8 {}
+
+@InheritedContainee(2) @InheritedContainee(3)
+class Me8 extends SuperOf8 {}