Hi Peter, On my way to dinner I realized we have a problem for inherited annotations there are inherited over two (or more) declarations. I believe this is essentially the issue you highlighted though I will look into this tomorrow.
On 22 okt 2013, at 21:20, Peter Levart <peter.lev...@gmail.com> wrote: > I think the problem could be solved in two ways: > > - by explicitly scanning the inheritance chain for the 1st class that has the > annotations of type T present directly or indirectly (by containment). > Yes, this is how I solved it for javax.lang.model and this is how I think we should fix this. > - by "canonicalizing" the representation of the repeating annotations in the > class file attributes. By this I mean that each repeating annotation is > placed inside it's container at compile-time even if it is a single > annotation of a particular type. This would mean that some features like > specifying different @Targets or @Retentions for repeating annotation types > and their containers would be prohibited and the specification would have to > be changed. But I think this way the "inheritance" aspect of repeating > annotations would be consistent even if not "looking through" containers (by > using the old JDK7 API)... > > The "canonicalization" could be performed at runtime though, and I think > there were such attempts already in the past. What happened to them? > There hasn't been any attempt to do this at runtime. Fwiw I don't think this is worth the complexity unless someone proves the iterative lookup on super types is a performance issue. cheers /Joel > > Regards, Peter > > On 10/22/2013 02:57 PM, Peter Levart wrote: >> Hi, >> >> The spec says: >> >> When the new get[Declared]AnnotationsByType(Class<T>) methods are >> called for a repeatable annotation type T, the question is how to extend the >> policy >> to handle multiple annotations of type T on the superclass and/or subclass. >> Oracle >> proposes the following policy for Java SE 8: >> >> • If a class declaration does not have either a "directly present" >> annotation of >> type T or an "indirectly present by containment" annotation of type T, the >> class >> declaration may be "associated" with an annotation of type T due to >> inheritance. >> >> • If a class declaration does have either a "directly present" annotation of >> type T >> or an "indirectly present by containment" annotation of type T, the >> annotation >> is deemed to "override" every annotation of type T which is "associated" >> with >> the superclass. >> >> This policy for Java SE 8 is reified in the following definitions: >> >> • An annotation A is directly present on an element E if E has a >> RuntimeVisibleAnnotations or RuntimeVisibleParameterAnnotations or >> RuntimeVisibleTypeAnnotations attribute, and the attribute contains A.1.2 >> Core Reflection API REPEATING ANNOTATIONS >> 12 >> >> • An annotation A is indirectly present on an element E if E has a >> RuntimeVisibleAnnotations or RuntimeVisibleParameterAnnotations or >> RuntimeVisibleTypeAnnotations attribute, and A's type is repeatable, and the >> attribute contains exactly one annotation whose value element contains A and >> whose type is the containing annotation type of A's type (§9.6). >> >> • An annotation A is present on an element E if either: >> - A is directly present on E; or >> - No annotation of A's type is directly present on E, and E is a class, and >> A's type >> is inheritable (§9.6.3.3), and A is present on the superclass of E. >> >> • An annotation A is associated with an element E if either: >> - A is directly or indirectly present on E; or >> - No annotation of A's type is directly or indirectly present on E, and E >> is a class, >> and A's type is inheritable (§9.6.3.3), and A is associated with the >> superclass >> of E. >> >> >> So I guess E.class.getAnnotations() returns the annotations "present" on the >> E >> and E.class.getAnnotationsByType(A.class) returns the annotations of type A >> "associated" with E >> >> Am I right? >> >> Let's just focus on "associated" part now - the getAnnotationsByType method. >> The following test: >> >> http://cr.openjdk.java.net/~plevart/jdk8-tl/RepearingAnnotations/InheritedRepeatingAnnotationsTest.java >> >> dumps @Repeatable @Inherited annotations "associated" with the following >> classes: >> >> @Ann(10) >> class A1 {} >> >> @Ann(20) >> class A2 extends A1 {} >> >> class A3 extends A2 {} >> >> >> @Ann(10) @Ann(11) >> class B1 {} >> >> @Ann(20) >> class B2 extends B1 {} >> >> class B3 extends B2 {} >> >> >> @Ann(10) >> class C1 {} >> >> @Ann(20) @Ann(21) >> class C2 extends C1 {} >> >> class C3 extends C2 {} >> >> >> @Ann(10) @Ann(11) >> class D1 {} >> >> @Ann(20) @Ann(21) >> class D2 extends D1 {} >> >> class D3 extends D2 {} >> >> >> Here's the output: >> >> >> class A2: [@Ann(value=20)] >> class B2: [@Ann(value=20)] >> class C2: [@Ann(value=20), @Ann(value=21)] >> class D2: [@Ann(value=20), @Ann(value=21)] >> >> class A3: [@Ann(value=20)] >> class B3: [@Ann(value=10), @Ann(value=11), @Ann(value=20)] >> class C3: [@Ann(value=10), @Ann(value=20), @Ann(value=21)] >> class D3: [@Ann(value=20), @Ann(value=21)] >> >> >> >> Class B3 is showing @Ann(10) and @Ann(11), while class C3 is showing >> @Ann(10) as being "associated" with them, but they should not, right? >> >> Class B3 should show the same annotations as B2 and class C3 should show the >> same annotations as class C2, I think. >> >> >> Regards, Peter >> >> >> On 10/22/2013 01:03 PM, Joel Borggrén-Franck wrote: >>> Hi Peter, >>> >>> Spec is here: >>> http://cr.openjdk.java.net/~abuckley/8misc.pdf >>> >>> >>> FYI I pushed this before I saw your mail but I do think the code is >>> correct. An extra pair of eyes would be great though! >>> >>> FWIW I suspect we can be better with regards to allocation especially if we >>> optimize for the common case where there is either a single annotation or a >>> single container, but that will have to be a follow up. >>> >>> Btw there are a lot of extra test cases in the langtools repository (!?!) >>> look in >>> langtools/test/tools/javac/annotations/repeatingAnnotations/combo/ReflectionTest.java >>> >>> cheers >>> /Joel >>> >>> On 22 okt 2013, at 18:50, Peter Levart >>> <peter.lev...@gmail.com> >>> wrote: >>> >>> >>>> Hi, I would just like to ask for a pointer to some specification document >>>> that describes inheritance of repeating annotations. I couldn't find one >>>> on the net. >>>> >>>> I have a feeling there's something wrong with the logic of >>>> AnnotationsSuport.getAssociatedAnnotations. Why? Because it is based on >>>> two maps: declaredAnnotations map and allAnnotations map. The later is a >>>> map of inherited and/or declared annotations which is aggregated without >>>> the knowledge of repeating annotations (the containers). I doubt this map >>>> keeps enough information to reconstruct a sound set of inherited and/or >>>> declared repeating annotations in all situations. >>>> >>>> But I'd like to 1st see the specification before showing you some examples >>>> where problems arise. >>>> >>>> Regards, Peter >>>> >>>> On 10/22/2013 12:21 PM, Joel Borggrén-Franck wrote: >>>> >>>>> Hi Andreas, >>>>> >>>>> A few nits: >>>>> >>>>> Class.java: >>>>> >>>>> import java.util.Collection; >>>>> +import java.util.Collections; >>>>> import java.util.HashSet; >>>>> >>>>> unused import. >>>>> >>>>> AnnotationSupport.java: >>>>> >>>>> + /** >>>>> + * Equivalent to calling {@code >>>>> getDirectlyAndIndirectlyPresentAnnotations( >>>>> + * annotations, annoClass, false)}. >>>>> + */ >>>>> >>>>> I think it is equivalent to annotations, annoClass, true >>>>> >>>>> Otherwise looks good. I can sponsor this fix. >>>>> >>>>> cheers >>>>> /Joel >>>>> >>>>> On 21 okt 2013, at 21:01, Andreas Lundblad >>>>> <andreas.lundb...@oracle.com> >>>>> wrote: >>>>> >>>>> >>>>>> Hi, >>>>>> >>>>>> New revision up for review: >>>>>> >>>>>> >>>>>> http://aoeu.se/webrevs/8019420-and-8004912/webrev.01 >>>>>> >>>>>> >>>>>> The following has been addressed since webrev.00: >>>>>> >>>>>> - Order of directly / indirectly present annotations now respects the >>>>>> order of the keys in the given map of annotations. >>>>>> >>>>>> - A new test has been added to test the above behavior. >>>>>> >>>>>> best regards, >>>>>> Andreas >>>>>> >>>>>> >>>>>> ----- Original Message ----- >>>>>> From: >>>>>> andreas.lundb...@oracle.com >>>>>> >>>>>> To: >>>>>> core-libs-dev@openjdk.java.net >>>>>> >>>>>> Sent: Wednesday, October 16, 2013 4:00:08 PM GMT +01:00 Amsterdam / >>>>>> Berlin / Bern / Rome / Stockholm / Vienna >>>>>> Subject: RFR: 8004912: Repeating annotations - getAnnotationsByType is >>>>>> not working as expected >>>>>> >>>>>> Hi, >>>>>> >>>>>> Please review the fix for JDK-8004912 and JDK-8019420 below. >>>>>> >>>>>> Description: >>>>>> >>>>>> The behavior of Class.get[Declared]AnnotationsByType was wrong. These >>>>>> methods delegate to sun.reflect.annotation.AnnotationSupport which has >>>>>> been rewritten. >>>>>> >>>>>> NonInheritableContainee.java is added and contains the test referred to >>>>>> in JDK-8019420. >>>>>> >>>>>> RepeatedUnitTest.java have been updated to include the test cases in >>>>>> JDK-8004912. >>>>>> >>>>>> There are more tests available in >>>>>> tl/langtools/test/tools/javac/annotations/repeatingAnnotations/combo/ReflectionTest.java >>>>>> (NB. this file is in the langtools repo) >>>>>> >>>>>> >>>>>> Link to web review: >>>>>> >>>>>> http://cr.openjdk.java.net/~alundblad/8019420-and-8004912/ >>>>>> >>>>>> >>>>>> Link to bug reports: >>>>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8004912 >>>>>> https://bugs.openjdk.java.net/browse/JDK-8019420 >>>>>> >>>>>> >>>>>> >>>>>> -- Andreas Lundblad >>>>>> >> >
