RFR: 8225377: type annotations are not visible to javac plugins across compilation boundaries [v2]

[Vicente Romero]

On Sat, 28 Oct 2023 20:39:02 GMT, Liam Miller-Cushon <cushon at openjdk.org> wrote:

>> Hello,
>> 
>> Please consider this fix for [JDK-8225377: type annotations are not visible to javac plugins across compilation boundaries](https://bugs.openjdk.org/browse/JDK-8225377).
>> 
>> ---
>> 
>> To provide some background context and motivation for the bug fix, consider an example like:
>> 
>> 
>> class B {
>>   @Nullable int f(int x) {
>>     return x;
>>   }
>> }
>> 
>> 
>> If `@Nullable` is a `@Target(METHOD)` annotation, an annotation processor can retrieve the annotation by from the  `ExecutableElement` for `f` by calling `getAnnotationMirrors()`. This is true regardless of whether `B` is being compiled from source in the current compilation, or loaded from a class file.
>> 
>> If `@Nullable` is a `@Target(TYPE_USE)` annotation, an annotation processor should be able to retrieve the annotation by locating the `ExecutableElement` for `f`, and calling `getReturnType().getAnnotationMirrors()`. This works today if `B` is compiled from source, but (due to [JDK-8225377](https://bugs.openjdk.org/browse/JDK-8225377)) not if `B` is loaded from a class file.
>> 
>> This is a hurdle to migrating from existing declaration annotations to `@Target(TYPE_USE)` annotations for use-cases like `@Nullable`. For example: a dependency injection framework might use an annotation processor to generate code, and that code would want to know if a formal parameter type accepted `null` values, or if a method returned `null` values. That works today with declaration annotation definitions of `@Nullable`, and this fix will allow it work with `TYPE_USE` definitions of `@Nullable` in the future.
>> 
>> ---
>> 
>> javac already reads type annotation information from `Runtime{Visible,Invisible}TypeAnnotations` attributes in class files, and `ClassReader` registers a completer that calls `Symbol#setTypeAttributes` to add the type annotations to the symbol's metadata. This change adds logic to associate those annotations with the corresponding type contained in the symbol.
>> 
>> Some notes on the approach:
>> 
>> * There are some similarities between the logic being added and existing logic in `TypeAnnotations`, but `TypeAnnotations` operates on the AST, and is solving different problems. In the AST annotations are already adjacent to the type they are annotating, and in bytecode they have been separated out and the implementation has to interpret the annotation target and type path information to match the annotations back up with their types.
>> 
>> * For initial test coverage, I have enabled test covera...
>
> Liam Miller-Cushon has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updates
>   
>   * handle annotating the same type twice (with runtime-visible and
>     -invisible annotations)
>   * don't throw an assertion on ill-formed type paths
>   * null-check ClassSymbol typarams

the code in TypeAnnotationMapper looks good to me, actually simpler than expected. I just wonder if it should be placed inside of ClassReader, if you want to keep it separated please consider not exposing its API as static methods.

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java line 2264:

> 2262:                 List<Attribute.TypeCompound> newList = deproxyTypeCompoundList(proxies);
> 2263:                 sym.setTypeAttributes(newList.prependList(sym.getRawTypeAttributes()));
> 2264:                 TypeAnnotationMapper.addTypeAnnotationsToSymbol(sym, newList);

I wonder if the new TypeAnnotationsMapper should actually live inside of ClassReader

-------------

PR Review: https://git.openjdk.org/jdk/pull/16407#pullrequestreview-1704837433
PR Review Comment: https://git.openjdk.org/jdk/pull/16407#discussion_r1376657664