JDK 12 RFR of JDK-8058202 : AnnotatedType implementations don't override toString(), equals(), hashCode()
joe darcy
joe.darcy at oracle.com
Thu Oct 11 19:12:15 UTC 2018
Hi Werner,
On 10/10/2018 1:23 PM, Werner Dietl wrote:
> Hi Joe, all,
>
> the logic looks good to me.
>
> In the tests I'm wondering whether to include an annotated wildcard
> bound. There is:
>
> 307 public @AnnotType(11) Set<@AnnotType(13) ? extends Number>
> fooNumberSet2() {return null;}
>
> but nothing like
>
> Set<? extends @AnnotType(13) Number> fooNumberSet2() {return null;}
>
> I wouldn't expect problems for this, but a test would exercise some of
> the code that gets added.
You were correct to probe at the bounds on the wildcard components; the
code that was reviewed and pushed does not print annotations on the bounds.
I'll work on an update to handle this and similar cases where there are
embedded types that could have annotations.
Thanks,
-Joe
>
> Best,
> cu, WMD.
>
>
>
>
> On Wed, Oct 10, 2018 at 2:40 AM Joel Borggrén-Franck
> <joel.borggren.franck at gmail.com> wrote:
>> 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
>>> <
>
>
More information about the core-libs-dev
mailing list