trailing comment

[michael keith]

On 09/01/2013 9:12 PM, michael keith wrote:
> On 08/01/2013 8:27 PM, Alex Buckley wrote:
>> Hi Mike,
>>
>> On 1/7/2013 7:03 PM, michael keith wrote:
>>> - p. 4 - Is there a use case for allowing @FooContainer to be
>>> @Documented/@Inherited when @Foo is not? In most cases seemingly
>>> arbitrary limitations are bad, but in this case I don't see a need for
>>> allowing it, and it might actually be detrimental to do so. For 
>>> example,
>>> if I make a mistake as the annotation developer and during the 
>>> course of
>>> development I no longer want @Foo to be @Inherited so I remove the
>>> meta-annotation from @Foo but forget to remove it from @FooContainer.
>>> When a user goes to use @Foo and annotates their UserClass with it 
>>> their
>>> UserSubClass won't see it, as expected. However, if they add another
>>> @Foo then both will be wrapped in a @FooContainer and now UserSubClass
>>> will be able to see them using getAllAnnotations(Foo.class) because the
>>> container is inherited. I am just wondering if the feature was 
>>> motivated
>>> by a case that someone thought would be useful, or if it was a case of
>>> trying not to impose unnecessary limitations. In general I am concerned
>>> about the maintenance issue of keeping the two annotations in sync. If
>>> they are allowed to be out of sync then the IDE won't be able to 
>>> help me.
>>
>> Simplicity a.k.a. "both are @Inherited, or neither is" is good, but 
>> unnatural because there's no solid link from a containing annotation 
>> type to a repeatable annotation type. Consider these types:
>>
>> @Inherited
>> @Repeatable(FooContainer.class)
>> @interface Foo {}
>>
>> @Inherited
>> @interface FooContainer { Foo[] value; }
>>
>> @Inherited
>> @interface RandomContainer { Foo[] value; }
>>
>> When compiling Foo, you can't fail to notice @Repeatable, so it's 
>> reasonable to load up the named FooContainer and enforce 
>> "Foo-is- at Inherited --> FooContainer-is- at Inherited".
>>
>> When compiling FooContainer and RandomContainer, you don't check 
>> anything about Foo. The presence of Foo in a return type is not 
>> significant enough for a compiler to load Foo and ponder a two-way 
>> relationship. It would be a wasted effort for RandomContainer, a 
>> perfectly legal type which has nothing to do with repeatability. For 
>> FooContainer, the compiler would be happy that it and Foo are 
>> @Inherited, but that's not authoritative as I can swap in a 
>> non- at Inherited FooContainer.class at runtime. (That's why core 
>> reflection has the AnnotationFormatError check on page 10.)
>>
>> In summary, I think the compiler should not be expected to "save" an 
>> annotation type owner who removes @Inherited from Foo but leaves it 
>> on FooContainer.
>
> I'm not convinced the compiler needs to verify both directions; I 
> think it only needs to verify Foo. Foo is the annotation of interest, 
> the one that will get modified during development. FooContainer is the 
> uninteresting "support annotation" needed for repeatability and will 
> likely be completely ignored once created. The only way for it to be 
> "out of sync" with Foo is if somebody goes back and modifies 
> FooContainer after having compiled Foo. In that unlikely case it is no 
> different than if somebody removes a method from a class and 
> recompiles the class, even though another class may be invoking the 
> removed method. Either the IDE "saves" or figures out the referencer 
> and an error shows up on the referencing class, or an exception occurs 
> at runtime. In our annotation case, the IDE could do the same, i.e. 
> remember the annotation/container pairs, or the AnnotationFormatError 
> will be eagerly thrown when Foo is first loaded. That makes sense to 
> me, and seems a more reasonable behavior than the incorrect and 
> unexpected outcome in the scenario I described earlier.

I should also have mentioned that the current restrictions aready result 
in the load-time exception (but no compile-time exception) if a 
container type is modified to *not* be @Inherited, when Foo is. Because 
there is no link back to Foo, both that case plus the case of a 
containing annotation type having a lesser Retention setting than the 
type it contains, will not be caught when compiling the containing type. 
They will only be noticed when the contained annotation type is loaded 
and validated for well-formed repeatability.

> In any case, it's not a showstopper issue, I just can't think of a 
> good use case for differing @Inherited specs (can you?) but I do see 
> some reasons for not allowing it.
>>> - p. 6 - The note after the first para in 9.7 states:
>>>      "This rule implements the policy that an annotation may repeat at
>>> only some of the locations where the annotation may appear. See §9.6 
>>> for
>>> more details."
>>> I don't see how the first para of 9.7 defines that behavior. Am I
>>> missing something? (It is defined in section 9.6 by the allowance of
>>> different Targets, in any case.)
>>
>> We have a policy that an annotation may repeat at only some of the 
>> locations where the annotation may appear. This is encoded in 9.6's 
>> rules about targets, but it's not enforced until someone actually 
>> writes @Foo @Foo at a given location - then 9.7 comes into play by 
>> requiring the containing type to be legal there.
>>
>> The reason I also require the repeatable type to be legal there is 
>> because I plan in JLS8 to centralize rules about annotation legality 
>> which are scattered throughout JLS7.
>
> Ok, thanks. Following the train of thought that led you to include 
> that statement was the key.
>
>>> - p. 9 - Feels like a typo, but the para that begins:  "An annotation A
>>> is directly present on an element E if ... " should finish by saying
>>> "... contains exactly one annotation C whose type is the containing
>>> annotation type of A's type (§9.6) and whose value element contains *at
>>> least one annotation of type* A."
>>
>> That would allow @A(1) to be directly present on an element if 
>> there's a container annotation with value={@A(2)}.
>
> Sorry, yes, I had a brain fart on the first phrase.
>
>>> - p. 18 - Cut/paste typo in para that begins: "Now suppose Foo is made
>>> repeatable ...". The bracketed part should be removed since it was
>>> stated above that FooContainer was inheritable (Foo may or may not be
>>> inheritable).
>>
>> Thanks, corrected.
>>
>>> - p. 18 - Missing closing bracket in
>>> "B.class.getAllDeclaredAnnotations(FooContainer.class = [ ]"
>>
>> Thanks, corrected.
>>
>> Alex