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

[Joel Borggrén-Franck]

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