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

Robert Field robert.field at oracle.com
Thu Dec 10 03:07:58 UTC 2015 

So, with CapturedType#getUpperBound you get CharSequence,
But, with WildcardType#getExtendsBound you get Object.

Right?

It seems cleaner to do this on visitWildcard but if this is something other than a fluke, getting a better answer wins and I’d think wins over adding code to collapse compound types.

Important note: the result of this call is used as the type in a generated variable declaration.  So, building a representation of compound type does not work.  

From the initial description (summarized):

The user code —

-> class Test1<T> {T[] get() {return null;}} 
-> Test1<? extends String> test1 = new Test1<>() 
-> test1.get() 

Generates (in the midst of a lot of other stuff) the variable declaration:

   public static ? extends String[] $1; 

Which, of course, should be:

   public static String[] $1; 

Similarly, the new example needs to generate:

   public static CharSequence $2;

Not:

   public static Object&CharSequence $2;

or:

   public static Object $2;

otherwise the generated code will break.

-Robert

> On Dec 9, 2015, at 5:33 PM, ShinyaYoshida <bitterfoxc at gmail.com> wrote:
> 
> 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 <mailto: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> <mailto: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 <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 <mailto: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 <mailto: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 < <mailto:bitterfoxc at gmail.com>bitterfoxc at gmail.com <mailto: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 <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