WIP: 8154714: JShell API: addExports support [for Java 9.X]
ShinyaYoshida
bitterfoxc at gmail.com
Thu Sep 8 14:14:44 UTC 2016
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)
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
>
More information about the kulla-dev
mailing list