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

[Liam Miller-Cushon]

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

Thanks for the review!

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

Moving it into ClassReader is fine with me. I initially thought it might end up requiring more logic than it did. I left it in a separate class because it's still a medium amount of logic and is fairly self-contained, but if you're preference is to move it into ClassReader I'm happy to proceed with that.

Just for my understanding, if it was a separate class would the preferred style be to register it in `Context` and have `ClassReader` create an instance of it?

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

PR Comment: https://git.openjdk.org/jdk/pull/16407#issuecomment-1786024913