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

[Maurizio Cimadamore]

On 29/11/17 03:25, Vicente Romero wrote:
> 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);

I think you are right - since that code doesn't make sense for pkind  = 
downwards (in which case we prematurely exit with the null type), the 
remainder of this method can assume that pkind = UPWARDS and 
pkind.complement() = 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.
upper projection is a total function - downward is not. The check you 
see in checkLocalVarType is not to check that the outcome of upward 
projection is defined (it always should be) - but is to check as to 
whether the type of the initializer expression is the null type, in 
which case it should be an error. I agree that as things are now is a 
bit messy because the code does the check AFTER the projection, but this 
is relying on the fact that the projection doesn't alter those types. 
Anyway, if it makes things clearer, I can move the check for VOID and 
NULL to _before_ the projection. Would that help?

Maurizio
>
> 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/20171129/8181e334/attachment.html>