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

[Vicente Romero]

Hi Maurizio,

Thanks for the review, some comments below

On 1/17/20 6:27 AM, Maurizio Cimadamore wrote:
> Tricky indeed. Few suggestions:
>
> * It seems like SymbolMetadata::remove has morphed more into a 
> removeDeclarationMetadata (since you no longer remove type annos from 
> there)

true

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

right that could work, will give it a try

>
> * 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?

we have already tests for that but they live in another neighborhood, see:

test/langtools/tools/javac/processing/model/element/CheckingTypeAnnotationsOnRecords.java
test/langtools/tools/javac/processing/model/element/AnnoProcessorOnRecordsTest.java

we can eventually move them to RecordCompilationTests in a separate effort

>
> Cheers
> Maurizio

Vicente
>
> 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/
>>