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

Thu Jan 23 18:30:57 UTC 2020

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