RFR: JDK-8215482: check for cycles in type variables can provoke NPE

[Vicente Romero]

thanks!
Vicente


On 1/14/19 5:56 AM, Maurizio Cimadamore wrote:
>
> Looks good!
>
> Maurizio
>
> On 11/01/2019 22:11, Vicente Romero wrote:
>>
>>
>> On 1/11/19 2:13 PM, Maurizio Cimadamore wrote:
>>>
>>> As per my suggestion to split into multiple stages, it was mostly a 
>>> suggestion to take full advantage of the tiered architecture of 
>>> TypeEnter. You are basically caching some types in a 'todo later' 
>>> list, and then you are coming back at them - avoiding these kind of 
>>> queuing is what the (relatively) new TypeEnter code is for.
>>>
>>> If the cyclic check is moved to a later phase, then I think you just 
>>> need to fetch the type variables from the symbol/type/tree and check 
>>> them; the types will be already set, no need to stash them into a 
>>> map. At least in theory :-)
>>>
>>
>> right thanks for the suggestion, that's a better option, please see:
>>
>> http://cr.openjdk.java.net/~vromero/8215482/webrev.01/
>>
>> Vicente
>>>
>>> Maurizio
>>>
>>> On 11/01/2019 18:05, Vicente Romero wrote:
>>>> I have realized that cloning the type variable is not necessary as 
>>>> the Pair<JCTypeParameter, TypeVar> element in the table will keep a 
>>>> reference to the type variable
>>>>
>>>> Vicente
>>>>
>>>>
>>>> On 1/11/19 12:29 PM, Vicente Romero wrote:
>>>>>
>>>>>
>>>>> On 1/11/19 12:21 PM, Maurizio Cimadamore wrote:
>>>>>>
>>>>>> It seems that another thing that your path is doing is storing 
>>>>>> all the type vars to check keyed by outermost class, so that only 
>>>>>> when you have finished entering the outermost symbol you actually 
>>>>>> start checking all pending typevars for cycles. I guess this 
>>>>>> delay is necessary otherwise you would hit a problem anyway when 
>>>>>> checking Bc for cyclicity?
>>>>>>
>>>>>
>>>>> correct
>>>>>
>>>>>> Have you consider moving the attribution and cyclicity check in 
>>>>>> different type enter phases? For instance, leave attribution in 
>>>>>> HeaderPhase, but move the cycle check in MembersPhase ?
>>>>>>
>>>>>
>>>>> I didn't try, but why would this be preferable? also as annotation 
>>>>> processing can nuke the type we would have to make sure that the 
>>>>> types are there before doing the cycle check which is something we 
>>>>> can guarantee now by keeping the check for cycles in the Header phase
>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>
>>>>> Vicente
>>>>>
>>>>>> On 11/01/2019 16:47, Vicente Romero wrote:
>>>>>>> Please review the fix for [1] at [2]. The NPE showed up in code 
>>>>>>> like:
>>>>>>>
>>>>>>> class Outer<A extends Outer.Inner, B> {
>>>>>>>     class Inner<Bc extends B> {}
>>>>>>> }
>>>>>>>
>>>>>>> here attribution of type variable `A` during type enter phase 
>>>>>>> will trigger attribution of class `Inner` while type variable 
>>>>>>> `B` hasn't been attributed yet and thus its bound is still set 
>>>>>>> to null. A similar problem arose a while ago see [3]. The issue 
>>>>>>> provoking the current bug is that checks for cycles in type 
>>>>>>> variables are done as soon as the type variable is attributed 
>>>>>>> but in cases like the one above we can't do that until the type 
>>>>>>> variable for the outer class has been attributed too.
>>>>>>>
>>>>>>> My first try was to create a fixup table that maps the outer 
>>>>>>> type to the list of type variables defined by it or its members 
>>>>>>> that happen to be types too, and once the compiler finish 
>>>>>>> entering the outer class it would be safe to check for cycles in 
>>>>>>> all the concerning type variables. I had a mild success here as 
>>>>>>> there were some trivial cases that were working before that 
>>>>>>> started failing. I realized that it was because the annotation 
>>>>>>> processing phase was setting all the types to null, no bueno. So 
>>>>>>> I decided to clone the type variables to be stored in the fixup 
>>>>>>> table and do the cycle check on the clones which is what the 
>>>>>>> current patch is doing.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vicente
>>>>>>>
>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8215482
>>>>>>> [2] http://cr.openjdk.java.net/~vromero/8215482/
>>>>>>> [3] https://bugs.openjdk.java.net/browse/JDK-6660289
>>>>>
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190114/3dda8fbe/attachment.html>