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

[Jan Lahoda]

Hi Joe,

Thanks for the comments. An updated webrev that uses Feature.MODULES 
instead of Elements.getAllModules, and also updates the copyright year 
as noted by Vicente, is here:
http://cr.openjdk.java.net/~jlahoda/8213103/webrev.01/

(As a side note, Elements.getAllModules() should be OK performance-wise 
for this purpose, it basically reads an existing/pre-filed field.)

For changing the method to throw IAE, while I agree this situation may 
point out an unintended difference between the compile- and run-time 
environments, sometimes the difference may be intentional (runtime 
dependencies need to be transitive, while compile time don't need to be) 
and may be benign, as is in the case where I've noticed this problem. So 
given the method didn't throw the exception in such cases before, I'd 
rather keep the behavior compatible.

Thanks,
     Jan

On 31.10.2018 18:27, joe darcy wrote:
> 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
>