RFR: JDK-8191842: JShell: Inferred type information is lost when assigning types to a "var"
Robert Field
robert.field at oracle.com
Mon Jan 15 21:33:51 UTC 2018
Looks good!
There is an indentation problem in Eval.anonymous2Member. No need for
re-review after format.
Great new functionality to have.
-Robert
On 01/15/18 12:24, Jan Lahoda wrote:
> 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
>>>>>>>
>>>>>>
>>>>>
>>>
More information about the kulla-dev
mailing list