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

[Vicente Romero]

On 1/23/20 6:23 PM, Maurizio Cimadamore wrote:
>
> On 23/01/2020 21:39, Vicente Romero wrote:
>>
>>
>> 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
>
> Sorry - I saw 'var' and I thought symbol - but you are right - it's 
> the JCVariableDecl tree that you need to initialize those guys

right
>
> Maurizio

Vicente

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