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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Jan 23 23:23:38 UTC 2020 

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

Maurizio

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

More information about the compiler-dev mailing list