WIP: 8154714: JShell API: addExports support [for Java 9.X]
ShinyaYoshida
bitterfoxc at gmail.com
Thu Sep 22 19:51:08 UTC 2016
Hi Robert and Jan,
I've updated the webrev to add -C option and --add-exports option(as a
non-standard option):
http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.10.langtools/
Could you review and give me a comment?
Another issue is found until implementing:
https://bugs.openjdk.java.net/browse/JDK-8166581
Could you confirm and address this?
Regards,
shinyafox(Shinya Yoshida)@JavaOne 20016
2016-09-13 9:26 GMT-07:00 Robert Field <robert.field at oracle.com>:
>
> On 09/12/16 17:55, ShinyaYoshida wrote:
>
> 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)
>
>
> OK, great.
>
> I started to write that I didn't think you needed or wanted a -C<opt>
> flag. But on further thought, that allows all kinds of flexibility that we
> may not want to provide item by item -- for example:
> --processor-module-path, --module-source-path, ....
> There is a question: which compiles should these flags get applied to, but
> it seems they would just be ignored where not needed, so probably all of
> them.
>
> I notice --add-exports is on the -X help for both java and javac -- it is
> non-standard. Seems wrong for it to be a non-standard option for those two
> and a standard option for jshell.
>
> I think it is a question for Jigsaw people as to what module options:
>
> * should be directly jshell command-line options
> * should be non-standard -X command-line options (if jshell has -X options)
> * obscure enough that users should use -R<opt> and -C<opt>
>
> -Robert
>
>
>
>
>
> 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/e
>>>>>>>>> xecution/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/e
>>>>>>>>> xecution/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/e
>>>>>>>>> xecution/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/e
>>>>>>>>> xecution/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.0
>>>>>>>>>> 4.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.0
>>>>>>>>>>> 3.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.0
>>>>>>>>>>>> 2.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.0
>>>>>>>>>>>>> 1.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.0
>>>>>>>>>>>>> 1.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.0
>>>>>>>>>>>>> 0.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.0
>>>>>>>>>>>>> 0.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