JDK 13 RFR of JDK-8224687: Add clarifying overrides of Element.asType to more specific subinterfaces

[Joe Darcy]

Hi Jon,

On 5/30/2019 10:23 AM, Jonathan Gibbons wrote:
>
>
> On 05/30/2019 10:15 AM, Jonathan Gibbons wrote:
>>
>>
>> On 05/29/2019 05:24 PM, Joe Darcy wrote:
>>> Hello,
>>>
>>> Please review the webrev and CSR for:
>>>
>>>     JDK-8224687: Add clarifying overrides of Element.asType to more 
>>> specific subinterfaces
>>>     webrev: http://cr.openjdk.java.net/~darcy/8224687.2/
>>>     CSR: https://bugs.openjdk.java.net/browse/JDK-8225027
>>>
>>> Thanks,
>>>
>>> -Joe
>>>
>>
>> Joe,
>>
>> CSR Reviewed.
>>
>> In the new test, I don't understand lines 119, 128, regarding 
>> ElementKind.FIELD.
>> Either the code is a copy-paste error, or the description elsewhere 
>> of DeclaredType
>> and TypeKind.DECLARED is incorrect, since these only refer to 
>> declared types
>> (class, interface, etc) and not fields.
>>
>> Surprising or maybe not, the test actually passes, suggesting that 
>> this is the
>> current behavior even if it is unexpected.  That being said, the 
>> output from the
>> test would be easier to understand if it included the ElementKind and 
>> simple name
>> of the element, as well as the typeMirror, typeMirror class and 
>> typeKind.
>>
>> -- Jon
>
> I guess what is happening is that the mapping to TypeMirror for 
> ElementKind.FIELD
> varies, depending on the field being declared. If so, that deserves 
> mention in the
> spec for VariableElement.asType, and also deserves a comment in the 
> test code.
> I'll stop short of suggesting that the test code should have more 
> fields in it, such as
> a field of primitive type, but if you could find an easy way to adapt 
> the test for that,
> that would be good too.
>

Correct. I propose adding some additional documentation around the maps 
used by the test:

>     /*
>      * For both of the maps below, a ElementKind value is mapped to
>      * one value related to an element's asType image. In some cases,
>      * the ElementKind -> (TypeMirror type, TypeKind) mapping is
>      * always the same, such as ElementKind.PACKAGE mapping to
>      * (NoType.class, TypeKind.PACKAGE). In other cases, such as for a
>      * field, there are many possible mappings and they are not
>      * attempted to be examined exhaustively by this test.
>      */
>     private static final Map<ElementKind, Class<?>> 
> elementKindToTypeClass =
>         Map.of(ElementKind.CLASS, DeclaredType.class,
>                ElementKind.CONSTRUCTOR, ExecutableType.class,
>                ElementKind.METHOD, ExecutableType.class,
>                ElementKind.PACKAGE, NoType.class,
>                ElementKind.MODULE, NoType.class,
>                ElementKind.TYPE_PARAMETER, TypeVariable.class,
>                // For the field NestedClass.name that is tested, a
>                // declared type is used.
>                ElementKind.FIELD, DeclaredType.class);
>
>     private static final Map<ElementKind, TypeKind> 
> elementKindToTypeKind =
>         Map.of(ElementKind.CLASS, TypeKind.DECLARED,
>                ElementKind.CONSTRUCTOR, TypeKind.EXECUTABLE,
>                ElementKind.METHOD, TypeKind.EXECUTABLE,
>                ElementKind.PACKAGE, TypeKind.PACKAGE,
>                ElementKind.MODULE, TypeKind.MODULE,
>                ElementKind.TYPE_PARAMETER, TypeKind.TYPEVAR,
>                // For the field NestedClass.name that is tested, a
>                // declared type is used.
>                ElementKind.FIELD, TypeKind.DECLARED);
>
For the cases where only one flavor of mapping is possible, such as 
packages and modules, the revised map explicitly mentions the mapping. 
I'll work on crafting an additional sentence along the lines of "Note 
that a variable can have many possible kinds of types."

Thanks,

-Joe