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

[Rafael Winterhalter]

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