RFR: JDK-8213103: RoundEnvironment.getElementsAnnotatedWith(Class) crashes with -source 8

[joe darcy]

Hi Jan,


On 10/31/2018 3:50 AM, Jan Lahoda wrote:
> Hi,
>
> When RoundEnvironment.getElementsAnnotatedWith(Class) converts the 
> Class to TypeElement, it uses:
> eltUtils.getTypeElement(eltUtils.getModuleElement(<class-module-name>), 
> <class-name>)
>
> But this fails with -source 8, as there are no module (so 
> getModuleElement returns null, and getTypeElement checks the module 
> parameter is non-null). The proposed fix is to avoid this code.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8213103
> Webrev: http://cr.openjdk.java.net/~jlahoda/8213103/webrev.00/index.html
>
> Any feedback is welcome,
>

Thanks for looking at this issue.

Stepping back a bit, the API functionality being supported is

     RoundEnvironment.getElementsAnnotatedWith​(Class<? extends 
Annotation> a)

and related methods which take in annotation types indicated using Class 
objects (a runtime core reflection representation) and convert them 
internally to TypeElement's (a compile-time representation) by 
extracting the name of the annotation type and then doing various look-ups.

If the compile-time and runtime environments and configured to be 
consistent with each other, then a runtime annotation type should be 
able to get back a corresponding compile-time representation. However, 
it is possible for the environments to not be consistent, leading to 
this bug and the earlier change 8190886: "package-info handling in 
RoundEnvironment.getElementsAnnotatedWith" which this proposed fix amends.

The proposed "return null" arm in

@@ -285,13 +285,15 @@
          // differ from the single module being compiled.
          String name = annotation.getCanonicalName();
          TypeElement annotationElement = eltUtils.getTypeElement(name);
          if (annotationElement != null)
              return annotationElement;
-        else {
+        else if (!eltUtils.getAllModuleElements().isEmpty()) {
              String moduleName = 
Objects.requireNonNullElse(annotation.getModule().getName(), "");
              return 
eltUtils.getTypeElement(eltUtils.getModuleElement(moduleName), name);
+        } else {
+            return null;
          }
      }

      private Element mirrorAsElement(AnnotationMirror annotationMirror) {
          return annotationMirror.getAnnotationType().asElement();

is basically the handling for an inconsistent environment pre-modules. 
The preceding lines are an, in retrospect, incomplete handling of an 
inconsistent state with modules.

I recommend the check for "if modules are supported" be implemented 
using a Source-based predicate so that any future dead code will get 
removed once modules are in all supported source levels. In that route 
is not chosen, is getAllModuleElements() a good-performing predicate for 
this purpose?  Would something like "if (getModuleElement​("java.base") 
!= null )" be better?

If the annotation type indicated by the Class object *cannot* be turned 
into an compile-time annotation type, arguably that is an erroneous 
situation that should cause an exception. The IllegalArgumentException 
clauses of

RoundEnvironment.getElementsAnnotatedWith​(Class<? extends Annotation> a)
RoundEnvironment.getElementsAnnotatedWithAny​(Set<Class<? extends 
Annotation>> annotations)

could be expanded to cover this situation and, in the implementation, 
return null replaced by throwing an informative exception.

What do you think?

Cheers,

-Joe