repeating annotations comments

[Joe Darcy]

Hi Mike,
On 12/20/2012 7:48 PM, michael keith wrote:
> Just to follow up on this, I did a quick survey and there are a number 
> of EE 6 specs that make use of the idiomatic container pattern: EJB, 
> JPA, JAXB, JAX-WS, JSF, Interceptors, Connectors, and Common 
> Annotations. These are just the EE 6 specs, I didn't go through the 
> ongoing EE 7 ones. This means that all of the implementations, IDE's, 
> 3rd party tools and frameworks that process annotations from these 
> specs will break, in many cases imperceptibly, if the semantics of the 
> existing reflection methods get changed. As I mentioned earlier, while 
> I was prepared to require that implementations change once we upgraded 
> the specs to use repeatable annotations, I feel the fanout is too 
> broad and the manner of breakage too subtle that the cost of 
> abandoning compatibility is too high. If people feel there is 
> confusion around the new methods acting differently from the old ones 
> in the presence of repeatable annotations then perhaps we could go 
> back to exploring the use of different names for the new ones.

It would be very helpful for someone to verify that the idiomatic 
container pattern used in the various EE specs can be successfully 
retrofitted to use the formal container pattern in the current JDK 8 
builds.  Several adjustments were made to the initial formal container 
design to accommodate the actual usage patterns in EE.

Thanks,

-Joe

>
> -Mike
>
> On 17/12/2012 9:44 AM, Mike Keith wrote:
>> Thanks for your comments, ideas and suggestions, Alex. I had more 
>> answers but my draft got prematurely sent. More stuff below.
>>
>> -Mike
>>
>> On 2012-12-16, at 10:33 PM, Mike Keith<michael.keith at oracle.com>  wrote:
>>
>>> On 2012-12-14, at 8:41 PM, Alex Buckley<alex.buckley at oracle.com>  
>>> wrote:
>>>
>>>> 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".
>>> Sure, there is some responsibility to be shared by both of those 
>>> parties.The problem is that the call succeeds at all. It should 
>>> probably fail (eg return null) if there are multiples
>>>
>>>
>>>>> 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.)
>> I might be misunderstanding you, but I don't see how that category of 
>> legacy application would continue to work. It still won't be 
>> expecting multiples as it looks in the collection, so it will stop 
>> looking once it finds the first one.
>>
>>>>> 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.
>> No, sorry, I was just describing what the processor might have to do, 
>> not implying that a Set would be returned by any of the reflection 
>> calls.
>>
>>>> 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.
>> Yes, and the number of users that would actually make use of this 
>> pattern is admittedly minuscule. Unfortunately, the processing code 
>> generally ends up having to handle it anyway, so the pattern for 
>> processing it is going to be widespread.
>>
>>>> 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?
>>  From a compatibility perspective that would be great. Existing code 
>> would be able to handle existing applications, and new code could 
>> easily handle both old and new applications.
>>
>> I would just like to mention, though, that while we in EE-land would 
>> like compatibility we are also sensitive to non-legacy users and 
>> don't want to impose changes that too severely hamper the API or 
>> render it unintuitive in the future. We are certainly willing to make 
>> changes to code if necessary, but I was kind of hoping that we 
>> wouldn't be in the situation of existing code apparently working, but 
>> not really. I would be interested in hearing how others view the idea 
>> of the typed version of get[Declared]Annotations behaving differently 
>> than the legacy untyped one.