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

[Vicente Romero]

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.

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

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/20171128/b3014f34/attachment.html>