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

[Maurizio Cimadamore]

On 17/01/2020 12:53, Vicente Romero wrote:
> 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

Ok - but do they check all the combinations that you gave recently added 
to the other test? Do they need to be made more powerful?

Maurizio

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