RFR 8144903: JShell: determine incorrectly the type of the expression which is array type of captured type
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Thu Dec 10 00:37:12 UTC 2015
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> 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/
>>> <http://cr.openjdk.java.net/%7Eshinyafox/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
>>> <mailto: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/
>>> <http://cr.openjdk.java.net/%7Eshinyafox/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/
>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8144903/webrev.01_1/>
>>>
>>> Regards,
>>> shinyafox(Shinya Yoshida)
>>>
>>>
>>>
>>>
>>> -Robert
>>>
>>> On Dec 8, 2015, at 1:10 AM, ShinyaYoshida
>>> <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/
>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8144903/webrev.00/>
>>>
>>> Regards,
>>> shinyafox(Shinya Yoshida)
>>>
>>>
>>>
>>>
>>>
>>
More information about the kulla-dev
mailing list