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

[Joel Borggrén-Franck]

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.

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.

Why the inverted logic line 250-253, IIRC you should be able to test
if it is an AnnotatedBaseTypeImpl, or?

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.

Cheers
/Joel


On Mon, Oct 8, 2018 at 10:45 PM joe darcy <joe.darcy at oracle.com> wrote:
>
> Hello,
>
> Please review the changes to address:
>
>      JDK-8058202 : AnnotatedType implementations don't override
> toString(), equals(), hashCode()
>      http://cr.openjdk.java.net/~darcy/8058202.2/
>
> Some discussion and explanation of the changes:
>
> The java.lang.reflect.AnnotatedType interface hierarchy was added in JDK
> 8 to support type annotations
> (http://openjdk.java.net/projects/type-annotations/) in core reflection.
> It offers a parallel set of interfaces to the java.lang.reflect.Type
> family of interfaces added in JDK 5. The separate AnnotatedType
> interfaces are needed since type annotations are used on type *uses* and
> not type declarations. For example, it is necessary to be able to model
> both of the types of the fields foo and bar in a case like:
>
>      @Nonnull String foo;
>      @Nullable String bar;
>
> so clearly the same object cannot be used for both as the annotated type
> of the fields since the type annotations differ.
>
> The implementation classes for the AnnotatedType family used in core
> reflection are in a single file AnnotatedTypeFactory.java. There are
> instantiated instances both of the base class AnnotatedTypeBaseImpl and
> its subclasses which implement the specialized subinterfaces of
> AnnotatedType for arrays, type variables, etc.
>
> The implementation classes do *not* declare equals, hashCode, or
> toString methods, which the proposed change rectifies.
>
> For equals and hashCode, the interfaces do *not* define how the equals
> relation should be calculated, an omission shared by the Type
> interfaces. While the default identity-is-equality is correct in some
> sense, it is not very useful as the objects returned by successive "get
> annotated return type" calls on the same method object will not be equal.
>
> The proposed equals implementations check if the argument object
> implements the same AnnotatedType subinterface (and makes sure one of
> the subinterfaces is *not* implemented when determining equality for
> AnnotatedType implementation objects) and compares the results of
> corresponding methods. This is analogous to the equals implementations
> of the Type implementation classes found in the impl classes in the
> sun.reflect.generics.reflectiveObjects package.
>
> The order of annotations is considered significant for equality; this is
> arguably overly strict, but I didn't think it was worthwhile to form
> sets of annotations for the purposes of equality comparison in this case.
>
> For toString to replicate the source ordering for most kinds of types,
> the type annotations, if any, occur textually to the left of the text
> representing the type as in
>
>      @Nonnull String
>
> or more interestingly
>
>          @Nonnull List<String>
>
> the latter meaning "a nonnull list where the list contains strings."
>
> However, this pattern is not followed for arrays. The annotated type
>
>      @Nonnull String[]
>
> means "an array of strings where the strings are nonnull" rather than
> "an array that is nonnull where the array contains strings." The
> rationale for this is explained in JSR 308 related documents such as:
>
>      https://checkerframework.org/jsr308/jsr308-faq.html#syntax
> https://checkerframework.org/jsr308/specification/java-annotation-design.html
>
> A multi-dimensional array is represented as an AnnotatedArrayType whose
> component type is an array type of lower dimension, and so on, bottoming
> out with a non-array component type. The existing toString output for
> the Type objects of an array uses VM style notation like
> "[[Ljava.lang.String" for a 2D string array. This is even less helpful
> for annotated type objects and therefore toString output in the style
> usable in source code ("String [] []") will be used instead. Starting
> from an AnnotatedArrayType object for a 2D array of strings, using the
> integer value of type annotations the encounter order for the
> annotations and types is:
>
>      @TypeAnno(2) String @TypeAnno(0) [] @TypeAnno(1) []
>
> Therefore, for multi-dimensional arrays, the results for the nested
> dimensions are appended to the in-progress string builder while the
> results for the non-array component are pre-pended.
>
> Thanks,
>
> -Joe
>