RFR 8191802: Upward projection result is A<? extends Number> instead of A<? super Integer>

[Maurizio Cimadamore]

On 29/11/17 02:22, Vicente Romero wrote:
>
>
> On 11/28/2017 06:14 PM, Vicente Romero wrote:
>> Hi,
>>
>> This one is a tough one to review, so I think that we will probably 
>> need a couple of iterations. As mentioned earlier today in an offline 
>> chat, some comments at test UpperBounds seems to don't match with the 
>> type expected in the annotation.
>>
>> It seems to me that the code could be simplified, more self 
>> documented, etc, if TypeProjection::visitTypeVar was not handling 
>> type variables and captured type variables at the same time, it 
>> doesn't feel natural either.
>
> never mind about the two below, I was checking the code again and I 
> think that the case I was thinking about can't happen. I missed that 
> visitor TypeProjection is invoked from TypeArgumentProjection
Exactly - was about to say that :-)

Maurizio
>
>
>>
>> At TypeArgumentProjection::visitWildcardType shouldn't we consider 
>> returning the same type if the wildcard doesn't contain restricted 
>> variables? In other words: can we guarantee that every wildcard 
>> visited will contain restricted variables? I'm thinking about nested 
>> cases.
>>
>> kind of similar comment for TypeArgumentProjection::visitType, can we 
>> guarantee that every time we get to this method, we are dealing with 
>> a type that contains restricted variables? again I'm considering 
>> nested cases. We have to be more careful in this case as this could 
>> imply issuing an error for a case for which a projection exists.
>>
>> Thanks,
>> Vicente
>
> Vicente
>
>>
>> On 11/27/2017 10:09 AM, Maurizio Cimadamore wrote:
>>>
>>> This is an updated version of the webrev:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/8191802-v2/
>>>
>>> I realized that there were further discrepancies w.r.t. the spec 
>>> text, so I handled them all at once:
>>>
>>> * the spec has different treatment for when a type argument is a 
>>> wildcard and for when it's a regular type - the implementation was 
>>> treating both wildcards and regular types in the same way, which led 
>>> to inconsistencies (see JDK-8191893). I now defined separate 
>>> visitors for the outer projection and type argument projection 
>>> parts, so that the code should be easier to follow.
>>>
>>> * not all fresh types created during a projection overrode the 
>>> 'needStripping' method, which could lead to issue with type 
>>> annotation processing
>>>
>>> * the fact that TypeProjection extended from StructuralMapping had a 
>>> subtle issue - on the one hand, StructuralMapping has the required 
>>> logic to handle arrays (e.g. map component type and return new array 
>>> if needed) - on the other hand, with projections we need to be 
>>> careful - if the element type has no projection, then projection of 
>>> the array is also undefined. The impl was returning an array of 
>>> <null> which was then causing crashes inside the compiler. This is 
>>> now called out explicitly.
>>>
>>> Maurizio
>>>
>>>
>>> On 24/11/17 17:39, Maurizio Cimadamore wrote:
>>>>
>>>> Hi,
>>>> please review the fix for JDK-8191802:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/8191802/
>>>>
>>>> This is a conformance issue with local variable type inference - 
>>>> the specification text for upper projection says this (section 4.10.5):
>>>>
>>>> "The upward projection of a type T with respect to a set of 
>>>> restricted type variables is defined as follows:
>>>>
>>>>     [...]
>>>>
>>>>     If T is a parameterized class type or a parameterized interface 
>>>> type, G<A1, ..., An>, then the result is G<A1', ..., An'>, where, 
>>>> for 1 ≤ i ≤ n, Ai' is derived from Ai as follows:
>>>>
>>>>         [...]
>>>>         If Ai is a type that mentions a restricted type variable, 
>>>> then Ai' is a wildcard. Let U be the upward projection of Ai. There 
>>>> are three cases:
>>>> *If U is not Object, and if either the declared bound of the ith 
>>>> parameter of G, Bi, mentions a type parameter of G, or Bi is not a 
>>>> subtype of U, then Ai' is an upper-bounded wildcard, ? extends U.*
>>>>             Otherwise, if the downward projection of Ai is L, then 
>>>> Ai' is a lower-bounded wildcard, ? super L.
>>>>             Otherwise, the downward projection of Ai is undefined 
>>>> and Ai' is an unbounded wildcard, ?."
>>>>
>>>> The text in bold is not implemented accurately by javac. What javac 
>>>> used to do was simply to throw away the upper bound and favor the 
>>>> lower bound if the upper was Object.
>>>>
>>>> The spec text is much more subtle and precise, allowing javac to 
>>>> throw away upper bounds that do not add any extra information 
>>>> w.r.t. declared bounds.
>>>>
>>>> As a result of this change, there are few places where the compiler 
>>>> used to infer A<? extend B> (where B was same type as declared 
>>>> bound) and now it infers A<?> as per spec - this caused few changes 
>>>> in the jshell test TypeNameTest.
>>>>
>>>> I've added a lvti harness test to verify the assertions in the 
>>>> above paragraph.
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20171129/0de1c39c/attachment-0001.html>