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

[Vicente Romero]

nit: at TypeArgumentProjection::visitType the code could be more explicit:

Type upper = t.map(TypeProjection.this, pkind);
        Type lower = t.map(TypeProjection.this, pkind.complement());

                                ||
                               \  /
                                \/

        Type upper = t.map(TypeProjection.this, ProjectionKind.UPWARDS);
        Type lower = t.map(TypeProjection.this, ProjectionKind.DOWNWARDS);


side: the spec says that the downward projection is a partial function 
so it could be assumed that the upward projection is a total function, 
but then at Check::checkLocalVarType an error message is issued if the 
compiler couldn't find the upward projection for a given type. If this 
is possible then the spec should be updated saying that the upward 
projection is also a partial function, if the upward projection is a 
total function and the compiler fail to find it because the initializer 
expression is incorrect, there is a self reference to the variable, 
etc., then IMO the error message should make a reference to the 
incorrect initializer expression as the root cause of the problem. The 
fact that the upward projection couldn't be determined is just a 
consequence of that and there shouldn't be an additional error message 
to state that.

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/8a20ea9d/attachment.html>