WIP: 8154714: JShell API: addExports support [for Java 9.X]
ShinyaYoshida
bitterfoxc at gmail.com
Sat Sep 24 06:49:48 UTC 2016
Hi Robert,
2016-09-23 0:06 GMT-07:00 Robert Field <robert.field at oracle.com>:
>
> On 09/22/16 12:51, ShinyaYoshida wrote:
>
> 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?
>
>
> Wow! This is so much simpler an approach!
>
> A couple little things and this is good...
>
> In JShellTool, should --add-exports have .withRequiredArg() rather than
> .withOptionalArg()?
>
Yeah! That's what I want to use!!
I've fixed
>
> I think
>
> 645 if (options.has("X")) {
> 646 printUsageX();
> 647 }
>
> Should be up below "help", and, like "help", should abort by returning
> null.
>
Absolutely yes!!
New version is here:
http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.11.langtools/
Regards,
shinyafox(ShinyaYoshida)
>
> Your comment looks fine.
>
> Test and resource file look good.
>
> Thanks,
> Robert
>
>
>
>
> 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.0
>>>>>>>>>> 0.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.co
>>>>>>>>>>>> m>>:
>>>>>>>>>>>>
>>>>>>>>>>>> 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