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

[joe darcy]

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
<