JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

joe darcy
Hello,

Please review the small spec and implementation changes for

     8175335: Improve handling of module types in
javax.lang.model.util.Types
     http://cr.openjdk.java.net/~darcy/8175335.0/

which treats pseudo-types for modules similarly to the pseudo-types for
packages.

Thanks,

-Joe

Reply | Threaded
Open this post in threaded view
|

Re: JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

Jonathan Gibbons
OK.

Quibble, you don't need java.compiler in the jtreg @modules directive,
since that is implied by jdk.compiler.

-- Jon

On 02/21/2017 05:37 PM, joe darcy wrote:

> Hello,
>
> Please review the small spec and implementation changes for
>
>     8175335: Improve handling of module types in
> javax.lang.model.util.Types
>     http://cr.openjdk.java.net/~darcy/8175335.0/
>
> which treats pseudo-types for modules similarly to the pseudo-types
> for packages.
>
> Thanks,
>
> -Joe
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

Alex Buckley-3
In reply to this post by joe darcy
On 2/21/2017 5:37 PM, joe darcy wrote:
> Please review the small spec and implementation changes for
>
>      8175335: Improve handling of module types in
> javax.lang.model.util.Types
>      http://cr.openjdk.java.net/~darcy/8175335.0/
>
> which treats pseudo-types for modules similarly to the pseudo-types for
> packages.

Three specific comments:

- Types::erasure(TypeMirror) is one of the family. It should throw IAE
on a module type as well as a package type.

- Types::getNoType(TypeKind) could say "To obtain a pseudo-type for
packages or modules, call asType() on the result of
Elements.getPackageElement(CharSequence) or
Elements.getModuleElement(CharSequence)."

- TypeMirror's spec fails to mention module types in "pseudo-types
corresponding to packages and to the keyword void."

A general comment:

The methods in Types that throw IAE could be clarified. Where they now
say "if given a type for an executable, package, or module", they really
mean "if given a mirror that does not represent a suitable type for the
operation". The Types spec could say "Utility methods for operating on
types. Most methods operate on primitive types, reference types
(including array types and the null type), intersection types, and the
pseudo-type 'void'. Executable types and the pseudo-types for packages
and modules are generally out of scope for these methods."

Alex
Reply | Threaded
Open this post in threaded view
|

Re: JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

joe darcy
Hi Alex,

Thanks for the careful reading.

Revised webrev uploaded as

          http://cr.openjdk.java.net/~darcy/8175335.1/

Details comments below.


On 2/21/2017 6:35 PM, Alex Buckley wrote:

> On 2/21/2017 5:37 PM, joe darcy wrote:
>> Please review the small spec and implementation changes for
>>
>>      8175335: Improve handling of module types in
>> javax.lang.model.util.Types
>>      http://cr.openjdk.java.net/~darcy/8175335.0/
>>
>> which treats pseudo-types for modules similarly to the pseudo-types for
>> packages.
>
> Three specific comments:
>
> - Types::erasure(TypeMirror) is one of the family. It should throw IAE
> on a module type as well as a package type.

Diff old vs new changes:

152c152
<      * @throws IllegalArgumentException if given a package type
---
 >      * @throws IllegalArgumentException if given a type for a package
or module

(The implementation class was already updated to the new spec in the
original webrev.)

>
> - Types::getNoType(TypeKind) could say "To obtain a pseudo-type for
> packages or modules, call asType() on the result of
> Elements.getPackageElement(CharSequence) or
> Elements.getModuleElement(CharSequence)."

Diff old vs new changes:

209,211c209,216
<      * For packages, use
<      * {@link Elements#getPackageElement(CharSequence)}{@code .asType()}
<      * instead.
---
 >      *
 >      * <p>To get the pseudo-type corresponding to a package or module,
 >      * call {@code asType()} on the element modeling the {@linkplain
 >      * PackageElement package} or {@linkplain ModuleElement
 >      * module}. Names can be converted to elements for packages or
 >      * modules using {@link Elements#getPackageElement(CharSequence)}
 >      * or {@link Elements#getModuleElement(CharSequence)},
 >      * respectively.


>
> - TypeMirror's spec fails to mention module types in "pseudo-types
> corresponding to packages and to the keyword void."

Patched as follows:

---
a/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
Thu Feb 16 14:47:39 2017 -0800
+++
b/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
Tue Feb 21 23:47:00 2017 -0800
@@ -36,7 +36,7 @@
   * array types, type variables, and the null type.
   * Also represented are wildcard type arguments,
   * the signature and return types of executables,
- * and pseudo-types corresponding to packages and to the keyword {@code
void}.
+ * and pseudo-types corresponding to packages, modules, and to the
keyword {@code void}.
   *
   * <p> Types should be compared using the utility methods in {@link
   * Types}.  There is no guarantee that any particular type will always


>
> A general comment:
>
> The methods in Types that throw IAE could be clarified. Where they now
> say "if given a type for an executable, package, or module", they
> really mean "if given a mirror that does not represent a suitable type
> for the operation". The Types spec could say "Utility methods for
> operating on types. Most methods operate on primitive types, reference
> types (including array types and the null type), intersection types,
> and the pseudo-type 'void'. Executable types and the pseudo-types for
> packages and modules are generally out of scope for these methods."

The general comment is valid, but I'd prefer to keep this bug about
getting modules-as-types consistent with packages-as-types and leave the
broader clarification you suggest to a future release. I've filed    
JDK-8175386: "Clarify exception behavior of Types utility methods" to
track that work.

Cheers,

-Joe

Reply | Threaded
Open this post in threaded view
|

Re: JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types

Alex Buckley-3
All looks good.

(Trivial: TypeMirror spec says "and to the keyword {@code void}" -- the
"to" is unnecessary.)

Alex

On 2/22/2017 9:48 AM, joe darcy wrote:

> Hi Alex,
>
> Thanks for the careful reading.
>
> Revised webrev uploaded as
>
>           http://cr.openjdk.java.net/~darcy/8175335.1/
>
> Details comments below.
>
>
> On 2/21/2017 6:35 PM, Alex Buckley wrote:
>> On 2/21/2017 5:37 PM, joe darcy wrote:
>>> Please review the small spec and implementation changes for
>>>
>>>      8175335: Improve handling of module types in
>>> javax.lang.model.util.Types
>>>      http://cr.openjdk.java.net/~darcy/8175335.0/
>>>
>>> which treats pseudo-types for modules similarly to the pseudo-types for
>>> packages.
>>
>> Three specific comments:
>>
>> - Types::erasure(TypeMirror) is one of the family. It should throw IAE
>> on a module type as well as a package type.
>
> Diff old vs new changes:
>
> 152c152
> <      * @throws IllegalArgumentException if given a package type
> ---
>  >      * @throws IllegalArgumentException if given a type for a package
> or module
>
> (The implementation class was already updated to the new spec in the
> original webrev.)
>
>>
>> - Types::getNoType(TypeKind) could say "To obtain a pseudo-type for
>> packages or modules, call asType() on the result of
>> Elements.getPackageElement(CharSequence) or
>> Elements.getModuleElement(CharSequence)."
>
> Diff old vs new changes:
>
> 209,211c209,216
> <      * For packages, use
> <      * {@link Elements#getPackageElement(CharSequence)}{@code .asType()}
> <      * instead.
> ---
>  >      *
>  >      * <p>To get the pseudo-type corresponding to a package or module,
>  >      * call {@code asType()} on the element modeling the {@linkplain
>  >      * PackageElement package} or {@linkplain ModuleElement
>  >      * module}. Names can be converted to elements for packages or
>  >      * modules using {@link Elements#getPackageElement(CharSequence)}
>  >      * or {@link Elements#getModuleElement(CharSequence)},
>  >      * respectively.
>
>
>>
>> - TypeMirror's spec fails to mention module types in "pseudo-types
>> corresponding to packages and to the keyword void."
>
> Patched as follows:
>
> ---
> a/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
> Thu Feb 16 14:47:39 2017 -0800
> +++
> b/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
> Tue Feb 21 23:47:00 2017 -0800
> @@ -36,7 +36,7 @@
>    * array types, type variables, and the null type.
>    * Also represented are wildcard type arguments,
>    * the signature and return types of executables,
> - * and pseudo-types corresponding to packages and to the keyword {@code
> void}.
> + * and pseudo-types corresponding to packages, modules, and to the
> keyword {@code void}.
>    *
>    * <p> Types should be compared using the utility methods in {@link
>    * Types}.  There is no guarantee that any particular type will always
>
>
>>
>> A general comment:
>>
>> The methods in Types that throw IAE could be clarified. Where they now
>> say "if given a type for an executable, package, or module", they
>> really mean "if given a mirror that does not represent a suitable type
>> for the operation". The Types spec could say "Utility methods for
>> operating on types. Most methods operate on primitive types, reference
>> types (including array types and the null type), intersection types,
>> and the pseudo-type 'void'. Executable types and the pseudo-types for
>> packages and modules are generally out of scope for these methods."
>
> The general comment is valid, but I'd prefer to keep this bug about
> getting modules-as-types consistent with packages-as-types and leave the
> broader clarification you suggest to a future release. I've filed
> JDK-8175386: "Clarify exception behavior of Types utility methods" to
> track that work.
>
> Cheers,
>
> -Joe
>