ModuleInfo extends AnnotatedElement

[Roger Riggs]

Hi,

Sorry this has gotten long winded...


Mandy Chung wrote:
> 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.   
Providing both interfaces allows the caller to choose the access 
sequence that works best
for the caller.

> 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.
This could be addressed by only returning the Accessible Annotations.
Alternatively, it could return the Annotation objects but lazily 
evaluate the bindings of the values.
Then the exception would be thrown when the Annotation value was accessed.

>>
>>
>> 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.
So the API should say that it never tries to resolve values that are 
.classes (and probably enum's too)
and will always throw an exception. And it should be clear when the 
exception is thrown.

I understood that it would try to resolve the classes and only fail if 
the classes or enum's were not
available. 


Just to be clear, if there is an Annotation with multiple values then 
all of the primitive type and string
values will be accessible and the classes and enum's will not.  An the 
exception will be thrown
when the value is retrieved.  For example,

@Foo() {
     Enum Colors { RED, GREEN, BLUE};
     String first();
     int second();
     boolean third();
     Class fourth() default Object.class;
     Color fifth(); default RED;
}

And the snippet:

Foo a = ModuleInfo.getAnnotation(Foo.class)
  try {
         a.first();  // Succeeds
         a.second();  // Succeeds
         a.third();  // Succeeds
         a.fourth();  // Fails
         a.fifth();  // Would fail but doesn't get here
  } catch (UnsupportedElementTypeException e) {
         System.out.println(e.getClassNames());
         // if you really want to load the Class
         Class<?> c = Class.forName(e.getClassNames().get(0));
  }


>
>>>
>>>> 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