WIP: 8154714: JShell API: addExports support [for Java 9.X]

Mon Sep 5 18:10:09 UTC 2016

Great!

It would be great if you copy your code and the discussion into the issue 
so we don't lose context.

Thanks!
Robert

On September 5, 2016 6:49:43 AM ShinyaYoshida <bitterfoxc at gmail.com> wrote:

> Hi Robert,
> Thank you for the review.
>
> I totally agree with you.
> I also think that availableModules and availablePackages(module) are
> worthless excepting for the command completion feature.
> I'd like to avoid adding these APIs which have not been speculated and
> revised because if API is once added, it is too difficult to remove than
> adding.
>
> So currently, I'll remove the completion feature for /set or /retain
> exports and make the issue to JBS to add completion feature for them in
> future.
>
> I'll update the webrev. So please review it again later.
> When the silently evaluation feature is added, I'd like to work for the
> completion feature again.
>
> Thank you,
> shinyafox(Shinya Yoshida)
>
>
> 2016-09-05 15:00 GMT+09:00 Robert Field <robert.field at oracle.com>:
>
>>
>> On 09/03/16 05:07, ShinyaYoshida wrote:
>>
>> Hi all,
>> I've updated my patch, new webrev is here:
>> webrev.01.langtools: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/
>> webrev.01.langtools/
>> webrev.01.jdk: http://cr.openjdk.java.net/~shinyafox/
>> kulla/8154714/webrev.01.jdk/ (Same as previous)
>>
>> In this version,
>> - rename /set add-exports to /set exports
>>
>>
>> OK
>>
>> - add /retain exports <module> <package>
>>
>>
>> The /retain implementation looks good.
>>
>> - add tests
>>
>>
>> Well tested
>>
>> - move EXT_CMD_XXX into RemoteCodes
>> - make RemoteCodes public and move it into jdk.jshell.execution.code which
>> is module-private
>>
>>
>> So, here, in addition to addExports, there are two more methods added to
>> the API availableModules() and availablePackage(String module).
>> I see how in rare cases they could be useful.  And the rare case that
>> causes them to be present is for command completion in the tool.
>> What are the options:
>>
>>   (1) Add them to the API.  If it is in the API then it is a first-class
>> form of functionality and should be treated as such in execution support
>> (as a command)
>>   (2) Not support them and not support command completion for /set
>> exports.  Command completion is not supported in much more common cases.
>> Then we could support command completion in the future (see (3)).
>>   (3) What these commands do is simply execute an expression on the remote
>> side.  But we already have a mechanism to execute expressions on the remote
>> side: Snippets.
>>         That leaves a problem: we wouldn't want these Snippets to be seen
>> by the user, but we have a solution for that the tool uses the idGenerator.
>>         Another problem is that, by default and with no current way around
>> the default, expressions generate temp-variables.  My intention was to make
>> this configurable -- see Eval.shouldGenTempVar(...).
>>
>> One goal I've been using in the design of the API is to keep it as small
>> and simple as possible.  This improves maintainability, but much more
>> importantly makes the API easier to learn and use.
>> So, for something to warrant inclusion it should be generally useful and
>> have no other way of being achieved -- neither is true of these two
>> queries.
>>
>> My suggestion, particularly in light of how late in the release this is,
>> would be (2).  Thoughts?
>>
>> -Robert
>>
>>
>>
>>
>>
>>
>> Regards,
>> shinyafox(Shinya Yoshida)
>>
>> 2016-09-03 18:15 GMT+09:00 ShinyaYoshida <bitterfoxc at gmail.com>:
>>
>>> Hi Robert,
>>> Thank you for reviewing.
>>> I'll update my patch later.
>>>
>>> 2016-09-02 11:38 GMT+09:00 Robert Field <robert.field at oracle.com>:
>>>
>>>> Thanks shiyafox! This is a significant piece of code!
>>>>
>>>> As to your questions --
>>>>
>>>> > I wonder if  you could work on the documentation of the API and the
>>>> tool instead?
>>>>
>>>> Yes, I will do API and tool docs.
>>>
>>> Thank you!
>>>
>>> > Subcommand of /set is reasonable? Similar command, /classpath, to make
>>>> users available to use new packages is not a subcommand of /set.
>>>>
>>>> Yes, I think /set exports is best.  And, yes, /classpath is
>>>> inconsistent, it should be /set classpath, that can be a separate task,
>>>> unless you want to roll that in.
>>>
>>> I see, I've created the issue to move /classpath to /set classpath:
>>> https://bugs.openjdk.java.net/browse/JDK-8165405
>>>
>>>
>>>> > Should we need retain feature to this?
>>>>
>>>> All /set have a /retain counterpart -- we should maintain that
>>>>
>>> Ok, I'll add /retain exports.
>>> I've also create the issue for retaining classpath:
>>> https://bugs.openjdk.java.net/browse/JDK-8165406
>>>
>>>
>>>> As to the code review: Looks good to me, just minor questions/issues --
>>>>
>>>> JShellTool.java:
>>>>
>>>> * As discussed /set add-exports => /set exports
>>>>
>>>> * Nit: a comment got moved out of place line 1194(old)
>>>>
>>>> JShell.java:
>>>>
>>>> * EXT_CMD_AVAILABLE_MODULES and EXT_CMD_AVAILABLE_PACKAGES, these should
>>>> be referenced via RemoteCodes.java rather than be coped.
>>>>
>>> Absolutely! I'd like to move these into RemoteCodes but it is
>>> package-private class and cannot be referred from JShell.
>>> Can I make RemoteCodes public and move it to another module-private
>>> package like jdk.jshell.execution.code?
>>>
>>>
>>>> * Which leads me to another question: why is addExports a command and
>>>> these two are extensions.  It seems fine, just curious.  I notice that
>>>> extensions are a lot less code -- ;-)
>>>
>>> Exporting module and package is similar to adding classpath to platform,
>>> so I made the addExports a command like setClasspath.
>>> And it should be added as API because it is very basic feature.
>>>
>>>
>>>> TaskFactory.java:
>>>>
>>>> * Question: the "/" separator between module and package is that
>>>> platform independent?
>>>>
>>> It seems to me that "{module}/{path}" format for --add-exports option of
>>> javac is platform independent.
>>>
>>> Regards,
>>> shinyafox(Shinya Yoshida)
>>>
>>>
>>>> DefaultLoaderDelegate.java:
>>>>
>>>> * Jan, can you look at this code?
>>>>
>>>>
>>>> -Robert
>>>>
>>>>
>>>> On 09/01/16 05:36, ShinyaYoshida wrote:
>>>>
>>>>> Hi all,
>>>>> I've implemented this feature to JShell API and Tool with full
>>>>> completion:
>>>>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8154714
>>>>>
>>>>> jdk: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/
>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>
>>>>> langtools: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.0
>>>>> 0.langtools/ <http://cr.openjdk.java.net/%7
>>>>> Eshinyafox/kulla/8154714/webrev.00.langtools/>
>>>>>
>>>>> This is missing the train of Java 9.0 but it could catch the train of
>>>>> Java 9.1 or later.
>>>>> I want to get your comments for this.
>>>>>
>>>>> jshell> /set add-exports jdk.jconsole sun.tools.jconsole
>>>>>
>>>>> jshell> import sun.tools.jconsole.*
>>>>>
>>>>> jshell> LocalVirtualMachine.getAllVirtualMachines().forEach((k, v) ->
>>>>> System.out.println(k + ":" + v))
>>>>> 10778:jdk.jshell/jdk.internal.jshell.tool.JShellTool
>>>>> 10826:jdk.jshell.execution.RemoteExecutionControl 35548
>>>>> 28447:org.netbeans.Main --cachedir /home/bitter_fox/.cache/netbeans/8.0.1
>>>>> --userdir /home/bitter_fox/.netbeans/8.0.1 --branding nb
>>>>>
>>>>> Currently, there is no testing, so it will be required to add test.
>>>>>
>>>>> Regards,
>>>>> shinyafox(Shinya Yoshida)
>>>>>
>>>>
>>>>
>>>
>>
>>