RFR: JDK-8191842: JShell: Inferred type information is lost when assigning types to a "var"

[Robert Field]

Review (partial) --


Wrap:

71: The new parameter makes slightly worse a horribly out-of-date method 
comment (my bad), which could be
       deleted, but better actually documented -- there are a lot of 
parameters and they would benefit from an
       explanation certainly including the new one "enhanced".

82: Built as a list is a good choice.

91: Nit: The comment is helpful but much longer than anything around it 
and would be more readable formatted.
       Also, the example type "int" copied from the comment below 
doesn't make any sense in this context, and so
       is confusing, maybe they should both be "Foo".

Eval:

106: Action at a distance.  Document, or better, use another mechanism.

213: The parameter order and cast are inconsistent with all the other 
process* calls in this switch.

341-343/347: Inconsistent

380-529: Indenting and spacing off

389-393...: I don't get why TreeScanner is run in ExpressionToTypeInfo 
and then again in Eval with deep interlinking
                assumptions (like the order of the list).  The only data 
added in the Eval scan is the 'NewClassTree node' which
                could be in AnonymousDescription.  Then you could just 
iterate the AnonymousDescriptions.

410: I'd make this something less likely to be a captured variable 
name.  That is a unique prefix.

418: ditto

399-445: Anonymous classes are infrequent enough to make this unlikely 
an issue, so can ignore this comment.
                 The components of a wrap have overhead:
                 they are stored independently for the duration and they 
are sequentially searched.  Within this
                 section these are all strings, so you could use 
StringBuilder, but the complexity of the extra variables would
                 make that messy.  You could just use string '+' rather 
then 2-5 'add' calls inline -- there is no value in
                 separating adjacent strings (only for Wraps).  And the 
result would probably be more readable.

436: BUG, you are double incrementing 'i'.  Please include a test like:

     class D { D(int foo, String bar) { this.foo = foo; this.bar = bar; 
} int foo; String bar; }
     new D(34, "hi") { String z = foo + bar; }
     $3.z

-Robert


On 01/05/18 07:07, Jan Lahoda wrote:
> Hi,
>
> Thanks for the comments. An updated patch is here:
> http://cr.openjdk.java.net/~jlahoda/8191842/webrev.01/
>
> I addition to addressing the comments, it also fixes var behavior when 
> the inferred type is inaccessible - that is allowed during normal 
> compilation.
>
> Feedback is welcome,
>     Jan
>
> On 13.12.2017 23:31, Robert Field wrote:
>> Part 2: tests
>>
>> ToolSimpleTest is missing the bug number.
>>
>> VariablesTest needs a copyright update.
>>
>> Please add tests for:
>>     * anon classes with added method access
>>     * anon class scratch var
>>
>> -Robert
>>
>>
>> On 12/13/17 13:47, Robert Field wrote:
>>> Nice generalization.
>>>
>>> Wrap:
>>>
>>> 90: this breaks array initialization -- this would no longer work:
>>> int[] d = {1, 2, 3}
>>>
>>> Eval:
>>>
>>> 559: This code should be designed to work the same for automatically
>>> created scratch variables -- aka $1, $2, ... -- they should also have
>>> anonymous types.
>>>
>>> ? 322: Can ei.anonymousClasses be used rather than depending on the
>>> naming added in Util.JSHELL_ANONYMOUS?
>>>
>>> 389, 419, 421, 486-493: the name "constructor" is confusing because it
>>> contains other things and not the complete constructor.  One of the
>>> unwritten rules of wraps in that
>>> they consist of a complete component, this code has several hanging
>>> pieces.  Note the two closing braces in 492, braces et. al. should
>>> always be matched in a wrap.
>>> The mixture of strings and wraps does make this challenging.
>>> I'd suggest doing the bodyParts/extraInit computation first, then
>>> build up each component as complete entities: constructor body, class
>>> body, class -- all wraps.
>>>
>>> ExpressionToTypeInfo:
>>>
>>> 355: Seems fragile to assume that the constructor is first
>>>
>>> -Robert
>>>
>>>
>>> On 12/13/17 02:02, Jan Lahoda wrote:
>>>> Hi,
>>>>
>>>> When doing:
>>>> var r = new Object() { String s; }
>>>>
>>>> the type of "r" is the anonymous class itself. JShell handles this by
>>>> "upgrading" the anonymous class to be a member class, so that it can
>>>> be properly used from other snippets.
>>>>
>>>> But JShell is not prepared for anonymous classes inside the
>>>> expression (that are part of the resulting type), like:
>>>> ---
>>>> jshell> var list = Stream.of(1, 2, 3).map(j -> new Object() { int i =
>>>> j; }).collect(Collectors.toList());
>>>>
>>>> jshell> list.forEach(a -> System.out.println(a.i));
>>>> ---
>>>>
>>>> So, part of the proposed fix is to upgrade all anonymous classes to
>>>> member classes if needed. Also, intersection types are cleared from
>>>> fields before writing (the intersection types are installed to ensure
>>>> var semantics for cases like:
>>>> <Z extends Runnable & CharSequence> Z get1() { return null; }
>>>> var i1 = get1();
>>>> )
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191842
>>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8191842/webrev.00/
>>>>
>>>> Thanks,
>>>>     Jan
>>>>
>>>
>>