RFR 8144903: JShell: determine incorrectly the type of the expression which is array type of captured type

ShinyaYoshida bitterfoxc at gmail.com
Thu Dec 10 01:33:35 UTC 2015 

Hi Maurizio and Robert,
Thanks a lot.

Yes, currently we make intersection types Object as you mentioned.
And I also think it is difficult to determine "right" type.

In my previous mail, I meant that:
Using CapturedType#getUpperBound, the test was passing before webrev.02.
Should we use wildcard instead of upper bound, allowing breaking a
regression test?

Now my question is:
Is breaking a regression test and changing the behavior acceptable, to
refactor the use of CapturedType#getUpperBound to the use of wildcard and
extends bound?

If it is no problem, I'll accept the change.

Regards,
shinyafox(Shinya Yoshida)

2015-12-10 9:37 GMT+09:00 Maurizio Cimadamore <
maurizio.cimadamore at oracle.com>:

>
>
> On 09/12/15 22:46, Robert Field wrote:
>
> Thanks Maurizio,
>
> There is very little Type processing code in jshell, we attempt to
> leverage higher level javac functionality as much as possible. TypePrinter
> is pretty much the extent of it. So there is not "some logic to 'normalize'
> intersection types away".
>
> Not entirely true :-)
>
> You do have logic to strip away intersection types; there is even some
> commented code in there:
>
> if (sym.name.length() == 0 && (sym.flags() & COMPOUND) != 0) {
>     /***    StringBuilder s = new StringBuilder(visit(t.supertype_field, locale));    for (List<Type> is = t.interfaces_field; is.nonEmpty(); is = is.tail) {        s.append('&');        s.append(visit(is.head, locale));    }    return s.toString();    ***/    return "Object";
>
>
> I tried some examples and I can confirm jshell always uses Object when an
> intersection type is found, which is suboptimal, and prevents solutions
> like the one I suggested from being implemented.
>
> Stepping back, given current logic 'erases' all intersection types to
> Object, I don't think it's too bad if last Shinya example infers Object
> instead of CharSequence; in other words, it all depends on where you draw
> the line: if you want to remain relatively type-system agnostic, using
> Object and the wildcard type is the way to go. If that route is too
> imprecise, then TypePrinter needs to do more heavy lifting in figuring out
> what the right type is - but you're probably up for some non-trivial stuff
> (example: if a type is Serializable & Runnable - which type is the 'right'
> one for the declaration??)
>
> Maurizio
>
>
> BTW: this code is currently used only to get the type of an expression.
>
> -Robert
>
> On December 9, 2015 2:12:48 PM Maurizio Cimadamore
> <maurizio.cimadamore at oracle.com> <maurizio.cimadamore at oracle.com> wrote:
>
>> What we can do to improve this is to use something similar to the
>> analysis used in JLS 9.9 for functional types:
>>
>> * if the bound of the corresponding parameter (the captured var bound
>> field) contains any other formal parameter (hard case), just use the
>> 'wildcard' type
>>
>> Example:
>>
>> class Foo<X extends Comparable<X>> {
>> X get();
>> }
>>
>> Foo<? extends String> fs = ...
>> fs.get() --> String
>>
>> * if the bound B of the corresponding parameter does not contain any
>> other formal (non fbound case) then:
>>
>>    - if wildcard is '? extends T', the inferred type is glb(B, T) (**)
>>    - if wildcard is '? super T', the inferred type is T
>>    - if wildcard is '?', the inferred type is B
>>
>> This should take care of most of the nasty cases, in a way that's
>> compatible with how things are handled for lambdas.
>>
>> In your CharsSequence case, since the bound is CharSequence and the
>> wildcard type is Object, you are in the happy case (#2), so the inferred
>> type is glb(CharSequence, Object) = Object & CharSequence.
>>
>> (**) as this example demonstrates, sometimes glb can return intersection
>> types, so I'm assuming you already have some logic to 'normalize'
>> intersection types away - and of course this logic should not throw info
>> away - i.e. if you normalize Object & Serializable, I would expect the
>> logic to retain Serializable, and discard the non-interesting bound.
>>
>> Opinions?
>>
>> Maurizio
>>
>>
>> On 09/12/15 20:46, ShinyaYoshida wrote:
>>
>> Hi Maurizio and Jan,
>> Thank you for your advice.
>> I've tried to unroll it in wildcard to take the advice.
>> http://cr.openjdk.java.net/~shinyafox/kulla/8144903/webrev.02/
>>
>> But I've met a problem.
>> In this case, we get the Object instead of CharSequence, this is not
>> acceptable for user:
>>
>> In TypeNameTest#testBounds
>>         assertEval("class Test1<X extends CharSequence> { public X get()
>> { return null; } }");
>>         Snippet x = varKey(assertEval("Test1<?> test1 = new Test1<>();"));
>>         VarSnippet sn4 = (VarSnippet) varKey(assertEval("test1.get()"));
>>         assertEquals(sn4.typeName(), "CharSequence");
>>
>> Compiler determines the upper bound of wildcard"?" as CharSequence from
>> the declaration.
>> But WildcardType doesn't have that information at all.
>>
>> Do you have any idea for this?
>>
>> Regards,
>> shinyafox(Shinya Yoshida)
>>
>>
>> 2015-12-10 0:00 GMT+09:00 Jan Lahoda <jan.lahoda at oracle.com>:
>>
>>> I am looking at:
>>>
>>> http://hg.netbeans.org/jet-main/file/default/java.source.base/src/org/netbeans/api/java/source/SourceUtils.java#l1067
>>>
>>> (which is solving a similar problem; and while I found out there may be
>>> a bug related to arrays in that.)
>>>
>>> I concur with Maurizio that CapturedType.wildcard should (presumably) be
>>> used always for CapturedTypes. When the resulting wildcard is not suitable
>>> for the given place (i.e. top-level type), then we should use
>>> getExtendsBound()/getSuperBound() to unroll the wildcard.
>>>
>>> Out of the recent two patches, I like webrev.01_0 much more. Presumably
>>> just override visitWildcardType and unroll the wildcard if needed, and
>>> change the visitCapturedType to just:
>>> return visit(t.wildcard, locale);
>>>
>>> Would that work?
>>>
>>> Jan
>>>
>>>
>>> On 9.12.2015 13:39, Maurizio Cimadamore wrote:
>>>
>>>> Hi Shinya,
>>>> In my experience, the best way to get to an usable declared type from a
>>>> capture type is to always follow the wildcard - i.e. the upper bound of
>>>> a captured type is more of an internal type system info (the result of
>>>> capture conversion) and I don't think it should be used.
>>>>
>>>> That said, I think there's a problem here: captured vars can appear in
>>>> two places, type position or type-argument position:
>>>>
>>>> 1) Type position
>>>>
>>>> class Foo<X> {
>>>>     X get() { .. }
>>>> }
>>>>
>>>> Foo<? extends String> fs = ...
>>>> fs.get(); // you get a #CAP in a type position - i.e. the return type of
>>>> get()
>>>>
>>>> 2) Type-argument position
>>>>
>>>> class Foo2<X> {
>>>>     List<X> get() { .. }
>>>> }
>>>>
>>>> Foo<? extends String> fs = ...
>>>> fs.get(); // you get a List<#CAP>, so #CAP is in a type-argument
>>>> position
>>>>
>>>> I think the TypePrinter should have some logic to detect as to whethe
>>>> you are in case #1 or #2, and if in #1 it should drop wildcards to the
>>>> floor; if it's in #2 it is ok to retain wildcards. So, I think a good
>>>> solution would emit String for #1 and List<? extends String> for #2.
>>>>
>>>> As usual - this is my 0.02$ :-)
>>>>
>>>> [This is essentially an inference problem - we are trying to infer a
>>>> denotable type from the type of an expression which might contain
>>>> non-denotable types - the problem is not too different from what we have
>>>> to do when the functional interfaces that are the target of a lambda
>>>> conversion contains wildcards].
>>>>
>>>> Maurizio
>>>>
>>>> On 09/12/15 00:34, Robert Field wrote:
>>>>
>>>>> Exactly, Shinya. Still curious of Jan's thinking.
>>>>>
>>>>> -Robert
>>>>>
>>>>>
>>>>>
>>>>> On December 8, 2015 15:13:42 ShinyaYoshida <bitterfoxc at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Hi Robert,
>>>>>> Thank you for your review and comments.
>>>>>>
>>>>>> 2015-12-09 5:28 GMT+09:00 Robert Field <robert.field at oracle.com>:
>>>>>>
>>>>>> [Jan, I'd like your opinion]
>>>>>>>
>>>>>>> Thank you for seeing this and drilling into where the problem is.
>>>>>>> This is
>>>>>>> important to fix.
>>>>>>>
>>>>>>> I see that this would address the problem.  However, I am squeamish
>>>>>>> about
>>>>>>> the solution as it unwraps to match the result of layers of
>>>>>>> underlying
>>>>>>> super class behavior.  Looking at it further, I believe the problem
>>>>>>> is in
>>>>>>> the pre-existing typeToPrint, my bad!  I don't think the approach I
>>>>>>> took is
>>>>>>> right.  Stepping back, we want upper bound unless the type is
>>>>>>> embedded in a
>>>>>>> type that takes generic types, class types.  That suggests to me
>>>>>>> replacing
>>>>>>> typeToPrint with a boolean useWildCard = false, which would be
>>>>>>> changed by
>>>>>>> visitClassType().  But alas this is recursive and this visitor does
>>>>>>> not
>>>>>>> have state that passes through the recursion, so it would be global
>>>>>>> -- in
>>>>>>> this case that should work, but ugly.
>>>>>>>
>>>>>>> Certainly, my fix is too depend on the super implementation.
>>>>>> The fix which you mentioned is like this, right?
>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8144903/webrev.01_0/
>>>>>>
>>>>>>
>>>>>>
>>>>>> Ah! Really the problem is that we want the top-level type to be
>>>>>>> erased.
>>>>>>> But that means holding an instance of Types.  Deeper integration with
>>>>>>> javac.  Hmmmm....
>>>>>>>
>>>>>>> Another possible fix is making TypePrinter general(no recursive
>>>>>> state) and
>>>>>> adding the print method what is erase top-level capture to
>>>>>> TypePrinter,
>>>>>> using the print method at callsite:
>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8144903/webrev.01_1/
>>>>>>
>>>>>> Regards,
>>>>>> shinyafox(Shinya Yoshida)
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> -Robert
>>>>>>>
>>>>>>> On Dec 8, 2015, at 1:10 AM, ShinyaYoshida < <bitterfoxc at gmail.com>
>>>>>>> bitterfoxc at gmail.com> wrote:
>>>>>>>
>>>>>>> Hi Robert,
>>>>>>> Could you review my patch for
>>>>>>> "JShell: determine incorrectly the type of the expression which is
>>>>>>> array
>>>>>>> type of captured type"?
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8144903
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8144903/webrev.00/
>>>>>>>
>>>>>>> Regards,
>>>>>>> shinyafox(Shinya Yoshida)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>
>>
>

More information about the kulla-dev mailing list