repeating annotations comments

[Alex Buckley]

Hi Mike,

Thanks for the quick feedback. (I know you've been playing with the JDK8 
b68 binary.)

On 12/14/2012 1:59 PM, michael keith wrote:
> 1. The getAnnotation(Class<T>) method is being changed to look through
> container annotations. This method returns an Annotation, i.e. a single
> instance, so applying it to repeated ones is necessarily going to have
> to return only one. But having it look through container annotation ends
> up not being very productive, and may be causing it to be
> counter-productive.
>
> Look-through for this method is not productive because the call is being
> made precisely in the legacy cases when only one annotation was
> expected, but a container annotation will only be synthesized when
> multiples are present. So look-through does not really help get "the"
> expected annotation. The only case when it would be useful is in the
> unlikely case when the container annotation is used explicitly and
> contains a single contained annotation.

I feel the author of the repeatable annotation type (who "opted in") and 
the author of the multiple annotations (@Foo @Foo class C ...) bear some 
responsibility when a legacy caller of getAnnotation(Class<T>) sees 
one-out-of-many annotations. Someone decided there is no more of "'the' 
expected annotation".

> It may be counter-productive for three reasons.
> The first is because getting a random single annotation in the presence
> of multiples is potentially dangerous since the calling code will
> continue without knowing that multiples are present, so legacy code is
> essentially broken, but will not actually fail. It will just be
> semantically incorrect.

Acknowledged.

> The second is because I didn't see (and I may just have missed it) where
> it was defined that if both T and TC were present that T would be
> guaranteed to be returned and not some instance within TC. Example 1.2-3
> shows this to be the case but I didn't see that it was specified
> behavior rather than just because the T occurred before the TC. If the
> example had been TC and then T would the instance of T be returned?

If both @T and @TC are present, we have not yet specified [1] whether 
get[Declared]Annotation(Class<T>) returns always the lone @T v. always a 
containerized @T v. something different every time. That's what your 
feedback is for :-)

[1] 
http://download.java.net/jdk8/docs/api/java/lang/reflect/AnnotatedElement.html#getAnnotation%28java.lang.Class%29

> This could be important because existing code to handle this case
> checks for the container annotation TC and then checks for the single
> annotation T. However, if checking for T happens to look through TC
> and return one of the contained annotations in TC instead of the lone
> T annotation then the lone annotation would not get found.

The implementation currently "prefers" a lone @T over a @T inside @TC. 
If you have a declaration  @TC(...) @T class C {}  then 
C.class.getAnnotation(T.class) will return the standalone @T. So 
existing code to handle this case will still work.

> The third is because all of the code that is currently out there that
> needs to check for the previous case, i.e. both T and TC occurring, will
> get a false positive in the simpler case of multiple instances of T
> annotations (as per the second part of example 1.2-1 in the doc). The
> getAnnotation(TC) will return the synthesized TC containing all of the
> instances of T, and then getAnnotation(T) will return one of those same
> instances again. Legacy code will assume that the result of
> getAnnotation(T) is disjoint from the contained annotations of the TC
> returned by getAnnotation(TC).

Acknowledged.

> 2. Given that get[Declared]Annotations() is going to change and return
> multiple instances of the same annotation (rather than a single instance
> of a container annotation) existing implementations are going to have to
> be fixed since there is no backwards compatibility there at all.

Acknowledged. The category of existing implementations which assume 
there will never be more than one annotation of a given type in the 
returned array is in for a surprise.

But there is a category of existing implementations which will continue 
to work fine. Suppose code calls get[Declared]Annotations() and looks 
first for @T in the array; only if @T is not found does the code look 
for @TC in the array. This seems like rational behavior for a 
non-repeating world, as it means one call to get annotations rather than 
getAnnotation(T.class) plus getAnnotation(TC.class). Now, run the code 
on SE 8 to get the new behavior of get[Declared]Annotations(). The code 
will find an @T in the array (the first of many, perhaps) and there will 
be no search for @TC and everything will be fine. (Assume a top-level @T 
is returned before containerized @T's.)

> 3. In general, it appears that once an annotation is made repeatable the
> annotation processing code is inevitably going to have to change in
> order to handle the repeatability. We were willing to take that hit,
> particularly since the new behavior requires opting in, and that means
> that specs could do so at their own discretion. However, the code to
> handle both the new and the old (i.e. be able to process both new
> repeatable annotations as well as the older idiomatic container
> annotations) is a little less intuitive to write than I had hoped it
> would be. The easiest way might just be to create a Set of instances to
> avoid duplicates. Not rocket science, just not what I would have expected.

I'm not sure if you are proposing that AnnotatedElement should return 
Set instances or your framework code should create Set instances. By 
policy, core reflection does not depend on the rest of the Java SE API, 
so uses array types as aggregates.

So, where are we? Joe and I have always recognized that changing 
getAnnotation(Class<T>) and get[Declared]Annotations() to look through 
official containers would affect legacy callers who assume there is at 
most one @T. A legacy caller which allows for both @T and @TC is perhaps 
hit harder than the rest - we weren't sure how many legacy callers were 
this careful, but you imply it's non-trivial.

Here's an idea. We pondered having the "existing" methods - 
get[Declared]Annotation(Class<T>) and get[Declared]Annotations() - be 
unaware of official containers, so they would return exactly what's in 
the class file. Essentially this is perfect behavioral compatibility. 
Meanwhile, the new get[Declared]Annotations(Class<T>) methods would look 
through official containers if present. How does that grab you?

Alex