JDK 12 RFR of JDK-8058202 : AnnotatedType implementations don't override toString(), equals(), hashCode()

[Joel Borggrén-Franck]

Hej Joe,

New version looks good!

Thanks for the explanations, makes sense to me.

Cheers
/Joel

On Wed, 10 Oct 2018 at 08:27, joe darcy <joe.darcy at oracle.com> wrote:

> Hi Joel,
>
> Thanks for the review; slightly revised version at
>
>      http://cr.openjdk.java.net/~darcy/8058202.3/
>
> Comments below.
>
>
> On 10/9/2018 11:00 AM, Joel Borggrén-Franck wrote:
> > Hi Joe,
> >
> > Good to see this happening. In general looks good to me, a few nits
> below.
> >
> > AnnotatedTypeBaseImpl contains comments indicating from which
> > superclass or interface the overridden methods comes. I'd either add
> > // Object at line 207 or delete line 145 and 177 for consistency.
>
> Okay; comments added to follow that local convention.
>
> >
> > Even though this isn't performance critical by far the allocation at
> > line 215 still bugs me a bit given that the common case will most
> > certainly be no annotations.
>
> Sure; refactored to avoid the allocation.
>
> >
> > Why the inverted logic line 250-253, IIRC you should be able to test
> > if it is an AnnotatedBaseTypeImpl, or?
>
> I was aiming to avoid testing for the impl class directly to not
> directly tie the classes to this particular implementation of the
> AnnotatedType hierarchy. However, inter-operability based on the specs
> would need the equals and hashCode behavior to be defined.
>
> >
> > For equalsTypeAndAnnotations and baseHashCode lines 232-244 equals
> > test for owner type equality while hashcode doesn't include owner
> > type. I must confess I no longer remember the equals-hashCode contract
> > but I assume this is intentional.
>
> Good catch; equals and hashCode checks made consistent.
>
> Some refactoring and hardening of the test included too.
>
> Delta-diffs below.
>
> Thanks,
>
> -Joe
>
> diff
> webrev.2/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>
> webrev/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>
>
> 2c2
> <  * Copyright (c) 2013, 2015, Oracle and/or its affiliates. All rights
> reserved.
> ---
>  >  * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights
> reserved.
> 207c207
> <         @Override
> ---
>  >         @Override // java.lang.Object
> 215d214
> <             StringJoiner sj = new StringJoiner(" ");
> 216a216
>  >         StringJoiner sj = new StringJoiner(" ");
> 228,229c228,231
> <             }
> <             return sj.toString();
> ---
>  >         return sj.toString();
>  >             } else {
>  >         return "";
>  >         }
> 244c246,247
> <                 Objects.hash((Object[])getAnnotations());
> ---
>  >                 Objects.hash((Object[])getAnnotations()) ^
>  >         Objects.hash(getAnnotatedOwnerType());
>
> diff
> webrev.2/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>
> webrev/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>
>
> 142d141
> <
> 157,168c156,158
> <             AnnotatedType annotType1 = m.getAnnotatedReturnType();
> <             AnnotatedType annotType2 = m.getAnnotatedReturnType();
> <
> <             boolean valid = annotType1.equals(annotType2);
> <
> <             if (!valid) {
> <                 errors++;
> <                 System.err.println(annotType1);
> <                 System.err.println(" is not equal to ");
> <                 System.err.println(annotType2);
> <                 System.err.println();
> <             }
> ---
>  >         checkTypesForEquality(m.getAnnotatedReturnType(),
>  >                   m.getAnnotatedReturnType(),
>  >                   true);
> 171a162,185
>  >     private static void checkTypesForEquality(AnnotatedType annotType1,
>  >                           AnnotatedType annotType2,
>  >                           boolean expected) {
>  >     boolean comparison = annotType1.equals(annotType2);
>  >
>  >     if (comparison) {
>  >         int hash1 = annotType1.hashCode();
>  >         int hash2 = annotType1.hashCode();
>  >         if (hash1 != hash2) {
>  >         errors++;
>  >         System.err.format("Equal AnnotatedTypes with unequal hash
> codes: %n%s%n%s%n",
>  >                   annotType1.toString(), annotType2.toString());
>  >         }
>  >     }
>  >
>  >     if (comparison != expected) {
>  >         errors++;
>  >         System.err.println(annotType1);
>  >         System.err.println(expected ? " is not equal to " : " is
> equal to ");
>  >         System.err.println(annotType2);
>  >         System.err.println();
>  >     }
>  >     }
>  >
> 184,196c198,200
> <                     AnnotatedType annotType1 =
> methods[i].getAnnotatedReturnType();
> <                     AnnotatedType annotType2 =
> methods[j].getAnnotatedReturnType();
> <
> <                     boolean valid = !annotType1.equals(annotType2);
> <
> <                     if (!valid) {
> <                         errors++;
> <                         System.err.println(annotType1);
> <                         System.err.println(" is equal to ");
> <                         System.err.println(annotType2);
> <                         System.err.println();
> <                     }
> <
> ---
>  > checkTypesForEquality(methods[i].getAnnotatedReturnType(),
>  >                       methods[j].getAnnotatedReturnType(),
>  >                       false);
> 209,210c213
> <         Method[] methods1  = clazz1.getDeclaredMethods();
> <         Method[] methods2 = clazz2.getDeclaredMethods();
> ---
>  >         System.err.println("Testing that presence/absence of
> annotations matters for equals comparison.");
> 212,226c215,230
> <         // Skip 0th element since void cannoted be annotated
> <         for (int i = 1; i < methods1.length; i++) {
> <             AnnotatedType annotType1 =
> methods1[i].getAnnotatedReturnType();
> <             AnnotatedType annotType2 =
> methods2[i].getAnnotatedReturnType();
> <
> <             boolean valid  = !annotType1.equals(annotType2);
> <
> <             if (!valid) {
> <                 errors++;
> <                 System.err.println(annotType1);
> <                 System.err.println(" is equal to ");
> <                 System.err.println(annotType2);
> <                 System.err.println();
> <             }
> <         }
> ---
>  >     String methodName = null;
>  >         for (Method method :  clazz1.getDeclaredMethods()) {
>  >         if ("void".equals(method.getReturnType().toString())) {
>  >         continue;
>  >         }
>  >
>  >         methodName = method.getName();
>  >         try {
>  >         checkTypesForEquality(method.getAnnotatedReturnType(),
>  > clazz2.getDeclaredMethod(methodName).getAnnotatedReturnType(),
>  >                       false);
>  >         } catch (Exception e) {
>  >         errors++;
>  >         System.err.println("Method " + methodName + " not found.");
>  >         }
>  >     }
> 230a235
>  >         System.err.println("Testing wildcards");
> 242,248c247
> <         if (awt1.equals(awt2)) {
> <             errors++;
> <             System.err.println(awt1);
> <             System.err.println(" is equal to ");
> <             System.err.println(awt2);
> <             System.err.println();
> <         }
> ---
>  >     checkTypesForEquality(awt1, awt2, false);
> 254d252
> <
>