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

[Joel Borggrén-Franck]

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