ModuleInfo extends AnnotatedElement

[Mandy Chung]

Roger Riggs wrote:
> Hi Mandy,
>
>
> Mandy Chung wrote:
>> Hi Roger,
>>
>> Thanks for the comments:
>>
>> Roger Riggs wrote:
>>> Hi,
>>>
>>> Some comments on the module APIs:
>>>
>>>    * On ModuleInfo, I would like to see an enumeration of the 
>>> Annotations;
>>>      like getAnnotations() on Module.  This will allow the caller to
>>>      iterate over
>>>      the available annotations without having to make a call to probe
>>>      for each one.
>>
>> The caller knows which annotations to look for.  You should use the 
>> isAnnotationPresent() method to check.  What's the use case to 
>> enumerate all Annotations?  What happens if a type doesn't exist?  
>> The caller will need to catch the exception if an annotation type is 
>> not found.  In that case, what would the caller do?
> The difference is in the coding style, the current API suggests/requires:
>
>        // This pattern always causes 5 calls and perhaps 5 linear 
> searches for the annotation
>     ModuleInfo m = xxx;
>
>        Annotation1 a1 = m.getAnnotation(Annotation1.class);
>     if (a1 != null) {...}
>        Annotation2 a2 = m.getAnnotation(Annotation2.class);
>     if (a1 != null) {...}
>        Annotation1 a3 = m.getAnnotation(Annotation3.class);
>     if (a1 != null) {...}
>        Annotation1 a4 = m.getAnnotation(Annotation4.class);
>     if (a1 != null) {...}
>        Annotation1 a5 = m.getAnnotation(Annotation5.class);
>     if (a1 != null) {...}
>
> vs.
>     // This pattern touches each annotation that is present only once.
>     ModuleInfo m = xxx;
>     for (Annotation a : m.getAnnotations) {
>         if (a instanceof Annotation1) {
>            Annotation1 a1 = (Annotation1)a;
>         }
>         if (a instanceof Annotation2) {
>            Annotation2 a2 = (Annotation2)a;
>         }
>         if (a instanceof Annotation3) {
>            Annotation3 a3 = (Annotation3)a;
>         }
>         if (a instanceof Annotation4) {
>            Annotation4 a4 = (Annotation4)a;
>         }
>         if (a instanceof Annotation5) {
>            Annotation5 a5 = (Annotation1)a;
>         }
>     }
>

This forces all annotations to be instantiated even if you are only 
interested in certain one.  The above loads all classes referenced by 
all annotations whereas the first example above lazily loads classes and 
creates objects.   I would assume you prefer to reference the 
annotations that you're interested in instead of all for space and time.

If one class is not found and an exception is thrown, you won't be able 
to access any of them. 

>
>
> Furthermore, if you want to write a tool the prints out the 
> annotations that are present
> it can be done conveniently.
>

I considered that.  A tool should use javax.lang.model and the 
annotation processing API to print out annotations.  There may be cases 
that you have to use ModuleInfo API rather than the tools API.  We can 
revisit this when we identify other use cases that require 
ModuleInfo.getAnnotations().   Once I push the changeset to jigsaw repo, 
this will enable you to access module-info with annotations and we can 
begin building examples and identify issues and new requirements.   How 
does that sound?
 
> Pretty much every other API that maintains a set of something provides 
> an iterator;
> ModuleInfo should too.
>

Hmm...
>>
>>>
>>>    * In the case of annotations whose values are classes (see the
>>>      getAnnotation method),
>>>      and the UnsupportedElementTypeException is thrown, under what
>>>      circumstances
>>>      will the getClassNames() method return more than one Class 
>>> name?      
>>
>> Class[]
> Ok, so it will just list the classes that are not available.

Ah.... I think you might misinterpret the spec (based on your comment 
below).  getClassNames() returns the value of the element.  For example,

@Foo(
    classes = {java.util.Map.class, java.util.Set.class}
)
------------

   Foo a = ModuleInfo.getAnnotation(Foo.class)
   try {
          a.classes()
   } catch (UnsupportedElementTypeException e) {
          System.out.println(e.getClassNames());
          // if you really want to load the Class
          Class<?> c = Class.forName(e.getClassNames().get(0));
   }

The above will print [java.util.Map, java.util.Set].  The implementation 
doesn't attempt to create the Class objects when a.classes() is called 
but instead throwing a UnsupportedElementTypeException which you can use 
the getClassNames() method to find out what classes are listed in the 
value.  You can then call Class.forName() so that you can specify which 
class loader to use.

>>
>>> A List<String> seems a bit heavyweight for this purpose.
>>
>> Why would you think it's heavyweight?  What performance concern do 
>> you have?
> Sorry, I've been working on targets where space and time always matter.
> If you don't create garbage, you don't have to clean it up.

Thanks for mentioning this.  I should have mentioned the recommendation 
in defining an annotation interface for module-info.  If an annotation 
is designed to be accessed before the module is installed, it should 
avoid defining elements of Class or Class[] type.

In that case, UnsupportedElementTypeException would not be thrown.

>>
>>>    * In the AnnotatedElement interface the TypeNotPresentException 
>>> is specified
>>>      to be thrown when the class is not accessible.  Should it be used
>>>      in ModuleInfo.getAnnotation
>>>      instead of the new UnsupportedElementTypeException?  BTW, this
>>>      name is not quite correct since
>>>      the type is just not found in the caller's type system.
>>
>> Suggestion on a better name is welcome.
> ClassNotFoundException; NoClassDefFoundException...
>
> TypeNotPresentException
>
> These are all failures because Class.forName fails because it cannot 
> find the class.
>
>>
>> As specified in the UnsupportedElementTypeException spec:
>> UnsupportedElementTypeException is thrown when an application 
>> attempts to read a |Class| object or a Class[]-valued element by 
>> invoking the relevant method on an annotation returned by the 
>> |ModuleInfo.getAnnotation(java.lang.Class| 
>> <http://cr.openjdk.java.net/%7Emchung/jigsaw/annotations/api/java/lang/module/ModuleInfo.html#getAnnotation%28java.lang.Class%29>) 
>> method.
>> The class could be found but access to elements of type Class object 
>> and Class[] is just not supported since the information necessary to 
>> locate and load a class is not available when the module is not 
>> installed. TypeNotPresentException is not appropriate for this case. 
> I understand there is no attempt to locate or define the class *in* 
> the module being inspected
> because the module is not fully defined and no classloader can be 
> created for it.
>
> But the class name(s) found in the annotations *are* being resolved using
> the caller's classloader (Class.forName() or equivalent).  If the 
> annotation class is not found
> then that is the failure that is being reported, and that is very 
> similar to ClassNotFoundException
> or TypeNotPresentException.
>

See above.
Mandy