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

[Vicente Romero]

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 

I can add that code to be extra cautious but given the way annotations 
are copied, the annotations in the record component will always have 
that field to null. And from there the annotation is copied to the 
accessor and to the parameter in the canonical constructor. We can do it 
but I think that it is not necessary

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

Vicente

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