On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggrén-Franck < joel.borggren.fra...@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.
