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

[Vicente Romero]

On 1/17/20 7:54 AM, Maurizio Cimadamore wrote:
>
> 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?

I will double check

>
> Maurizio

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