Seems OK to me.
Jan
On 26.9.2016 22:20, ShinyaYoshida wrote:
> Hi Robert,
> Thank you for asking for the internal review.
>
> I've updated and replaced the previous webrev!
> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.11.langtools/
>
> Regards,
> shinyafox(Shinya Yoshida)
>
> 2016-09-26 10:10 GMT-07:00 Robert Field <robert.field at oracle.com
> <mailto:robert.field at oracle.com>>:
>
> That's great!
>
> I've submitted this to internal review of the new command-line
> options. We should get that in less than a week.
>
> One nit, I've noticed that --help doesn't list the new -X option.
> Borrowing from "java -help" this should be:
>
> -X Print help on non-standard options
>
>
> Thanks!
>
> -Robert
>
>
>
> On 09/23/16 23:49, ShinyaYoshida wrote:
>> Hi Robert,
>>
>>
>> 2016-09-23 0:06 GMT-07:00 Robert Field <robert.field at oracle.com
>> <mailto: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/
>>> <http://cr.openjdk.java.net/%7Eshinyafox/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/
>> <http://cr.openjdk.java.net/%7Eshinyafox/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
>>> <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 <mailto: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 <mailto: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/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>
>>>> <http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/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>
>>>> <mailto: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/webrev.04.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.04.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.04.langtools/>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> In this version,
>>>> -
>>>> ExecutionControl#addExports
>>>> throws
>>>> BadArgumentException
>>>> which is
>>>> extends
>>>> ExecutionControlException
>>>> if module or package
>>>> are not
>>>> found.
>>>> -
>>>> JShell#addExports
>>>> throws
>>>> IllegalArgumentException
>>>> when
>>>> BadArgumentException
>>>> is occured
>>>>
>>>> I've notice that
>>>> /classpath and
>>>> addToClasspath says
>>>> nothing when
>>>> it doesn't exist.
>>>> I think it
>>>> should be reported,
>>>> so I've created a issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8165650
>>>> <https://bugs.openjdk.java.net/browse/JDK-8165650>
>>>> <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>
>>>> <mailto: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/webrev.03.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.03.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.03.langtools/>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Now,
>>>> -
>>>> JShell#addExports will
>>>> throw
>>>> BadArgumentException
>>>> when
>>>> module
>>>> or package is
>>>> wrong, which is
>>>> check exception.
>>>> -
>>>> JShellTool
>>>> checks that
>>>> exception.
>>>> -
>>>> ExecutionControl#addExports
>>>> throws
>>>> InternalException when
>>>> something is wrong.
>>>>
>>>> Regards,
>>>> shinyafox(Shinya
>>>> Yoshida)
>>>>
>>>>
>>>> 2016-09-06 6:28
>>>> GMT+09:00 Robert
>>>> Field
>>>>
>>>> <robert.field at oracle.com
>>>> <mailto:robert.field at oracle.com>
>>>> <mailto: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/webrev.02.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.02.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.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>
>>>> <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>
>>>> <mailto: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>
>>>> <mailto: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/webrev.01.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.01.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.01.langtools/>>
>>>>
>>>>
>>>>
>>>>
>>>> webrev.01.jdk:
>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.01.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.01.jdk/>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.01.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.01.jdk/>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> (Same as
>>>> previous)
>>>>
>>>> In
>>>> this
>>>> version,
>>>> -
>>>> rename
>>>> /set
>>>> add-exports
>>>> to /set
>>>> exports
>>>>
>>>>
>>>> OK
>>>>
>>>> -
>>>> add
>>>> /retain
>>>> exports
>>>> <module>
>>>> <package>
>>>>
>>>>
>>>> The
>>>> /retain
>>>> implementation
>>>> looks good.
>>>>
>>>> -
>>>> add tests
>>>>
>>>>
>>>> Well
>>>> tested
>>>>
>>>> -
>>>> move
>>>> EXT_CMD_XXX
>>>> into
>>>> RemoteCodes
>>>> -
>>>> make
>>>> RemoteCodes
>>>> public
>>>> and move
>>>> it into
>>>> jdk.jshell.execution.code
>>>> which is
>>>> module-private
>>>>
>>>>
>>>> So,
>>>> here, in
>>>> addition to
>>>> addExports,
>>>> there are
>>>> two
>>>> more methods
>>>> added to the API
>>>> availableModules()
>>>> and
>>>> availablePackage(String
>>>> module).
>>>> I see
>>>> how in rare
>>>> cases they
>>>> could be useful.
>>>> And
>>>> the rare
>>>> case that
>>>> causes them
>>>> to be
>>>> present
>>>> is for
>>>> command
>>>> completion
>>>> in the tool.
>>>> What
>>>> are the options:
>>>>
>>>> (1)
>>>> Add them to
>>>> the API. If
>>>> it is in the
>>>> API
>>>> then
>>>> it is a
>>>> first-class
>>>> form of
>>>> functionality
>>>> and
>>>> should be
>>>> treated as
>>>> such in
>>>> execution
>>>>
>>>> support (as
>>>> a command)
>>>> (2)
>>>> Not support
>>>> them and not
>>>> support command
>>>> completion
>>>> for /set
>>>> exports. Command
>>>> completion
>>>> is not
>>>> supported in
>>>> much more
>>>> common cases.
>>>> Then
>>>> we could
>>>> support
>>>> command
>>>> completion in
>>>> the
>>>> future
>>>> (see (3)).
>>>> (3)
>>>> What these
>>>> commands do
>>>> is simply
>>>> execute
>>>> an
>>>> expression
>>>> on the
>>>> remote side.
>>>> But we
>>>>
>>>> already have
>>>> a mechanism
>>>> to execute
>>>> expressions
>>>> on the
>>>> remote side:
>>>> Snippets.
>>>> That leaves
>>>> a problem:
>>>> we wouldn't
>>>> want
>>>> these
>>>> Snippets to
>>>> be seen by
>>>> the user, but we
>>>> have a
>>>> solution for
>>>> that the
>>>> tool uses the
>>>> idGenerator.
>>>> Another
>>>> problem is
>>>> that, by default
>>>> and
>>>> with
>>>> no current
>>>> way around
>>>> the default,
>>>> expressions
>>>> generate
>>>> temp-variables.
>>>> My
>>>> intention
>>>> was to make
>>>> this
>>>> configurable
>>>> -- see
>>>> Eval.shouldGenTempVar(...).
>>>>
>>>> One
>>>> goal I've
>>>> been using
>>>> in the
>>>> design of the
>>>> API is
>>>> to keep it
>>>> as small and
>>>> simple as
>>>> possible.
>>>> This
>>>> improves
>>>> maintainability,
>>>> but
>>>> much
>>>> more
>>>> importantly
>>>> makes the
>>>> API easier to
>>>> learn
>>>> and use.
>>>> So,
>>>> for
>>>> something to
>>>> warrant
>>>> inclusion it
>>>> should
>>>> be
>>>> generally
>>>> useful and
>>>> have no
>>>> other way of
>>>> being
>>>> achieved --
>>>> neither is
>>>> true of
>>>> these two
>>>> queries.
>>>>
>>>> My
>>>> suggestion,
>>>> particularly
>>>> in light of how
>>>> late
>>>> in the
>>>> release this
>>>> is, would be
>>>> (2).
>>>> Thoughts?
>>>>
>>>> -Robert
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Regards,
>>>> shinyafox(Shinya
>>>> Yoshida)
>>>>
>>>> 2016-09-03
>>>> 18:15
>>>> GMT+09:00 ShinyaYoshida
>>>>
>>>> <bitterfoxc at gmail.com
>>>> <mailto:bitterfoxc at gmail.com>
>>>> <mailto: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>
>>>> <mailto: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>
>>>> <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>
>>>> <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>
>>>> <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/webrev.00.jdk/>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> langtools:
>>>> http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/>>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/>
>>>>
>>>>
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.langtools/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.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)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>