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

[Vicente Romero]

I'm not an expert in the area, but the patch looks good to me,

Vicente

On 05/31/2018 08:39 PM, Liam Miller-Cushon wrote:

Hi,

Are there any concerns with the patch that I can address? I was hoping to
get this in to JDK 11.

I confirmed that it still builds and passes all tests at head.

Thanks,
Liam

On Mon, May 14, 2018 at 10:38 AM Liam Miller-Cushon 
wrote:


Ping

On Thu, Apr 5, 2018 at 10:57 AM Liam Miller-Cushon 
wrote:


Hi,

Is there any more feedback on the patch?

On Wed, Feb 28, 2018 at 4:26 PM Liam Miller-Cushon 
wrote:


On Wed, Feb 28, 2018 at 4:13 PM, Martin Buchholz 
wrote:


Follow their lead and rename your method to assertArrayEquals


Done: http://cr.openjdk.java.net/~cushon/7183985/webrev.04/

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

[Martin Buchholz]

I was surprised to learn that junit has a magic assertEquals specifically
for Object[]
assertEquals(Object[], Object[])
but that is obviously a bad idea and junit eventually deprecated it.
https://junit.org/junit4/javadoc/4.12/org/junit/Assert.html#assertEquals(java.lang.Object[],
java.lang.Object[])
Follow their lead and rename your method to assertArrayEquals

 124 static void assertEquals(Object[] actual, Object[] expected) {

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

[Joseph D. Darcy]

Hi Liam,

On 2/24/2018 5:14 PM, Liam Miller-Cushon wrote:

Hi, thanks for the comments.

The updated webrev is at: 
http://cr.openjdk.java.net/~cushon/7183985/webrev.02/ 



On Fri, Feb 23, 2018 at 4:29 PM, Joseph D. Darcy > wrote:


Objects implementing the AnnotatedElement interface are also
created in javac during annotation processing. As a secondary
concern, it would be good to be have behavior between both javac
and runtime annotations consistent when possible. (My own to-do
list includes at least once such alignment JDK-8164819: "Make
javac's toString() on annotation objects consistent with core
reflection".)


Do you have a pointer to where that happens? There's 
javax.lang.model.AnnotatedConstruct#getAnnotation, which isn't 
affected by this issue--it throws MirroredTypesExceptionProxy when 
accessing an annotation value that is an array of class literals, 
regardless of whether the elements' classes are missing. I'm not 
seeing implementations of AnnotatedElement in langtools.


Sorry, I misremembered the situation in javac. For annotation processing 
in javac, AnnotatedElement is not a factor, but the class


src/jdk.compiler/share/classes/com/sun/tools/javac/model/AnnotationProxyMaker.java

in javac does create annotation proxies, which is the same general 
technique used to create the annotation objects at runtime for core 
reflection.  These annotation objects in javac for javax.lang.model and 
annotation processing, do not follow the full contract of 
java.lang.reflect.AnnotatedElement. In particular, the annotations 
returned are not necessarily serializable. The intersection of methods 
on AnnotatedConstruct and AnnotatedElement is only a proper subset of 
the methods on AnnotatedElement; however, getAnnotation​(Class 
annotationClass) is in the intersection.



Even in javac we've moved away from test and directory names based
on bugid. I'd recommend incorporating these regression tests into
the existing tests in
test/jdk/java/lang/annotation/Missing


Thanks for the reminder, done.



I believe the new tests could reuse some of the existing types in 
test/jdk/java/lang/annotation/Missing. For example, the new 
MissingAnnotation.java is an alpha-rename of the existing Missing.java. 
If such sharing is not practical, then I'd recommend putting the new 
files into a subdirectory under underneath test/.../annotation/Missing 
(otherwise it will be confusing to edit these tests in the future since 
too many file names will start with "Missing".)


In MissingClassArrayElementTest.java:

  71 public static void main(String[] args) throws Exception {
  72 ClassArrayAnnotation[] outer = 
Test.class.getAnnotation(AnnotationAnnotation.class).value();
  73 // The second entry in the values array can be loaded 
successfully
  74 assertEquals(Arrays.toString(outer[1].value()), "[class 
java.lang.String]");


Something like

assertEquals(Arrays.equals(outer[1].value(), {String.class})

would be more robust against any future changes in Class.toString. 
Likewise for the analogous comparisons.



It would be worth verifying whether or not this change also covers
java.lang.reflect.Executable.getParameterAnnotations(), more
specifically the implementation of that method in Method and
Constructor. The method getParameterAnnotations() is much less
used than getAnnotations and the other methods on the
AnnotatedElement interface, but still part of the annotations feature.


Done.


Thanks.

Cheers,

-Joe

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

[Joseph D. Darcy]

Hello,

A few initial comments, not my final review.

Objects implementing the AnnotatedElement interface are also created in 
javac during annotation processing. As a secondary concern, it would be 
good to be have behavior between both javac and runtime annotations 
consistent when possible. (My own to-do list includes at least once such 
alignment JDK-8164819: "Make javac's toString() on annotation objects 
consistent with core reflection".)


Even in javac we've moved away from test and directory names based on 
bugid. I'd recommend incorporating these regression tests into the 
existing tests in


test/jdk/java/lang/annotation/Missing

or creating a subdirectory for conditions, etc..

It would be worth verifying whether or not this change also covers 
java.lang.reflect.Executable.getParameterAnnotations(), more 
specifically the implementation of that method in Method and 
Constructor. The method getParameterAnnotations() is much less used than 
getAnnotations and the other methods on the AnnotatedElement interface, 
but still part of the annotations feature.


(As a follow-up refactoring, it might be worthwhile to replace the 
interior of the three parseFooArray methods to a shared method that is 
passed a lambda for the "Object value = parseFooValue(/*args to get 
foo*/...);" logic.)


Thanks,

-Joe

On 2/23/2018 10:06 AM, joe darcy wrote:

On 2/23/2018 9:39 AM, Alan Bateman wrote:

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places 
ArrayStoreException
was being explicitly handled, and none that were depending on the 
current

behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will 
reply to your review with any background or issues from when this 
came up in the past. From a distance then retrofitting 
AnnotatedElement getXXX methods to throw TypeNotPresentException 
seems reasonable, I'm less sure about the isAnnotationPresent method 
as it might be surprising for that to fail.




On my list!

Cheers,

-Joe

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

[joe darcy]

On 2/23/2018 9:39 AM, Alan Bateman wrote:

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places 
ArrayStoreException
was being explicitly handled, and none that were depending on the 
current

behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will 
reply to your review with any background or issues from when this came 
up in the past. From a distance then retrofitting AnnotatedElement 
getXXX methods to throw TypeNotPresentException seems reasonable, I'm 
less sure about the isAnnotationPresent method as it might be 
surprising for that to fail.




On my list!

Cheers,

-Joe

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

[Alan Bateman]

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places ArrayStoreException
was being explicitly handled, and none that were depending on the current
behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will reply 
to your review with any background or issues from when this came up in 
the past. From a distance then retrofitting AnnotatedElement getXXX 
methods to throw TypeNotPresentException seems reasonable, I'm less sure 
about the isAnnotationPresent method as it might be surprising for that 
to fail.


-Alan

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

[Joel Borggrén-Franck]

Hi Liam,

Sorry for the delay,

On Sat, 30 Jan 2016 at 04:45 Liam Miller-Cushon  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

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

[Liam Miller-Cushon]

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.

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

[Joel Borggrén-Franck]

Hi,

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.

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?

- 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 can't help you with the internal processes at Oracle anymore, but if
you are interested in doing a compatibility analysis I can make sure
it reaches the right people.

cheers
/Joel


On Thu, Jan 7, 2016 at 7:59 PM, Liam Miller-Cushon  wrote:
> Hi Joel -
>
> On Tue, Jan 5, 2016 at 1:16 PM, Joel Borggrén-Franck
>  wrote:
>>
>> I think you need to do skipMemberValue the correct number of times so
>> that the cursor in buf is correct, or?
>
>
> Thanks for the catch. I added your test case, and the exception proxy is now
> stored and returned after the entire array is read.
>
>>
>> Also, in MissingArrayElementTest, why not just rethrow t?
>
>
> Done.
>
> Thanks!

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

[Joel Borggrén-Franck]

Hi Liam,

I think you need to do skipMemberValue the correct number of times so
that the cursor in buf is correct, or?

I modified your test slightly to provoke an
AnnotationTypeMismatchException that I think shouldn't be there:

@AnnotationAnnotation({@ClassArrayAnnotation({Missing.class, String.class}),
   @ClassArrayAnnotation({String.class})})
public class F {}

and

@Retention(RUNTIME)
public @interface AnnotationAnnotation {
ClassArrayAnnotation[] value();
}

giving:

java.lang.RuntimeException:
java.lang.annotation.AnnotationTypeMismatchException: Incorrectly
typed data found for annotation element public abstract
ClassArrayAnnotation[] AnnotationAnnotation.value() (Found data of
type Array with component tag: 99)

Also, in MissingArrayElementTest, why not just rethrow t?

 52 if (t.getClass() != EnumConstantNotPresentException.class) {
 53 System.err.printf(
 54 "expected %s, saw %s\n",
EnumConstantNotPresentException.class, t);
 55 throw new RuntimeException(t);
 56 }

cheers
/Joel

On Mon, Dec 21, 2015 at 9:42 PM, Liam Miller-Cushon  wrote:
> If an annotation value is an array of class literals or enum constants,
> calling Class.getAnnotation() fails with ArrayStoreException if the element
> type is not found.
>
> This patch implements the proposed fix from the bug thread, which allows
> getAnnotation() to succeed and a TypeNotPresentException to be thrown from
> Annotation.value().
>
> bug: https://bugs.openjdk.java.net/browse/JDK-7183985
>
> The patch is attached.