RFR: JDK15-8236210: javac generates wrong annotation for fields generated from record components

[Maurizio Cimadamore]

Tricky indeed. Few suggestions:

* It seems like SymbolMetadata::remove has morphed more into a 
removeDeclarationMetadata (since you no longer remove type annos from there)

* Another way to simplify the code and avoid the 'ignoreTypeUse' trick, 
would be for "annotationApplicable" to give you the list of all the 
applicable targets, rather than just a boolean. Then, from there, I 
think you can implement the rest of your logic w/o the need of 
rechecking for applicability multiple times.

* The test seems good, but it only checks for one side of the story: 
classfile generation; should we also test that the annotations are 
indeed available to processors?

Cheers
Maurizio

On 16/01/2020 21:24, Vicente Romero wrote:
> Please review the fix for [1] at [2]. Some background:
>
> Annotations applied to record components are propagated to the 
> corresponding record member so if an annotation has target: FIELD, it 
> is propagated to the corresponding FIELD, if it has target METHOD, it 
> is propagated to the accessor and so on. But at the moment when method 
> members are generated there is no enough information to propagate only 
> the right annotations. So all the annotations are propagated to all 
> the possible locations.
>
> So there is a point when we need to remove all the annotations that 
> are not in place before going on with the annotation party. On top of 
> the above there is the issue that there is no AST representing record 
> components, just symbols so the corresponding field has been holding 
> all the annotations and it's metadata has been modified as if it was 
> both a field and a record component.
>
> So there are two places where we need to trim annotations from: the 
> metadata of the symbol and / or the modifiers in the AST. Whatever is 
> in the metadata will be written to the class file, whatever is in the 
> modifiers could be see by annotation processors.
>
> The metadata contains both type annotations and declaration 
> annotations. Type annotations are all in the right place as they are 
> applicable to all the types of the corresponding record members 
> generated by the compiler. But we could need to remove declaration 
> annotations. So for declaration annotations if they are not applicable 
> to the given record member it will be needed to remove them. For the 
> AST modifiers if the annotation is not applicable either as type 
> annotation and or declaration annotation, only in the last case it 
> will be removed.
>
> So it could be that an annotation is removed as a declaration 
> annotation but it is kept in the AST modifier for further inspection 
> by annotation processors.
>
> For example:
>
>     import java.lang.annotation.*;
>
>     @Target({ElementType.TYPE_USE, ElementType.RECORD_COMPONENT})
>     @Retention(RetentionPolicy.RUNTIME)
>     @interface Anno { }
>
>     record R(@Anno String s) {}
>
> just before Enter ends we will have for the case of the generated field:
>   - @Anno in the modifier
>   - @Anno as a type annotation
>   - @Anno as a declaration annotation
>
> the last one should be removed because the annotation has not FIELD as 
> target but it was applied as a declaration annotation because the 
> field was being treated both as a field and as a record component. 
> Once we reach the point when the corresponding annotations have been 
> copied to the record component, the field doesn't need to hold 
> annotations that are not intended for it anymore. Still @Anno has to 
> be kept in the AST's modifiers as it is applicable as a type 
> annotation to the type of the field.
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8236210
> [2] http://cr.openjdk.java.net/~vromero/8236210/webrev.00/
>