WIP: 8154714: JShell API: addExports support [for Java 9.X]
ShinyaYoshida
bitterfoxc at gmail.com
Tue Sep 13 00:55:37 UTC 2016
Hi Jan and all,
I've confirmed your snippet.
Certainly it have a security problem and it should be avoided.
I'll update my patch to add a new command-line option -C which give the
option to compiler.
In addition to, I'd like to add a new command-line option --add-exports
<module>/<package> which is a short-circuit for "-J--add-exports
<module>/<package>=ALL-UNNAMED -C--add-exports
<module>/<package>=ALL-UNNAMED."
java and javac are also have --add-exports option, so it seems to me that
it is reasonable for jshell to have --add-exports option.
What do you think about this?
Kind Regards,
shinyafox(Shinya Yoshida)
2016-09-13 9:38 GMT+09:00 Robert Field <robert.field at oracle.com>:
> If our approach to security is "do nothing a user could not do with their
> own use of javac/java" (which we have yet to have validated) then if the
> SharedSecrets use in JShell requires the change to the module.info.java in
> java.base to add jdk.jshell, then that would be a violation.
>
> If this changeset gave access only to snippets executing via the JShell
> API maybe that would be OK. But it, as your exploit verifies, could easily
> be used by anyone to change module boundaries.
>
> Maybe there would be a hack that would allow only proper JShell use, but I
> don't see what that would be, and that sounds dangerous and unappealing.
>
> Command-line option only would match the original request and other
> tools. The implementation would closely match the implementation of '-R',
> being on the command-line of JShellTool and the API in JShell.Builder.
> Alas, this is mostly a rewrite, only some code in TaskFactory,
> ToolBasicTest, and maybe KullaTesting would be reusable.
>
> Bummer!
>
> But thanks for catching this Jan!
>
> -Robert
>
>
>
> On 09/12/16 14:12, Jan Lahoda wrote:
>
>> (Basically, the use of SharedSecrets in jdk.jshell seems highly
>> suspicious to me.)
>>
>> Jan
>>
>> On 12.9.2016 23:05, Jan Lahoda wrote:
>>
>>> On 12.9.2016 20:06, Robert Field wrote:
>>>
>>>> The jshell tool is built on the JShell API. Command-line options, like
>>>> -R, pass to the remote process via the JShell API. -R would allow
>>>> --patch-module or --add-exports. Having addExports functionality
>>>>
>>>
>>> I am not much worried about --add-exports added using -R, that seems OK
>>> to me. This only affects a newly spawned JVM, and the API client could
>>> spawn it even without JShell, so not a problem IMO.
>>>
>>> What I am worried about is that using the most recent proposed patch
>>> (lwebrev.04.langtools), one can use the DirectExecutionControl to change
>>> module boundaries in the _currently running VM_. Please see the attached
>>> class to see what I mean - I didn't even have to create JShell as such,
>>> simply using DirectExecutionControl was enough. It probably does not
>>> even need some special privileges to achieve the result.
>>>
>>> Jan
>>>
>>> anywhere is jshell means having it in the JShell API.
>>>>
>>>> I believe this cycles us, to some degree, back to the security
>>>> discussion. What is our level of concern about a launched process
>>>> running pure JDK code?
>>>>
>>>> -Robert
>>>>
>>>> On 09/12/16 09:57, Jan Lahoda wrote:
>>>>
>>>>> [adding Jon]
>>>>>
>>>>> On 12.9.2016 17:31, Robert Field wrote:
>>>>>
>>>>>> Both javac and java have a --add-exports option, so it isn't opening
>>>>>> anything that isn't already open.
>>>>>> I don't see how command-line options are any different that API in
>>>>>> terms
>>>>>> of exposure.
>>>>>>
>>>>>
>>>>> I think there's a huge difference between command line and an API, in
>>>>> at least two aspects:
>>>>> a) the person that controls the command line, has a lot of power in
>>>>> any case (they could do -Xbootclasspath/p: in 8 or --patch-module now
>>>>> to get around the boundaries). API users are much more restricted in
>>>>> what they can do.
>>>>> b) affecting the command line options is generally very hard - imagine
>>>>> library maintainers - how can they affect the command line setting?
>>>>> The only thing they can do is to write some documentation, but they
>>>>> cannot directly affect the command line. But they can call an API (and
>>>>> they can call it even reflectively).
>>>>>
>>>>> I believe it is generally undesirable to create APIs that allow to
>>>>> create holes in module boundaries. If you think that's something
>>>>> JShell needs, I think we need to take that to the Jigsaw team.
>>>>>
>>>>> Jan
>>>>>
>>>>>
>>>>>> -Robert
>>>>>>
>>>>>>
>>>>>> On 09/11/16 13:17, Jan Lahoda wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I apologize for not following this sooner.
>>>>>>>
>>>>>>> Looking at this, it seems to me the change would effectively allow to
>>>>>>> overcome the module boundary limitations using JShell - I don't think
>>>>>>> that's something that would be acceptable. We could take this to the
>>>>>>> Jigsaw team, but I don't think they would be happy about this.
>>>>>>>
>>>>>>> I wonder what exactly is the usecase here. Would it be sufficient if
>>>>>>> the jshell tool as such would allow command line options to break
>>>>>>> through the boundaries? We already have -R, which should allow to
>>>>>>> make
>>>>>>> holes to the boundaries when running the snippets, so maybe we just
>>>>>>> need to add something for compile-time?
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>> On 11.9.2016 02:12, ShinyaYoshida wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>> Please apply this webrev:
>>>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/
>>>>>>>> <http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/>
>>>>>>>>
>>>>>>>> Changing module-info is needed.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> shinyafox(Shinya Yoshida)
>>>>>>>>
>>>>>>>> 2016-09-10 7:51 GMT+09:00 Robert Field <robert.field at oracle.com
>>>>>>>> <mailto:robert.field at oracle.com>>:
>>>>>>>>
>>>>>>>> [audience reduced]
>>>>>>>>
>>>>>>>> BTW shinyafox,
>>>>>>>>
>>>>>>>> Building "make images" with this patch and the latest jdk9/dev
>>>>>>>> bits
>>>>>>>> fails. Probably need to change module set-up --
>>>>>>>>
>>>>>>>> /w/y/dev/langtools/src/jdk.jshell/share/classes/jdk/jshell/
>>>>>>>> execution/DirectExecutionControl.java:33:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> error: package jdk.internal.misc does not exist
>>>>>>>> import jdk.internal.misc.JavaLangReflectModuleAccess;
>>>>>>>> ^
>>>>>>>> /w/y/dev/langtools/src/jdk.jshell/share/classes/jdk/jshell/
>>>>>>>> execution/DirectExecutionControl.java:34:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> error: package jdk.internal.misc does not exist
>>>>>>>> import jdk.internal.misc.SharedSecrets;
>>>>>>>> ^
>>>>>>>> /w/y/dev/langtools/src/jdk.jshell/share/classes/jdk/jshell/
>>>>>>>> execution/DirectExecutionControl.java:155:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> error: cannot find symbol
>>>>>>>> JavaLangReflectModuleAccess moduleAccess =
>>>>>>>> SharedSecrets.getJavaLangReflectModuleAccess();
>>>>>>>> ^
>>>>>>>> symbol: class JavaLangReflectModuleAccess
>>>>>>>> location: class DirectExecutionControl
>>>>>>>> /w/y/dev/langtools/src/jdk.jshell/share/classes/jdk/jshell/
>>>>>>>> execution/DirectExecutionControl.java:155:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> error: cannot find symbol
>>>>>>>> JavaLangReflectModuleAccess moduleAccess =
>>>>>>>> SharedSecrets.getJavaLangReflectModuleAccess();
>>>>>>>> ^
>>>>>>>> symbol: variable SharedSecrets
>>>>>>>> location: class DirectExecutionControl
>>>>>>>> 4 errors
>>>>>>>>
>>>>>>>> -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/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>> v.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
>>>>>>>>> <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
>>>>>>>>> <mailto: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/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>> v.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 <mailto: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/
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>> v.02.langtools/>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I added an issue for the completion feature.
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8165445
>>>>>>>>>>> <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 <mailto: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
>>>>>>>>>>> <mailto: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/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.01.langtools/>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> webrev.01.jdk:
>>>>>>>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.
>>>>>>>>>>>> 01.jdk/
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.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
>>>>>>>>>>>> <mailto: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
>>>>>>>>>>>> <mailto: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
>>>>>>>>>>>> <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
>>>>>>>>>>>> <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
>>>>>>>>>>>> <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/webre
>>>>>>>>>>>> v.00.jdk/>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.00.jdk/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.00.jdk/>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> langtools:
>>>>>>>>>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.
>>>>>>>>>>>> 00.langtools/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.00.langtools/>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webre
>>>>>>>>>>>> v.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