JDK 13 RFR of JDK-8224687: Add clarifying overrides of Element.asType to more specific subinterfaces
Jonathan Gibbons
jonathan.gibbons at oracle.com
Thu May 30 23:03:47 UTC 2019
There's a `type type` stutter in ExecutableElement.
Otherwise, Looks Good To Me, and good to go
-- Jon
On 5/30/19 2:22 PM, Joe Darcy wrote:
> Updated webrev:
>
> webrev: http://cr.openjdk.java.net/~darcy/8224687.3/
>
> More descriptive comments in the test, added more specific @see tags
> to the overridden methods (in part of suppress unwanted propagation of
> the all the @see entries from Element.asType), and gave guidance on
> the range of types for a variable.
>
> Diff of patches below, trimmed out some white space differences.
>
> Thanks,
>
> -Joe
>
>
> 27,29c29,40
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/ExecutableElement.java
> 2019-05-29 17:14:43.047376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/ExecutableElement.java
> 2019-05-29 17:14:42.667376000 -0700
> < @@ -41,6 +41,15 @@
> ---
> > TypeMirror asType();
> >
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/ExecutableElement.java
> 2019-05-30 14:15:59.625000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/ExecutableElement.java
> 2019-05-30 14:15:59.261000999 -0700
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2013, Oracle and/or its affiliates. All
> rights reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> rights reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or
> modify it
> > @@ -41,6 +41,17 @@
> 36a48,49
> > + *
> > + * @see ExecutableType
> 45,46c58,59
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/ModuleElement.java
> 2019-05-29 17:14:43.735376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/ModuleElement.java
> 2019-05-29 17:14:43.375376000 -0700
> ---
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/ModuleElement.java
> 2019-05-30 14:16:00.341000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/ModuleElement.java
> 2019-05-30 14:15:59.957000999 -0700
> 55c68
> < @@ -37,6 +38,12 @@
> ---
> > @@ -37,6 +38,16 @@
> 60c73,74
> < + * Returns a {@linkplain javax.lang.model.type.NoType
> pseudo-type} for this module.
> ---
> > + * Returns a {@linkplain javax.lang.model.type.NoType pseudo-type}
> > + * for this module.
> 61a76,78
> > + *
> > + * @see javax.lang.model.type.NoType
> > + * @see javax.lang.model.type.TypeKind#MODULE
> 68,69c85,93
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/PackageElement.java
> 2019-05-29 17:14:44.439376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/PackageElement.java
> 2019-05-29 17:14:44.071376000 -0700
> ---
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/PackageElement.java
> 2019-05-30 14:16:01.049000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/PackageElement.java
> 2019-05-30 14:16:00.657000999 -0700
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2017, Oracle and/or its affiliates. All
> rights reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> rights reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or
> modify it
> 78c102
> < @@ -38,6 +39,12 @@
> ---
> > @@ -38,6 +39,16 @@
> 83c107,108
> < + * Returns a {@linkplain javax.lang.model.type.NoType
> pseudo-type} for this package.
> ---
> > + * Returns a {@linkplain javax.lang.model.type.NoType pseudo-type}
> > + * for this package.
> 84a110,112
> > + *
> > + * @see javax.lang.model.type.NoType
> > + * @see javax.lang.model.type.TypeKind#PACKAGE
> 91,93c119,128
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/TypeElement.java
> 2019-05-29 17:14:45.139376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/TypeElement.java
> 2019-05-29 17:14:44.751376000 -0700
> < @@ -60,6 +60,25 @@
> ---
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/TypeElement.java
> 2019-05-30 14:16:01.745000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/TypeElement.java
> 2019-05-30 14:16:01.373000999 -0700
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2017, Oracle and/or its affiliates. All
> rights reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> rights reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or
> modify it
> > @@ -60,6 +60,28 @@
> 110a146,148
> > + *
> > + * @see Types#asMemberOf(DeclaredType, Element)
> > + * @see Types#getDeclaredType(TypeElement, TypeMirror...)
> 119,120c157,165
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/TypeParameterElement.java
> 2019-05-29 17:14:45.811376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/TypeParameterElement.java
> 2019-05-29 17:14:45.455376000 -0700
> ---
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/TypeParameterElement.java
> 2019-05-30 14:16:02.509000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/TypeParameterElement.java
> 2019-05-30 14:16:02.109000999 -0700
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2011, Oracle and/or its affiliates. All
> rights reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> rights reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or
> modify it
> 137,139c182,191
> < ---
> old/src/java.compiler/share/classes/javax/lang/model/element/VariableElement.java
> 2019-05-29 17:14:46.475376000 -0700
> < +++
> new/src/java.compiler/share/classes/javax/lang/model/element/VariableElement.java
> 2019-05-29 17:14:46.119376000 -0700
> < @@ -26,6 +26,7 @@
> ---
> > ---
> old/src/java.compiler/share/classes/javax/lang/model/element/VariableElement.java
> 2019-05-30 14:16:03.233000999 -0700
> > +++
> new/src/java.compiler/share/classes/javax/lang/model/element/VariableElement.java
> 2019-05-30 14:16:02.857000999 -0700
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2005, 2013, Oracle and/or its affiliates. All
> rights reserved.
> > + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> rights reserved.
> > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> > *
> > * This code is free software; you can redistribute it and/or
> modify it
> > @@ -26,6 +26,8 @@
> 143a196
> > +import javax.lang.model.type.TypeKind;
> 147c200
> < @@ -38,6 +39,12 @@
> ---
> > @@ -38,6 +40,19 @@
> 152a206,210
> > + *
> > + * Note that the types of variables range over {@linkplain
> > + * TypeKind many kinds} of types, including primitive types,
> > + * declared types, and array types, among others.
> > + *
> 153a212,213
> > + *
> > + * @see TypeKind
> 161,162c221,222
> < +++
> new/test/langtools/tools/javac/processing/model/element/TestElementAsType.java
> 2019-05-29 17:14:46.795376000 -0700
> < @@ -0,0 +1,139 @@
> ---
> > +++
> new/test/langtools/tools/javac/processing/model/element/TestElementAsType.java
> 2019-05-30 14:16:03.561000999 -0700
> > @@ -0,0 +1,152 @@
> 273a334,342
> > + /*
> > + * 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.
> > + */
> 280a350,351
> > + // For the field NestedClass.name that is tested, a
> > + // declared type is used.
> 289a361,362
> > + // For the field NestedClass.name that is tested, a
> > + // declared type is used.
>
>
> On 5/30/2019 12:04 PM, Joe Darcy wrote:
>> 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
>>
>>
More information about the compiler-dev
mailing list