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

Sun Sep 11 00:35:38 UTC 2016

Hi Robert,
Thank you.

I'll reply inline

2016-09-10 3:45 GMT+09:00 Robert Field <robert.field at oracle.com>:

> Jan, I've been involved enough that I'd like you to take an independent
> look.
>
> Looks great!  I've provided a comment below and a suggestion on exception
> parameters -- you can change these without another review from me.
> I've asked Jan to take a look and because it is an API change, I'll send
> it to Joe to take a look now.  I'll let you know when I get a thumbs up
> from him, then you can push.
>

> As for addToClasspath, a classpath (as specified with the CLASSPATH
> environment variable or -classpath option) for the java or javac commands
> can have non-existent paths on it without error -- seems we should be
> consistent with that.  I could see the tool warning about non-existent
> paths.   Seems though that it should throw IllegalStateException if the
> JShell instance is closed.
>
I see. I've update the issue to produce warning by /classpath command.

>
> JShell.java --
>
> 485,488: I don't think we want to expose the internal exception, all it
> would usually show would be the forwarding mechanism.  But we do want the
> message.  So, I'd pass ex.getMessage() instead -- see line 663 for an
> example.
>
> Comment for ExecutionControl.BadArgumentException:
>
>     /**
>      * An exception indicating that an invalid argument was provided.
>      */
>
> I've updated my previous webrev!
I've changed StreamingExecutionControl#addToClasspath, #setClasspath to
convert BadArgumentException to InternalException and changed comments.

Regards,
shinyafox(Shinya Yoshida)

>
> -Robert
>
>
>
> On 09/08/16 07:14, ShinyaYoshida wrote:
>
> Hi Robert,
> Thank you for your pointing out.
> I've understood.
>
> Here is new webrev:
> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.04.langtools/
>
> In this version,
> - ExecutionControl#addExports throws BadArgumentException which is extends
> ExecutionControlException if module or package are not found.
> - JShell#addExports throws IllegalArgumentException when
> BadArgumentException is occured
>
> I've notice that /classpath and addToClasspath says nothing when it
> doesn't exist.
> I think it should be reported, so I've created a issue:
> https://bugs.openjdk.java.net/browse/JDK-8165650
>
> What do you think?
>
> Please review me again!
>
> Thank you,
> shinyafox(Shinya Yoshida)
>
>
> 2016-09-08 4:25 GMT+09:00 Robert Field <robert.field at oracle.com>:
>
>> Thanks for the update.
>>
>> l10n.properties --
>>
>> Oops, typo: the help.set.exports entry got pasted again instead of
>> help.retain.exports
>>
>> Sorry I was unclear about the exceptions...
>>
>> On the execution side --
>>
>> ExecutionControl.InternalException is only for unexpected failures.
>>
>> In this case you are dealing an expected exception that occurs when the
>> user gives the wrong input.  None of the existing ExecutionControlException
>> subclasses are good candidates, so a new ExecutionControlException subclass
>> is needed.  I was suggesting "BadArgumentException" for this exception.
>> None of ExecutionControlException subclasses are seen at the JShell level
>> -- this should be the case for this new one as well.
>>
>> I don't what kind of exceptions are thrown by
>> moduleAccess.addExportsToAllUnnamed(target, pack), but that message text
>> or some other appropriate descriptive message text needs to be placed in
>> the BadArgumentException message so it can be seen all the way up
>> (including the JShellTool).
>>
>>
>> On the JShell class --
>>
>> Incorrect arguments on JShell API methods generate the Java standard
>> java.lang.IllegalArgumentException (see, for example, drop() or status()
>> ) -- so, this is also the appropriate exception for JShell.addExports().
>> Thus, JShell.addExports() catchs the BadArgumentException, turning it into
>> an IllegalArgumentException.
>>
>> Thanks,
>> Robert
>>
>>
>> On 09/07/16 06:10, ShinyaYoshida wrote:
>>
>> 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)
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>
>