RFR: JDK-8239447: compiler error for annotations applied to record components with target METHOD

[Maurizio Cimadamore]

Looks good

Maurizio

On 13/03/2020 12:45, Vicente Romero wrote:
> I have modified the patch to include the previous code too, please 
> check [1]
>
> Thanks,
> Vicente
>
> [1] http://cr.openjdk.java.net/~vromero/8239447/webrev.02/
>
> On 3/13/20 6:39 AM, Maurizio Cimadamore wrote:
>> I agree that this is a better solution, thanks for investigating.
>>
>> In general sharing trees is best avoided as it can lead (as in this 
>> case) to very odd, and hard to diagnose failures.
>>
>> It is possible that the patch you had yesterday is also needed - you 
>> might want to double check with Jan about that; perhaps a different 
>> test case might reveal some issues with the lack of cleanup in the 
>> annotation processing round.
>>
>> Maurizio
>>
>> On 13/03/2020 06:02, Vicente Romero wrote:
>>> Hi
>>>
>>> On 3/12/20 8:13 PM, Maurizio Cimadamore wrote:
>>>> Looks good - I wonder: why wasn't this failing if TYPE_USE was used 
>>>> as a target? I know that processing for type annotations is quite 
>>>> different, but I'm still a bit surprised to see that the data from 
>>>> previous rounds doesn't cause issues in that case.
>>>
>>> I also thought that the difference was due to the different nature 
>>> of type annotations. But while double checking to answer you I 
>>> discovered that the cause is a bug in the records implementation, 
>>> let's assume we have:
>>>
>>> record R(@MyAnno int i) {}
>>>
>>> currently what is happening is that the same AST created by the 
>>> parser for the annotation in the record header is shared by the 
>>> generated field, the record component, the accessor and the 
>>> parameter in the generated constructor. Exactly the same AST. This 
>>> is the root cause behind the reason why in some cases the 
>>> information from a previous round was being cleared and sometimes 
>>> not. I think that a cleaner solution should be to create different 
>>> ASTs per each record member. So this way each member including 
>>> record components will have their own AST. This fixes this bug and 
>>> probably others that were potentially lurking in this area. I have 
>>> uploaded a new iteration to [1].
>>>
>>>>
>>>> Maurizio
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] http://cr.openjdk.java.net/~vromero/8239447/webrev.01/
>>>>
>>>> On 12/03/2020 17:57, Vicente Romero wrote:
>>>>> ping
>>>>>
>>>>> On 3/7/20 12:34 AM, Vicente Romero wrote:
>>>>>> Please review fix for [1] at [2]. When there are APs involved, 
>>>>>> every time a new round of annotation processing will start, a 
>>>>>> visitor, see [3], sets the attribute field of annotation ASTs to 
>>>>>> null. This visitor was not visiting some annotations stored at 
>>>>>> the record component. This provoked that data from previous 
>>>>>> rounds could be accessible to later rounds provoking later 
>>>>>> annotation validation code to fail. Just visiting the annotations 
>>>>>> at the record component as it is done for the rest of the 
>>>>>> annotations has fixed the issue.
>>>>>>
>>>>>> Thanks,
>>>>>> Vicente
>>>>>>
>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8239447
>>>>>> [2] http://cr.openjdk.java.net/~vromero/8239447/webrev.00/
>>>>>> [3] 
>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/7af6364e1792/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l1591
>>>>>
>>>
>