RFR: JDK-8042981: Strip type annotations in Types' utility methods [v5]
Liam Miller-Cushon
cushon at openjdk.org
Fri Jan 12 20:34:26 UTC 2024
On Fri, 12 Jan 2024 07:27:26 GMT, Joe Darcy <darcy at openjdk.org> wrote:
>> Early review for JDK-8042981: "Strip type annotations in Types' utility methods". I work more often in the Element world rather than the Type word of the annotation processing APIs.
>>
>> The type annotations on primitive types are *not* cleared by the existing annotation clearing mechanisms. I suspect Type.Visitor is missing a case for primitive types. Someone with familiarity with javac's type modeling should take a look; thanks.
>
> Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:
>
> Update implementation and tests.
Thanks!
re: `TypeMirror #TypeMirror#getAnnotation*(Class)`, I agree that's a bug, and also that it probably warrants a separate issue.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 429:
> 427: }
> 428: //where
> 429: private static final TypeMapping<Void> stripMetadata = new StructuralTypeMapping<Void>() {
It might be worth adding an implementation comment here to note that the visitor only needs to handle are the ones where 'contained' types can be annotated, which are the cases described by JVMS 4.7.20.2: classes (for type parameters and enclosing types), wildcards, and arrays.
test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java line 140:
> 138: }
> 139:
> 140: void checkEmptyAnnotations(AnnotatedConstruct ac) {
I think it would be valuable to expand the assertion to visit contained types, and assert that none of the contained types have annotations:
diff --git a/test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java b/test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java
index c627f41ba0b..8bb3c7cd3a8 100644
--- a/test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java
+++ b/test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java
@@ -82,10 +82,10 @@ public boolean process(Set<? extends TypeElement> annotations,
System.err.print("\tgetArrayType()");
ArrayType arrayType = typeUtils.getArrayType(returnType);
- checkEmptyAnnotations(arrayType);
/*
* "Annotations on the component type are preserved."
*/
+ checkEmptyAnnotations((AnnotatedConstruct) arrayType);
checkEqualTypeAndAnnotations(returnType, arrayType.getComponentType());
if (!returnType.getKind().isPrimitive()) {
@@ -93,10 +93,14 @@ public boolean process(Set<? extends TypeElement> annotations,
* "Annotations on the bounds are preserved."
*/
WildcardType wcType;
- checkEmptyAnnotations(wcType = typeUtils.getWildcardType(returnType, null));
+ System.err.print("\tgetWildcardType(returnType, null)");
+ wcType = typeUtils.getWildcardType(returnType, null);
+ checkEmptyAnnotations((AnnotatedConstruct) wcType);
checkEqualTypeAndAnnotations(returnType, wcType.getExtendsBound());
- checkEmptyAnnotations(wcType = typeUtils.getWildcardType(null, returnType));
+ System.err.print("\tgetWildcardType(null, returnType)");
+ wcType = typeUtils.getWildcardType(null, returnType);
+ checkEmptyAnnotations((AnnotatedConstruct) wcType);
checkEqualTypeAndAnnotations(returnType, wcType.getSuperBound());
}
@@ -158,6 +162,54 @@ void checkEmptyAnnotations(AnnotatedConstruct ac) {
}
}
+ void checkEmptyAnnotations(TypeMirror ac) {
+ System.err.println("\t" + ac);
+ if (ac == null) {
+ return;
+ }
+ new SimpleTypeVisitor14<Void, Void>() {
+ @Override
+ protected Void defaultAction(TypeMirror t, Void o) {
+ checkEmptyAnnotations((AnnotatedConstruct) t);
+ return null;
+ }
+
+ @Override
+ public Void visitArray(ArrayType t, Void o) {
+ scan(t.getComponentType());
+ return super.visitArray(t, o);
+ }
+
+ @Override
+ public Void visitDeclared(DeclaredType t, Void o) {
+ scan(t.getEnclosingType());
+ t.getTypeArguments().stream().forEach(this::scan);
+ return super.visitDeclared(t, o);
+ }
+
+ @Override
+ public Void visitTypeVariable(TypeVariable t, Void o) {
+ // the bounds correspond to the type variable declaration, not its use
+ // scan(t.getUpperBound());
+ // scan(t.getLowerBound());
+ return super.visitTypeVariable(t, o);
+ }
+
+ @Override
+ public Void visitWildcard(WildcardType t, Void o) {
+ scan(t.getExtendsBound());
+ scan(t.getSuperBound());
+ return super.visitWildcard(t, o);
+ }
+
+ private void scan(TypeMirror t) {
+ if (t != null) {
+ visit(t);
+ }
+ }
+ }.visit(ac);
+ }
+
void checkEqualTypeAndAnnotations(TypeMirror tm1, TypeMirror tm2) {
if (!typeUtils.isSameType(tm1, tm2)) {
failures++;
test/langtools/tools/javac/processing/model/util/types/TestAnnotationStripping.java line 189:
> 187: public static String @TestTypeAnnotation("foo5")[] foo5() {return null;}
> 188:
> 189: public static java.util. @TestTypeAnnotation("foo6") Set < @TestTypeAnnotation("foo7") String> foo6() {return null;}
Consider adding a case for type variable usages:
public static <@TestTypeAnnotation("foo8") T extends @TestTypeAnnotation("foo9") String> @TestTypeAnnotation("foo10") T foo8() {return null;}
This one raises a couple of questions about type annotations associated with the declaration of the type variable. I think probably only the annotations on the use of the type var need to be stripped, not the declaration.
Currently this causes failures for the assertion that `asElement()` has no annotations, because the annotations are only stripped from the type corresponding to the use of the type variable, not from the corresponding declaration.
Similarly `TypeVariable#getUpperBound` would report annotations.
-------------
PR Review: https://git.openjdk.org/jdk/pull/8984#pullrequestreview-1818788534
PR Review Comment: https://git.openjdk.org/jdk/pull/8984#discussion_r1450775093
PR Review Comment: https://git.openjdk.org/jdk/pull/8984#discussion_r1450896707
PR Review Comment: https://git.openjdk.org/jdk/pull/8984#discussion_r1450895756
More information about the compiler-dev
mailing list