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