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

[Vicente Romero]

Hi Maurizio,

after a deep research and having changed some of the basics of record's 
implementation, here is another iteration [1]. I have added a map to 
preserve the original annotations attached to record components in case 
we need to go back to the originals. This is necessary when there are 
APs in the middle as the new test made clear.

Thanks for your comments and to Jan for offline suggestions,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8236210/webrev.01/

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