WIP: 8154714: JShell API: addExports support [for Java 9.X]
ShinyaYoshida
bitterfoxc at gmail.com
Wed Sep 7 13:10:02 UTC 2016
Hi Robert,
Thank you for your helping.
I've update webrev according to your comments.
If I'm missing your meaning, I'm sorry. Please point it.
http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.03.langtools/
Now,
- JShell#addExports will throw BadArgumentException when module or package
is wrong, which is check exception.
- JShellTool checks that exception.
- ExecutionControl#addExports throws InternalException when something is
wrong.
Regards,
shinyafox(Shinya Yoshida)
2016-09-06 6:28 GMT+09:00 Robert Field <robert.field at oracle.com>:
> Code looks good. Needs docs I promised (see below).
>
> As to completion issue you created, I see you already added discussion and
> tests. I added your completion code.
>
> Below is my suggested doc...
>
> src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n.properties
> --
>
> /set exports <module> <package>\n\t\
> Allow access to some of the unexported types of a module\n\n\
>
>
> /retain exports <module> <package>\n\t\
> Allow access to some of the unexported types of a module\n\n\
>
>
> help.set.exports=\
> Set a package to add to those exported to snippets.\n\
> \n\t\
> /set exports <module> <package>\n\
> \n\
> Where <module> is the module from which to export <package>.\n\
> Where <package> is the package to export to snippets.\n
>
>
> help.retain.exports=\
> Retain a package to add to those exported to snippets.\n\
> \n\t\
> /retain exports <module> <package>\n\
> \n\
> Where <module> is the module from which to export <package>.\n\
> Where <package> is the package to export to snippets.\n
>
> [I notice that 'snippet' is used in doc without being defined. If, while
> you are it, you change the help for /list to define it:]
>
> help.list =\
> Show the source of snippets, prefaced with the snippet id (a snippet is a
> chunk of code that the jshell tool evaluates -- the code you enter at the
> prompt is a snippet).\n\
> ...
>
> src/jdk.jshell/share/classes/jdk/jshell/JShell.java --
>
>
> /**
> * Add a package to those exported to snippets.
> * @param module The module from which to export
> * @param pack The package to export
> * @throws NullPointerException if arguments are null.
> * @throws IllegalStateException if this {@code JShell} instance is
> closed.
> * @throws XYZ if ....
> */
>
> OK, actually this code should change. Like addToClasspath above it, it
> should return void. It looks like under normal operation it can fail --
> the failure should be reported with an exception.
> What are the failure modes? IllegalArgumentException would be likely
> choice at the JShell API layer. But then the exception needs to be
> propagated there.
> Since none of the existing exceptions is a match, this means a new
> ExecutionControlException. Maybe something general, like
> BadUserArgumentException.
> Note: I've also added IllegalStateException -- this can be thrown when you
> get EngineTerminationException.
> Also, typo, the second "module cannot be null" should be "package cannot
> be null"
>
> src/jdk.jshell/share/classes/jdk/jshell/execution/RemoteCodes.java --
>
> The comment you have is fine. Just remove PLEASE REVISE THE COMMENT
>
> src/jdk.jshell/share/classes/jdk/jshell/spi/ExecutionControl.java --
>
> /**
> * Add a package to those exported to snippets.
> * @param module The module from which to export
> * @param pack The package to export
> * @throws EngineTerminationException the execution engine has
> terminated
> * @throws ....
> * @throws InternalException an internal problem occurred
> */
>
> Plus add the bad argument exception.
> Whatever it is, should be added to extensionCommand() exceptions.
>
> Robert
>
>
>
> On 09/05/16 08:02, ShinyaYoshida wrote:
>
> Hi,
> I've update webrev of langtools:
> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.02.langtools/
>
> I added an issue for the completion feature.
> https://bugs.openjdk.java.net/browse/JDK-8165445
>
> Regards,
> shinyafox(Shinya Yoshida)
>
> 2016-09-05 22:49 GMT+09:00 ShinyaYoshida <bitterfoxc at gmail.com>:
>
>> 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/k
>>> ulla/8154714/webrev.01.langtools/
>>> webrev.01.jdk: http://cr.openjdk.java.net/~shinyafox/kulla/8
>>> 154714/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.0
>>>>>> 0.jdk/ <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>> v.00.jdk/>
>>>>>> langtools: http://cr.openjdk.java.net/~sh
>>>>>> inyafox/kulla/8154714/webrev.00.langtools/ <
>>>>>> http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>> v.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)
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
More information about the kulla-dev
mailing list