RFR: JDK-8191842: JShell: Inferred type information is lost when assigning types to a "var"
Robert Field
robert.field at oracle.com
Fri Jan 12 04:02:46 UTC 2018
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.
402: Nit: "create and explicit class that extens" --> "create AN
explicit class that EXTENDS"
ExpressionToTypeInfo:
93: it is conventional, consistent with surrounding vars, and clearer
to preface predicate names with "is" -- isPrimativeType
414: for variable declarations with an explicit type, it doesn't seem
we should be processing anonymous classes in that case
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?
Thanks for the high-level description of setVariableType, however how
it is implemented is complex enough to deserve
documentation (something like the first paragraph).
TreeDissector:
OK
TypePrinter:
OK
Util:
OK
VarSnippet:
OK
Wrap:
Nicely documented!
Tests:
Please include a test covering the bug (above).
Please include tests covering:
captured variables
extending a variety of complex library classes and interfaces
varied argument types
parameterized types
VarSnippet.typeName() under various conditions
-Robert
On 01/11/18 09: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
>>>>>>
>>>>>
>>>>
>>
More information about the kulla-dev
mailing list