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.da...@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 >
