Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > > Co-authored-by: Mandy Chung HiddenTest is not a hidden class in this test. So it should be fine. The bottom line is to ensure that the type reference is not a hidden class. - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083352956
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > > Co-authored-by: Mandy Chung `this::toString` references the hidden class by name [1] which fails to be resolved. The code needs to ensure that hidden class's name is not referenced for example by casting to a supertype whose is a normal class: HiddenTest o = this; Supplier s = o::toString; [1] https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/Class.html#hiddenClasses - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083336824
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > > Co-authored-by: Mandy Chung The code snippet probably doesn't work here assuming `HiddenTest` is the Hidden class itself, you might have meant `Object o = this;` - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083342710
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Mon, 29 Apr 2024 17:09:24 GMT, ExE Boss wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update
>> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
>>
>> Co-authored-by: Mandy Chung
>
> This doesn’t fix the case where the lambda is created from an instance
> method, e.g.:
>
>
> import java.util.function.Supplier;
>
> public class HiddenLambda implements HiddenTest {
> public String test() {
> Supplier s = this::toString;
> return s.get();
> }
>
> public String toString() {
> return getClass().getName();
> }
> }
@ExE-Boss This patch intentionally does not support the said case. Reason being
that an instance method will require a hidden class receiver, yet this receiver
cannot be represented as part of a method type from the constant pool; so even
if you grab the MH from condy, you still cannot `invokeExact` in bytecode, but
have to perform an `asType` conversion first (or just use plain `invoke`)
-
PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083259410
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Mon, 29 Apr 2024 17:09:24 GMT, ExE Boss wrote: > This doesn’t fix the case where the lambda is created from an instance > method, e.g.: > Yes, unfortunately the fix is limited to invokeStatic (see the comments above). - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083253380
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
On Thu, 18 Apr 2024 05:54:17 GMT, Adam Sotona wrote:
>> Current implementation of `LambdaMetafactory` does not allow to use lambdas
>> in hidden classes. Invocation throws `NoClassDefFoundError` instead.
>>
>> This patch includes lambda implementation in a hidden class under the
>> special handling of `useImplMethodHandle`.
>> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to
>> correctly cover this test case.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
>
> Co-authored-by: Mandy Chung
This doesn’t fix the case where the lambda is created from an instance method,
e.g.:
import java.util.function.Supplier;
public class HiddenLambda implements HiddenTest {
public String test() {
Supplier s = this::toString;
return s.get();
}
public String toString() {
return getClass().getName();
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2083240740
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v4]
> Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/18810/files - new: https://git.openjdk.org/jdk/pull/18810/files/e8eb1752..091ee2af Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18810=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18810=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18810.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18810/head:pull/18810 PR: https://git.openjdk.org/jdk/pull/18810
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v3]
On Wed, 17 Apr 2024 16:08:31 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - typo in comment fix > - applied suggested changes Looks fine to me. A hidden class cannot be resolved by name. I tweaked the comment to make it clear. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 182: > 180: // live implementation method handle to the proxy class to invoke > 181: // directly. (javac prefers to avoid this situation by generating > 182: // bridges in the target class) Suggestion: // lambda class has no access to the resolved method, or does // 'invokestatic' on a hidden class which cannot be resolved by name. // Instead, we need to pass the live implementation method handle to // the proxy class to invoke directly. (javac prefers to avoid this // situation by generating bridges in the target class) - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2007006920 PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1569431448
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v3]
On Wed, 17 Apr 2024 16:08:31 GMT, Adam Sotona wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - typo in comment fix > - applied suggested changes Looks good to me. I conservatively bumped the review count to 2. - Marked as reviewed by psandoz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2006903761
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v2]
On Wed, 17 Apr 2024 15:39:11 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> applied suggested changes
>
> test/jdk/java/lang/invoke/defineHiddenClass/src/Lambda.java line 28:
>
>> 26: public class Lambda implements HiddenTest {
>> 27: public void test() {
>> 28: Function f = o -> o.toString();
>
> Recommend you retain the existing method reference e.g.:
>
> Function fmref = Object::toString;
> Function flexp = o -> o.toString();
> String s = fmref.apply(this) + flexp.apply(this);
> ...
>
> Or split them out into two tests (easier to debug if case fails).
I've split the tests, thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1569099751
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v3]
> Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - typo in comment fix - applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/18810/files - new: https://git.openjdk.org/jdk/pull/18810/files/c637ab90..e8eb1752 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18810=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18810=01-02 Stats: 18 lines in 3 files changed: 15 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18810.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18810/head:pull/18810 PR: https://git.openjdk.org/jdk/pull/18810
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v2]
On Wed, 17 Apr 2024 12:51:55 GMT, Adam Sotona wrote:
>> Current implementation of `LambdaMetafactory` does not allow to use lambdas
>> in hidden classes. Invocation throws `NoClassDefFoundError` instead.
>>
>> This patch includes lambda implementation in a hidden class under the
>> special handling of `useImplMethodHandle`.
>> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to
>> correctly cover this test case.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> applied suggested changes
test/jdk/java/lang/invoke/defineHiddenClass/src/Lambda.java line 28:
> 26: public class Lambda implements HiddenTest {
> 27: public void test() {
> 28: Function f = o -> o.toString();
Recommend you retain the existing method reference e.g.:
Function fmref = Object::toString;
Function flexp = o -> o.toString();
String s = fmref.apply(this) + flexp.apply(this);
...
Or split them out into two tests (easier to debug if case fails).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1569060714
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v2]
On Wed, 17 Apr 2024 10:58:59 GMT, Jorn Vernee wrote: > Do we also need to check if the parameters or return type are hidden classes? I think this is already failing and will continue to fail. Don't think we need special error messages for this case. - PR Comment: https://git.openjdk.org/jdk/pull/18810#issuecomment-2061260710
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class [v2]
> Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: applied suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/18810/files - new: https://git.openjdk.org/jdk/pull/18810/files/4a53389d..c637ab90 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18810=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18810=00-01 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18810.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18810/head:pull/18810 PR: https://git.openjdk.org/jdk/pull/18810
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class
On Wed, 17 Apr 2024 09:14:31 GMT, Chen Liang wrote: >> Current implementation of `LambdaMetafactory` does not allow to use lambdas >> in hidden classes. Invocation throws `NoClassDefFoundError` instead. >> >> This patch includes lambda implementation in a hidden class under the >> special handling of `useImplMethodHandle`. >> The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to >> correctly cover this test case. >> >> Please review. >> >> Thanks, >> Adam > > src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java > line 186: > >> 184:!VerifyAccess.isSamePackage(targetClass, >> implInfo.getDeclaringClass())) || >> 185:implKind == H_INVOKESPECIAL || >> 186:implInfo.getDeclaringClass().isHidden(); > > Might need to check the difference between `implInfo.getDeclaringClass()` and > `implClass` Yes, we should support only invoke static refs for hidden classes and `implClass == implInfo.getDeclaringClass()` in that case. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1568782761
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class
On Wed, 17 Apr 2024 08:46:59 GMT, Adam Sotona wrote: > Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam Do we also need to check if the parameters or return type are hidden classes? I suppose if we compile a direct bytecode reference to the target method, all of them need to be referenceable from bytecode, not just the holder. - PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2005733945
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class
On Wed, 17 Apr 2024 08:46:59 GMT, Adam Sotona wrote: > Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam I wonder if we should only support invokeStatic refs for hidden classes, as instance call lambdas will always require receiver which can't be represented outside of hidden classes. src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 186: > 184:!VerifyAccess.isSamePackage(targetClass, > implInfo.getDeclaringClass())) || > 185:implKind == H_INVOKESPECIAL || > 186:implInfo.getDeclaringClass().isHidden(); Might need to check the difference between `implInfo.getDeclaringClass()` and `implClass` - PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2005483864 PR Review Comment: https://git.openjdk.org/jdk/pull/18810#discussion_r1568505421
RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class
Current implementation of `LambdaMetafactory` does not allow to use lambdas in hidden classes. Invocation throws `NoClassDefFoundError` instead. This patch includes lambda implementation in a hidden class under the special handling of `useImplMethodHandle`. The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to correctly cover this test case. Please review. Thanks, Adam - Commit messages: - 8330467: NoClassDefFoundError when lambda is in a hidden class Changes: https://git.openjdk.org/jdk/pull/18810/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18810=00 Issue: https://bugs.openjdk.org/browse/JDK-8330467 Stats: 8 lines in 2 files changed: 2 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18810.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18810/head:pull/18810 PR: https://git.openjdk.org/jdk/pull/18810
