[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not p
From:       Liam Miller-Cushon <cushon () google ! com>
Date:       2016-01-30 3:45:32
Message-ID: CAL4QsgvPx1X9Ga1S_HDdmYxj9ti5+6XBA5z35bZbDhVPZwxtuA () mail ! gmail ! com
[Download RAW message or body]

On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggr=C3=A9n-Franck <
joel.borggren.franck@gmail.com> wrote:

> Thanks for the update. I think it makes sense to expand testing
> slightly, testing all three parsing clauses that you fixed, and for
> all three of them also checking that a "thing" after the broken item
> is parsed correctly.
>

Sure, I'd be happy to add additional tests if it looks like the other
questions can be addressed.


> 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.

- 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.
[prev in list] [next in list] [prev in thread] [next in thread]