Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes [v2]
On Thu, 24 Mar 2022 18:58:43 GMT, Joe Darcy wrote: >> Small refactoring to use sealed classes in the MethodHandle implementation >> hierarchy. >> >> DelegatingMethodHandle is non-sealed rather than sealed since it has two >> subclasses, one defined as a nested class and only a separate type in the >> same package. The permits clause does not allow listed those two kinds of >> subclasses. >> >> Please also review the corresponding CSR >> https://bugs.openjdk.java.net/browse/JDK-8283434 > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8283416 > - JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes [v2]
> Small refactoring to use sealed classes in the MethodHandle implementation > hierarchy. > > DelegatingMethodHandle is non-sealed rather than sealed since it has two > subclasses, one defined as a nested class and only a separate type in the > same package. The permits clause does not allow listed those two kinds of > subclasses. > > Please also review the corresponding CSR > https://bugs.openjdk.java.net/browse/JDK-8283434 Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Respond to review feedback. - Merge branch 'master' into JDK-8283416 - JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7881/files - new: https://git.openjdk.java.net/jdk/pull/7881/files/f4f79411..e7c6495f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7881=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7881=00-01 Stats: 65233 lines in 1034 files changed: 62326 ins; 1345 del; 1562 mod Patch: https://git.openjdk.java.net/jdk/pull/7881.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7881/head:pull/7881 PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On 22/03/2022 4:56 pm, Rémi Forax wrote:
On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss wrote:
javac should be changed to allow fully qualified access to private inner
classes in the permits clause of an enclosing class.
This is not how private works, private means accessible between the '{' and the
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class,
thus a private member class is not visible from the permits clause.
This seems like an oversight in the Sealed classes specification. It
makes perfect sense to have a sealed class that can only be extended by
its own nested classes, so you should be able to clearly state that.
David
-
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote: > Small refactoring to use sealed classes in the MethodHandle implementation > hierarchy. > > DelegatingMethodHandle is non-sealed rather than sealed since it has two > subclasses, one defined as a nested class and only a separate type in the > same package. The permits clause does not allow listed those two kinds of > subclasses. > > Please also review the corresponding CSR > https://bugs.openjdk.java.net/browse/JDK-8283434 But the private inner classes are visible to the compiled `PermittedSubclasses` class attribute (otherwise it wouldn’t be possible for private inner classes to extend a sealed outer class with an implicit `permits` clause). --- Technically, `private` means accessible only within the current nest. - PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss wrote:
> javac should be changed to allow fully qualified access to private inner
> classes in the permits clause of an enclosing class.
This is not how private works, private means accessible between the '{' and the
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class,
thus a private member class is not visible from the permits clause.
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote: > Small refactoring to use sealed classes in the MethodHandle implementation > hierarchy. > > DelegatingMethodHandle is non-sealed rather than sealed since it has two > subclasses, one defined as a nested class and only a separate type in the > same package. The permits clause does not allow listed those two kinds of > subclasses. > > Please also review the corresponding CSR > https://bugs.openjdk.java.net/browse/JDK-8283434 **javac** should be changed to allow fully qualified access to private inner classes in the `permits` clause of an enclosing class. - PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Mon, 21 Mar 2022 18:17:06 GMT, Mandy Chung wrote:
> > $ javac com/acme/*.java
> > is fine. Am I missing something?
>
> If you make P.Q private, it will fail the compilation.
>
Yes, I got that part. I did make a comment earlier ("Two of those subclasses
are nested "private" in another class and hence cannot be referred from
DelegatingMethodHandle's permits clause, right?"). My example was in response
to "I did try to name the nested type in the permits clause, and it was not
accepted by javac." comment by @jddarcy. I wondered if there is any other issue
with permits beyond private access of those 2 nested classes.
> To make `DelegatingMethodHandle` a sealed class, we could make
> `AsVarargsCollector` and `WrappedMember` package-private classes.
right.
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Mon, 21 Mar 2022 06:08:32 GMT, Athijegannathan Sundararajan wrote: > $ javac com/acme/*.java > > is fine. Am I missing something? If you make P.Q private, it will fail the compilation. To make `DelegatingMethodHandle` a sealed class, we could make `AsVarargsCollector` and `WrappedMember` package-private classes. - PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote: > Small refactoring to use sealed classes in the MethodHandle implementation > hierarchy. > > DelegatingMethodHandle is non-sealed rather than sealed since it has two > subclasses, one defined as a nested class and only a separate type in the > same package. The permits clause does not allow listed those two kinds of > subclasses. > > Please also review the corresponding CSR > https://bugs.openjdk.java.net/browse/JDK-8283434 LGTM - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote:
> Small refactoring to use sealed classes in the MethodHandle implementation
> hierarchy.
>
> DelegatingMethodHandle is non-sealed rather than sealed since it has two
> subclasses, one defined as a nested class and only a separate type in the
> same package. The permits clause does not allow listed those two kinds of
> subclasses.
>
> Please also review the corresponding CSR
> https://bugs.openjdk.java.net/browse/JDK-8283434
Hmm.. I tried the following example:
$ cat com/acme/X.java
package com.acme;
// We can omit permits clause if all the subclasses are in the same compilation
unit.
// But that's applicable here as two subclasses "Z", "P.Q" are outside the
compilation unit.
// So we use explicit permits clause that lists all the subclasses.
public sealed class X
permits X.Y, W, Z, P.Q {
final class Y extends X {}
}
final class W extends X {}
$ cat com/acme/Z.java
package com.acme;
final class Z extends X {
}
$ cat com/acme/P.java
package com.acme;
class P {
final class Q extends X {}
}
$ javac com/acme/*.java
is fine. Am I missing something?
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Mon, 21 Mar 2022 04:04:45 GMT, Athijegannathan Sundararajan
wrote:
> "DelegatingMethodHandle is non-sealed rather than sealed since it has two
> subclasses, one defined as a nested class and only a separate type in the
> same package. The permits clause does not allow listed those two kinds of
> subclasses."
>
> We can have a permits clause that lists fully qualified class name for the
> nested class and simple name for that separate type in the same package,
> right?
>
> Also
>
> $ cd $jdk_src/src/java.base/share/classes/java/lang/invoke $ grep "extends
> DelegatingMethodHandle" * MethodHandleImpl.java: private static final class
> AsVarargsCollector extends DelegatingMethodHandle { MethodHandleImpl.java:
> static class CountingWrapper extends DelegatingMethodHandle {
> MethodHandleImpl.java: private static final class WrappedMember extends
> DelegatingMethodHandle { MethodHandleImpl.java: static final class
> IntrinsicMethodHandle extends DelegatingMethodHandle
>
> All 4 are nested classes. Two of those subclasses are nested "private" in
> another class and hence cannot be referred from DelegatingMethodHandle's
> permits clause, right?
I did try to name the nested type in the permits clause, and it was not
accepted by javac. (If all the subclasses are in the same compilation unit,
either as auxiliary classes or nested classes, an explicit permits clause can
be skipped as the compiler will infer it.)
Per your other point about the nested classes in MethodHandleImpl, JLS 8.1.6
states:
"Every TypeName [in the permits clause] must name an accessible class ([§6.6]),
or a compile-time error occurs."
so the private classes of another class would not be accessible either.
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote:
> Small refactoring to use sealed classes in the MethodHandle implementation
> hierarchy.
>
> DelegatingMethodHandle is non-sealed rather than sealed since it has two
> subclasses, one defined as a nested class and only a separate type in the
> same package. The permits clause does not allow listed those two kinds of
> subclasses.
>
> Please also review the corresponding CSR
> https://bugs.openjdk.java.net/browse/JDK-8283434
"DelegatingMethodHandle is non-sealed rather than sealed since it has two
subclasses, one defined as a nested class and only a separate type in the same
package. The permits clause does not allow listed those two kinds of
subclasses."
We can have a permits clause that lists fully qualified class name for the
nested class and simple name for that separate type in the same package, right?
Also
$ cd $jdk_src/src/java.base/share/classes/java/lang/invoke
$ grep "extends DelegatingMethodHandle" *
MethodHandleImpl.java:private static final class AsVarargsCollector extends
DelegatingMethodHandle {
MethodHandleImpl.java:static class CountingWrapper extends
DelegatingMethodHandle {
MethodHandleImpl.java:private static final class WrappedMember extends
DelegatingMethodHandle {
MethodHandleImpl.java:static final class IntrinsicMethodHandle extends
DelegatingMethodHandle
All 4 are nested classes. Two of those subclasses are nested "private" in
another class and hence cannot be referred from DelegatingMethodHandle's
permits clause, right?
-
PR: https://git.openjdk.java.net/jdk/pull/7881
RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
Small refactoring to use sealed classes in the MethodHandle implementation hierarchy. DelegatingMethodHandle is non-sealed rather than sealed since it has two subclasses, one defined as a nested class and only a separate type in the same package. The permits clause does not allow listed those two kinds of subclasses. Please also review the corresponding CSR https://bugs.openjdk.java.net/browse/JDK-8283434 - Commit messages: - JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes Changes: https://git.openjdk.java.net/jdk/pull/7881/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7881=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283416 Stats: 16 lines in 5 files changed: 2 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7881.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7881/head:pull/7881 PR: https://git.openjdk.java.net/jdk/pull/7881
