8202469 / 8202473: Correct type annotation resolution for class type variables

Sat Jun 6 09:44:34 UTC 2020

Hi Rafael,

Let's try to get this into 15,

http://cr.openjdk.java.net/~winterhalter/8202473/webrev.00/test/jdk/java/lang/annotation/typeAnnotations/TypeVariableBoundParameterIndex.java.udiff.html
looks strange did you unintentionally remove the test for 8202469? The
issues are orthogonal so I'd like to keep the cases you developed for the
previous fix.

I think the fix can lead to further refactorings since this was the last
use of buildAnnotatedType where parameter 4 and 5 wasn't identical. This
can be followed up later.

cheers
/Joel

On Tue, Apr 7, 2020 at 8:52 PM Rafael Winterhalter <rafael.wth at gmail.com>
wrote:

> I created this webrev to also fix the second part that was previously
> fixed as part of 8202469:
> http://cr.openjdk.java.net/~winterhalter/8202473/webrev.00/ - this would
> be the second part of the adjustment.
>
> This is reported in https://bugs.openjdk.java.net/browse/JDK-8202473 and
> was closed as a duplicate. With this cleaner solution, the duplication does
> however no longer apply.
>
> Am Mo., 6. Apr. 2020 um 14:01 Uhr schrieb Joel Borggrén-Franck <
> joel.borggren.franck at gmail.com>:
>
>> Many thanks to Vicente for sponsoring this. I'll start to look at the
>> second part shortly.
>>
>> cheers
>> /Joel
>>
>> On Mon, Mar 16, 2020 at 9:44 PM Joel Borggrén-Franck <
>> joel.borggren.franck at gmail.com> wrote:
>>
>>> Looks good to me!
>>>
>>> I'll see if I can find a sponsor for this.
>>>
>>> cheers
>>> /Joel
>>>
>>> On Sat, Mar 7, 2020 at 2:15 AM Rafael Winterhalter <rafael.wth at gmail.com>
>>> wrote:
>>>
>>>> I finally found the time to look at this again, sorry for the delay.
>>>>
>>>> Thank you for your comments. I had the chance to better look into the
>>>> problem and simplify the code a bit more and also reduced the scope of the
>>>> fix to a single problem. I also added test cases that should be exhaustive
>>>> for all possible scenarios and adjusted the code comment.
>>>>
>>>> I uploaded the adjusted patch as a webrev:
>>>> http://cr.openjdk.java.net/~winterhalter/8202469/webrev.01/
>>>>
>>>> Thanks, Rafael
>>>>
>>>> Am So., 16. Feb. 2020 um 10:51 Uhr schrieb Joel Borggrén-Franck <
>>>> joel.borggren.franck at gmail.com>:
>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> Thanks for reaching out and reminding me of this!
>>>>>
>>>>> I managed to find some time to look into 8202469 during the weekend.
>>>>> By swapping place of  the TVars in the test I managed to isolate this
>>>>> without depending on 8202473, take a look at my hacky copy-paste update to
>>>>> your test at http://cr.openjdk.java.net/~jfranck/8202469/ .
>>>>>
>>>>> The comment on lines 269-276 needs to be updated. Perhaps include a
>>>>> table with the start index depending of the type? Also referencing JVMLS
>>>>> 4.4 for the structure of the bound might be instructive for future readers.
>>>>>
>>>>> My current understanding is that there are two problems with the code
>>>>> on (the original) lines 277-287:
>>>>> 1) Type Variables should have index 0 while getting index 1 due to
>>>>> lines 279-280.
>>>>> 2) Bounds that are instances of ParameterizedType always gets index 1
>>>>> but if the first bound represents a Class type the index should be 0.
>>>>>
>>>>> Does this make sense?
>>>>>
>>>>> Can you make the if-switches positive? I find my original code with
>>>>> the negative tests hard to read and the update doesn't help.
>>>>>
>>>>> Also can you expand the test to cover the different kinds of bounds
>>>>> mentioned in 4.4 and also test Type Variables on methods, I suspect javac
>>>>> treats method (and constructor) tvars similarly but there have been bugs ...
>>>>>
>>>>> TL;DR please update the webrev with:
>>>>>
>>>>> - Split out test and fix for 8202469
>>>>> - More test coverage of different kinds of bounds
>>>>> - Test for method tvars
>>>>> - See if you can rework the logic to have (mostly) positive tests in
>>>>> if switch
>>>>>
>>>>> Thanks again for looking into this, I'll start looking into 8202473, I
>>>>> think the fix for that one opens up a bigger rework of the code which is
>>>>> why I want to separate the two.
>>>>>
>>>>> cheers
>>>>> /Joel
>>>>>
>>>>> On Sun, Aug 4, 2019 at 10:12 PM Rafael Winterhalter <
>>>>> rafael.wth at gmail.com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> appologies for the delay, I only managed to get my patched up to
>>>>>> webrev today (http://cr.openjdk.java.net/~winterhalter/). It's three
>>>>>> patches in total now, for the third one I could not add a test case, it
>>>>>> would require to compile an annotation property against an enumeration type
>>>>>> and load it against another class where the enumeration was turned into an
>>>>>> annotation. I struggled a bit with jtreg to make it work but I cannot see
>>>>>> myself complete this in a meaningful amount of time. However, I hope that
>>>>>> the patch and the error it solves are straightforward to see.
>>>>>>
>>>>>> I only submitted a patch for 8202469. 8202473 is solved by it. It's
>>>>>> two bugs but they are a symptom of the same oversight.
>>>>>>
>>>>>> I hope this helps you to review it but let me know if you have any
>>>>>> questions or if I should further adjust my patch.
>>>>>>
>>>>>> Best regards, Rafael
>>>>>>
>>>>>> Am Fr., 2. Aug. 2019 um 12:18 Uhr schrieb Rafael Winterhalter <
>>>>>> rafael.wth at gmail.com>:
>>>>>>
>>>>>>> Thanks Daniel,
>>>>>>> I did some work on Glassfish a bunch of years ago, I had no idea.
>>>>>>> I will try to split up the changes (I fixed another bug in
>>>>>>> annotation processing yesterday) and upload everything there.
>>>>>>> Best regards, Rafael
>>>>>>>
>>>>>>> Am Fr., 2. Aug. 2019 um 11:59 Uhr schrieb Daniel Fuchs <
>>>>>>> daniel.fuchs at oracle.com>:
>>>>>>>
>>>>>>>> Hi Rafael,
>>>>>>>>
>>>>>>>> On 02/08/2019 09:00, Joel Borggrén-Franck wrote:
>>>>>>>> > I can host webrevs for you on cr to make it easier for other
>>>>>>>> reviewers, if
>>>>>>>> > you also send me the patches or webrefs off-list to get around the
>>>>>>>> > attachment stripping I can upload them to cr.
>>>>>>>>
>>>>>>>> I see that you are a JDK project author, so you already own a page
>>>>>>>> on cr (http://cr.openjdk.java.net/~winterhalter/) where you can
>>>>>>>> upload webrevs (e.g. using `scp` as in:
>>>>>>>> $ scp -r webrev-NNNNN.01 winterhalter at cr.openjdk.java.net: )
>>>>>>>>
>>>>>>>> best regards and welcome!
>>>>>>>>
>>>>>>>> -- daniel
>>>>>>>>
>>>>>>>