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

Thu Jan 23 21:29:07 UTC 2020

Looks good.

Minor tweak - seems like if RecordComponent constructor took a VarSymbol 
you should be able to set all fields (including original annotations) in 
a single shot and make the original annotation field immutable?

Maurizio

On 23/01/2020 20:58, Vicente Romero wrote:
> Hi Maurizio,
>
> Thanks for your comments, I have uploaded another iteration at [1]
>
> Vicente
>
> [1] http://cr.openjdk.java.net/~vromero/8236210/webrev.02/
>
> On 1/23/20 1:30 PM, Maurizio Cimadamore wrote:
>> Some comments:
>>
>> * we can't create a new HashMap per symbol - at the very least the 
>> map should be lazy or we will most surely add a lot of footprint
>>
>> * names such as getRCOriginalAnnos seems odd
>>
>> * also... if the goal of the map is to keep track of the original 
>> annotations applied to a component... why not storing the original 
>> annotations directly in RecordComponent (and get rid of the map) ?
>>
>> * Check - I suggest retaining "annotationApplicable" as a predicate 
>> which calls "getApplicableTargets" and test if result isEmpty() - 
>> this way you won't need to tweak other clients
>>
>> * I don't like the recovery target much - better to have 
>> getApplicableTargets return an Optional<Set<Name>> - so that the code 
>> has to decide how to handle recovery
>>
>> Maurizio
>>
>>
>> On 23/01/2020 17:57, Vicente Romero wrote:
>>> 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/
>>>>>>>
>>>>>
>>>
>