RFR: JDK-8042981: Strip type annotations in Types' utility methods [v5]

[Joe Darcy]

On Fri, 12 Jan 2024 20:28:00 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

>> Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update implementation and tests.
>
> 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 ch...

> 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
> 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++;
> ```

I've added a variation of the new code to the PR. The checks I added are in line with the properties discussed by the API changes in Types. Thanks.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/8984#discussion_r1459485081