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
