RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Liam Miller-Cushon]

Hi, I finally have an update -

We've been using the patch internally for about six weeks, and it hasn't
caused any problems. The compatibility impact still appears to be minimal.

I've attached an updated version of the patch with the improvements to the
test coverage you suggested.

Are you still willing to help get the compatibility review started?

Thanks,
Liam

On Tue, Feb 16, 2016 at 9:48 AM, Joel Borggrén-Franck <
joel.borggren.franck at gmail.com> wrote:

> Hi Liam,
>
> Sorry for the delay,
>
> On Sat, 30 Jan 2016 at 04:45 Liam Miller-Cushon <cushon at google.com> wrote:
>
>> On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggrén-Franck <
>> joel.borggren.franck at gmail.com> wrote:
>>
>> But, the reason we didn't fix this the last two times we looked at it
>>> (that I am aware of) is the compatibility story. In order to fix his
>>> you need to figure out two things:
>>>
>>> - Is this is a spec change, that is, can we get away with throwing an
>>> AnnotationFormatError where we would now do? Should we clarify the
>>> spec?
>>>
>>
>> Can you clarify which parts of the spec might need to be updated? I can't
>> find anything relevant in the jls or jvms. The javadoc for AnnotatedElement
>> mentions some conditions under which TypeNotPresentException is thrown:
>>
>> "If an annotation returned by a method in this interface contains
>> (directly or indirectly) a Class-valued member referring to a class that is
>> not accessible in this VM, attempting to read the class by calling the
>> relevant Class-returning method on the returned annotation will result in a
>> TypeNotPresentException."
>>
>> So throwing TypeNotPresentException when an array-valued annotation
>> indirectly references an inaccessible class sounds like the right
>> behaviour, and is consistent with how the implementation currently handles
>> similar cases.
>>
>
> I think you answered my concerns. By the spec I meant either the Java
> Language Specification or the normative parts of the javadoc. This seems to
> be in line with what is currently specified.
>
>
>> - Since this is a behaviorally incompatible change, how big is the
>>> impact? This is of course a hard question to answer, but if one could
>>> do a corpus analysis over a large code base and look for catches of
>>> ArrayStoreExceptions when reflecting over annotations, that could be
>>> useful. If it turns out that "a lot" of users have adopted to this
>>> bug, perhaps it isn't worth fixing? On the other hand I can imagine
>>> that this is so uncommon that no one catches either type of error.
>>>
>>
>> I'm working on evaluating the impact. A review of our code base showed
>> that handling ArrayStoreException is fairly uncommon. Of the instances I
>> found, none of them were in code that was inspecting annotations.
>>
>> The next step is to start using the patch internally and see if anything
>> breaks. I'll update the thread when I have results on that.
>>
>
> Great, if the experiment works out I'll help formulate a change request
> for compatibility review.
>
> cheers
> /Joel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 7183985.3.patch
Type: text/x-patch
Size: 19441 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20160427/53b919af/7183985.3.patch>