Hi Liam, Sorry for the delay,
On Sat, 30 Jan 2016 at 04:45 Liam Miller-Cushon <cus...@google.com> wrote: > On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggrén-Franck < > joel.borggren.fra...@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
