Please review the change to access the annotations from module-info

[Joe Darcy]

Hello.

Mandy Chung wrote:
> Joe,
>
> Thanks for the feedback to the ModuleInfo API.  This change also 
> implements the reflection methods to access the annotations in the 
> java.lang.reflect.Module class (i.e. implementation of 
> AnnotatedElement methods).
>
> The webrev is at:
>   http://cr.openjdk.java.net/~mchung/jigsaw/annotations/webrev/
>
> Most of the work is done in the java.lang.module.ModuleInfoAnnotation 
> class that does the following:
> - parses a raw annotation from the module-info.class
> - stores the element value pairs
> - creates an Annotation object using the core reflection
> - it supports for both ModuleInfo and Module API to access 
> annotations.  The Annotation objects created for the 
> ModuleInfo.getAnnotation() method doesn't support elements of Class type.
>
> I added a constructor of CoreReflectionFactory to take a Module 
> parameter so that its module class loader will be used to load classes 
> when instantiating an Annotation object.   Is there any issue with 
> that?  The factory will only be used to parse class signatures.


Some comments from a first pass over the code:

In ModuleInfo.java, I recommend adding an "@see 
java.lang.reflect.AnnotatedElement#isAnnotationPresent" to the 
isAnnotationPresent method.

In ModuleInfoReader.java, if you want to start using some new language 
features, the following line is an ideal candidate for the diamond operator:

70     private Map<String, ModuleInfoAnnotation> annotationTypes = new 
LinkedHashMap<String, ModuleInfoAnnotation>();

With diamond

70     private Map<String, ModuleInfoAnnotation> annotationTypes = new 
LinkedHashMap<>();

Later in that file, this looks like a candidate for strings in switch:

 135             if (name.equals(MODULE))
 136                 readModule();
 137             else if (name.equals(MODULE_PROVIDES))
 138                 readModuleProvides();
 139             else if (name.equals(MODULE_REQUIRES))
 140                 readModuleRequires();
 141             else if (name.equals(MODULE_PERMITS))
 142                 readModulePermits();
 143             else if (name.equals(MODULE_CLASS))
 144                 readModuleClass();
 145             else if (name.equals(RUNTIME_VISABLE_ANNOTATION)) {
 146                 readAnnotations();
 147             } else {
 148                 in.skip(length);

switch(name) {
    case MODULE:
        readModule(); break;

    case MODULE_PROVIDES:
    ...
}

In src/share/classes/java/lang/module/ModuleSystem.java, this method body

  43     // Module names must be legal Java identifiers
  44     //
  45     public static final String checkModuleName(String nm) {
  46         if (nm == null)
  47             throw new IllegalArgumentException();
  48         int n = nm.length();
  49         if (n == 0 || !Character.isJavaIdentifierStart(nm.charAt(0)))
  50             throw new IllegalArgumentException();
  51         for (int i = 1; i < n; i++) {
  52             char c = nm.charAt(i);
  53             if (!Character.isJavaIdentifierPart(c) && (c != '.'))
  54                 throw new IllegalArgumentException(nm
  55                                                    + ": Illegal 
module-name"
  56                                                    + " character"
  57                                                    + " at index " + i);
  58         }
  59         return nm;
  60     }

could be replaced by a call to SourceVersion.isIdentifier:
http://download.java.net/jdk7/docs/api/javax/lang/model/SourceVersion.html#isIdentifier(java.lang.CharSequence)

An advantage of the isIdentifier method is that it does the right thing 
in regard to code points; see the code in
http://hg.openjdk.java.net/jdk7/jdk7/langtools/file/71c2c23a7c35/src/share/classes/javax/lang/model/SourceVersion.java
for the codepoint-correct loop if you can't call this method.

/src/share/classes/java/lang/reflect/Module.java

Is there a reason the annotation handling code here looks different from 
the analogous code in java.lang.reflect.Constructor?

Should Module extend AccessbileObject?

src/share/classes/java/lang/module/ModuleInfoAnnotation.java

  53  * Represents an annotation in module-info.class. This class is
  54  * based on the com.sun.tools.classfile library.
  55  *
  56  * @see com.sun.tools.classfile.Annotation
 
I don't think the official javadoc of a java.lang.* class should 
reference a com.sun.* class.

As to how this class is structured, I'm not familiar with the origin of 
provenance of com.sun.tools.classfile.Annotation.  However, perhaps the 
time has come to have a unified "arms length annotation maker" to 
subsume the functionality in 
langtools/src/share/classes/com/sun/tools/apt/mirror/declaration/AnnotationProxyMaker.java 
and a similar class in javac.

src/share/classes/java/lang/module/UnsupportedElementTypeException.java

  44  */
  45 public class UnsupportedElementTypeException extends RuntimeException {
  46 
  47     private static final long serialVersionUID = 269;
  48 
  49     private transient List<String> classnames; // cannot be serialized

I assume you used javax.lang.model.type.MirroredTypeException[s] for 
inspiration here :-)  A difference is that the classname 
UnsupportedElementTypeException should be able to be serialized since it 
is just a list of strings; the MirroredTypeExceptions cannot be 
serialized because the Type objects are tied up in a lot of internal 
javac state.  (And if UnsupportedElementTypeException is made fully 
serializable, it should define a good serial form for the classname.)

As a general comment, I would encourage shell regression tests to be 
rewritten as java tests.

-Joe