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

[Vicente Romero]

On 1/23/20 4:29 PM, Maurizio Cimadamore wrote:
> 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?
right I can do that before pushing, only that it won't be only a 
VarSymbol as it doesn't have a link to the annotations which are in the AST
>
> Maurizio

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