RFR: JDK15-8236210: javac generates wrong annotation for fields generated from record components
Vicente Romero
vicente.romero at oracle.com
Thu Jan 23 20:58:25 UTC 2020
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