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

[Jan Lahoda]

Thanks for the comments. An updated patch is here:
http://cr.openjdk.java.net/~jlahoda/8191842/webrev.03/

> Eval:
>
>    Nicely documented!
>
>    1218: anonCount is not incremented, so you always have the same
> anonymous class name.
>              Which, of course, is a problem if you have more than one:
>
>              jshell> class A {}
>              |  created class A
>
>              jshell> new A() { int foo() {return 66; }}
>              $2 ==> $0 at 7dc7cbad
>
>              jshell> new A() { int hhoo() {return 99; }}
>              $3 ==> $0 at 548a9f61
>
>              jshell> $3.hhoo()
>              An exception has occurred in the compiler (10-internal).
> Please file a bug against the Java compiler via ...
>              java.lang.ClassCastException:
> jdk.compiler/com.sun.tools.javac.code.Symbol$ClassSymbol cannot be cast to
> jdk.compiler/com.sun.tools.javac.code.Symbol$MethodSymbol
>                      at
> jdk.compiler/com.sun.tools.javac.comp.TransTypes.visitApply(TransTypes.java:676)
>                      at
> jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1634)
>                      ...
>
>              Please include tests with multiple anon classes with member
> accesses.

Oops. Fixed and done.

>
>    402: Nit: "create and explicit class that extens" --> "create AN
> explicit class that EXTENDS"

Fixed.

>
> ExpressionToTypeInfo:
>
>    93: it is conventional, consistent with surrounding vars, and clearer
> to preface predicate names with "is" -- isPrimativeType

Fixed.

>
>    414: for variable declarations with an explicit type, it doesn't seem
> we should be processing anonymous classes in that case

Fixed.

>
> TaskFactory:
>
>    The new code is, for every pass through Enter (of which there are
> several per new snippet), iterating through all the
>    snippets, filtering for valid vars, looking up the wrap class
> containing the var, iterating through the members of
>    that class to find the field representing the var, compiling a cast
> to use to set the type by force.  The field type
>    is restored before generation.
>
>    One concern is that the performance decreases with the number of
> snippets.
>    We need to stay aware of long-running JShell instances -- as for a
> user that leaves the jshell tool open for exploration.
>    The filtering on fullTypeName being non-null for enhanced types
> limits the heavy processing to these rarer cases,
>    so I'm not concerned
>
>    I assume all types downstream of these variables are processed after
> Enter finishes?

Yes. I've changed the code to run through the snippets only once.

>
>    Thanks for the high-level description of setVariableType, however how
> it is implemented is complex enough to deserve
>    documentation (something like the first paragraph).

Fixed.

>
> TreeDissector:
>
>    OK
>
> TypePrinter:
>
>    OK
>
> Util:
>
>    OK
>
> VarSnippet:
>
>    OK
>
> Wrap:
>
>    Nicely documented!
>
> Tests:
>
>    Please include a test covering the bug (above).

Done.

>
>    Please include tests covering:
>      captured variables

There already were multiple such tests? Or what exactly test is missing?

>      extending a variety of complex library classes and interfaces

Done.

>      varied argument types

Done.

>      parameterized types

Done.

>      VarSnippet.typeName() under various conditions

Done.

Jan

>
> -Robert


On 11.1.2018 18:17, Jan Lahoda wrote:
> Hi,
>
> Thanks for all the comments so far. I tried to adjust the code and add
> comments. The updated patch is here:
> http://cr.openjdk.java.net/~jlahoda/8191842/webrev.02/
>
> On 9.1.2018 04:19, Robert Field wrote:
>> 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.
>
> That's not quite simple, as the positions don't match (which could of
> course be solved) and the NewClassTree from the ExpressionToTypeInfo
> also already has the synthetic constructor added. I tried to define one
> method that lists the relevant NewClassTrees in order, and use it on
> both places, which should hopefully make it more robust.
>
> Jan
>
>>
>> 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
>>>>>>
>>>>>
>>>>
>>