RFR: 8173056: Add test that captures current behavior of annotations with invalid annotation types

[Peter Levart]

Hi Claes,

On 01/18/2017 12:01 PM, Claes Redestad wrote:
> Hi Peter,
>
> On 01/17/2017 03:10 PM, Peter Levart wrote:
>>
>> As for the failure reporting: requesting an annotation for invalid 
>> annotation type now always produces AnnotationFormatError. 
>> Previously, some invalid annotations were simply skipped, some 
>> produced AnnotationFormatError and for some of them, 
>> AnnotationFormatError was raised only when evaluating Annotation's 
>> equals() method.
>>
>> I would like to discuss this failure behavior more in-depth.
>
> I don't have an example, but if there are applications that have invalid
> annotations which are currently ignored completely or until someone
> calls equals on them, this behavior change could result in some
> application breaking in unexpected ways.
>
> Although this change might be well-intended and align implementation
> better with specified intent, I fear this kind this behavior change 
> might be
> too late for 9 (maybe even for later releases), and think it'd be 
> easier to
> accept a patch that took great care not to change observable behavior
> and discuss this separately.
>
> Thanks!

Mind you that we are talking about the behavior of invalid annotations 
types (class files containing annotation interfaces which are invalid 
according to JLS) and not about the behavior of invalid annotation data 
(annotation attributes in the class files containing annotated 
types/members/...). The later behavior is not changed with the presented 
patch, just the behavior when an annotation type is encountered which 
must have been created by some other tool - not javac - and violates JLS 
for annotation types.

Anyway, the behavior of such invalid annotation types has already 
changed in the past (with a patch for the issue: 8035781: Improve 
equality for annotations) which introduced additional checks into the 
AnnotationInvocationHandler constructor and the equalsImpl() method 
which implements annotation's equals() method.

I extended the AnnotationVerifier test with additional cases that cover 
various "wrongs" in annotation types:

http://cr.openjdk.java.net/~plevart/jdk9-dev/8173056_AnnotationVerifier/webrev.01/

Running this on current jdk9 shows all the shades of behavior for 
different "wrongs":

HolderA.class.getAnnotations() = [@GoodAnnotation()]
test AnnotationVerifier.holderA_annotations(): success

HolderA.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderA_goodAnnotation(): success

HolderB.class.getAnnotation(AnnotationWithParameter.class) = null
test AnnotationVerifier.holderB_annotationWithParameter(): success

HolderB.class.getAnnotations() = [@GoodAnnotation()]
test AnnotationVerifier.holderB_annotations(): success

HolderB.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderB_goodAnnotation(): success

HolderC.class.getAnnotation(AnnotationWithVoidReturn.class) = 
java.lang.annotation.AnnotationFormatError: Invalid default: public 
abstract void AnnotationWithVoidReturn.m()
test AnnotationVerifier.holderC_annotationWithVoidReturn(): success

HolderC.class.getAnnotations() = 
java.lang.annotation.AnnotationFormatError: Invalid default: public 
abstract void AnnotationWithVoidReturn.m()
test AnnotationVerifier.holderC_annotations(): success

HolderC.class.getAnnotation(GoodAnnotation.class) = 
java.lang.annotation.AnnotationFormatError: Invalid default: public 
abstract void AnnotationWithVoidReturn.m()
test AnnotationVerifier.holderC_goodAnnotation(): success

HolderD.class.getAnnotation(AnnotationWithExtraInterface.class) = 
java.lang.annotation.AnnotationFormatError: Attempt to create proxy for 
a non-annotation type.
test AnnotationVerifier.holderD_annotationWithExtraInterface(): success

HolderD.class.getAnnotations() = 
java.lang.annotation.AnnotationFormatError: Attempt to create proxy for 
a non-annotation type.
test AnnotationVerifier.holderD_annotations(): success

HolderD.class.getAnnotation(GoodAnnotation.class) = 
java.lang.annotation.AnnotationFormatError: Attempt to create proxy for 
a non-annotation type.
test AnnotationVerifier.holderD_goodAnnotation(): success

HolderE.class.getAnnotation(AnnotationWithException.class) = 
@AnnotationWithException(m=1)
test AnnotationVerifier.holderE_annotationWithException(): success

@AnnotationWithException(m=1).equals(@AnnotationWithException(m=1)) = 
java.lang.annotation.AnnotationFormatError: Malformed method on an 
annotation type
test AnnotationVerifier.holderE_annotationWithException_equals(): success

HolderE.class.getAnnotations() = [@GoodAnnotation(), 
@AnnotationWithException(m=1)]
test AnnotationVerifier.holderE_annotations(): success

HolderE.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderE_goodAnnotation(): success

HolderF.class.getAnnotation(AnnotationWithHashCode.class) = 
@AnnotationWithHashCode(hashCode=1)
test AnnotationVerifier.holderF_annotationWithHashCode(): success

@AnnotationWithHashCode(hashCode=1).equals(@AnnotationWithHashCode(hashCode=1)) 
= java.lang.annotation.AnnotationFormatError: Malformed method on an 
annotation type
test AnnotationVerifier.holderF_annotationWithHashCode_equals(): success

HolderF.class.getAnnotations() = [@GoodAnnotation(), 
@AnnotationWithHashCode(hashCode=1)]
test AnnotationVerifier.holderF_annotations(): success

HolderF.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderF_goodAnnotation(): success

HolderG.class.getAnnotation(AnnotationWithDefaultMember.class) = 
@AnnotationWithDefaultMember(m=1)
test AnnotationVerifier.holderG_annotationWithDefaultMember(): success

@AnnotationWithDefaultMember(m=1).equals(@AnnotationWithDefaultMember(m=1)) 
= java.lang.annotation.AnnotationFormatError: Malformed method on an 
annotation type
test AnnotationVerifier.holderG_annotationWithDefaultMember_equals(): 
success

HolderG.class.getAnnotations() = [@GoodAnnotation(), 
@AnnotationWithDefaultMember(m=1)]
test AnnotationVerifier.holderG_annotations(): success

HolderG.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderG_goodAnnotation(): success

HolderX.class.getAnnotation(AnnotationWithoutAnnotationAccessModifier.class) 
= null
test 
AnnotationVerifier.holderX_annotationWithoutAnnotationAccessModifier(): 
success

HolderX.class.getAnnotations() = [@GoodAnnotation()]
test AnnotationVerifier.holderX_annotations(): success

HolderX.class.getAnnotation(GoodAnnotation.class) = @GoodAnnotation()
test AnnotationVerifier.holderX_goodAnnotation(): success


Cases that throw "AnnotationFormatError: Malformed method on an 
annotation type" and "AnnotationFormatError: Attempt to create proxy for 
a non-annotation type" are the cases that changed behavior with a patch 
for 8035781 and nobody complained. This patch causes 
AnnotationFormatError even for some valid cases (see bellow).

When AnnotationType.getInstance() throws IllegalArgumentException, the 
annotation is skipped while parsing it. This currently happens only when:
- the annotation interface is not marked with ACC_ANNOTATION access 
modifier (Class.isAnnotation() == false).
- the annotation interface contains a non-synthetic public abstract 
method with parameters.

But AnnotationType.getInstance() can also throw AnnotationFormatError 
which is propagated out of parsing logic and out of public 
annotation-obtaining method(s). This currently happens only when:
- the annotation interface contains a non-synthetic public abstract 
method with invalid return type (such as void) because 
Method.getDefaultValue() throws such error which is propagated out of 
AnnotationType constructor.

AnnotationInvocationHandler constructor throws AnnotationFormatError 
which is propagated out of parsing logic and out of public 
annotation-obtaining method(s). This currently happens only when:
- the annotation interface is marked with ACC_ANNOTATION access modifier 
but it either doesn't extend java.lang.annotation.Annotation or it 
extends any additional interfaces.

In all other "wrong annotation type" cases (and some valid cases too), 
the annotation is successfully constructed and returned, but such 
annotation later fails with AnnotationFormatError when .equals() method 
is evaluated on it. Those cases are:

- the annotation interface contains a method (of any kind: synthetic, 
abstract, default, static, private) that declares a thrown exception or 
when it contains a non-abstract or non-public method. The perfectly 
valid situation when this happens is the following example:

public class AnnotationWithLambda {

     @Retention(RetentionPolicy.RUNTIME)
     @interface Ann {
         Callable<Void> callable = () -> {throw new Exception();};
     }

     @Ann
     static class Holder1 {}

     @Ann
     static class Holder2 {}

     public static void main(String[] args) {
         Ann ann1 = Holder1.class.getAnnotation(Ann.class);
         Ann ann2 = Holder1.class.getAnnotation(Ann.class);

         // throws AnnotationFormatError: Malformed method on an 
annotation type
         ann1.equals(ann2);
     }
}

This happens as a consequence of fix for 8035781 which introduced a very 
rigorous validation in annotation's equals method which is more rigorous 
than it should be.

- the annotation interface contains a method (of any kind: synthetic, 
abstract, default, static, private) that has an invalid return type
- the annotation interface contains a method (of any kind: synthetic, 
abstract, default, static, private) which is override equivalent with 
public/protected methods in Object class or Annotation interface.


Do we really want to keep this behavior?

I suggest that when an annotation type is not marked with ACC_ANNOTATION 
modifier (Class.isAnnotation() == false) such annotations are skipped 
when parsing. This can happen when a former annotation type is changed 
to be a normal interface and separately compiled. In all other cases the 
invalid annotation type should be reported when annotations are parsed.

Perhaps this could be performed lazily. An idea:

During parsing of annotations, if an annotation for invalid annotation 
type that has ACC_ANNOTATION modifier is being parsed, the parsing skips 
such annotation and returns a special Proxy implementation implementing 
the invalid annotation type, but this proxy throws AnnotationFormatError 
on any method being invoked on it. This would not prevent other valid 
annotations to be obtained on annotated elements just because they are 
also annotated with invalid annotations.

What do you think?

Regards, Peter