Add isQuotable attribute to LambdaOp

[mabbay]

In the interpreter and ByteCodeGenerator we detect if a lambda is quotable based on its functional interface. This approach will not work if intersection type is used e.g. Runnable r = (Runnable & Quotable) () -> {}; This PR adds a flag to LambdaOp that will be set by the javac for quotable lambdas.

---

Progress

• Change must not contain extraneous whitespace

Reviewers

• Paul Sandoz (@PaulSandoz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/545/head:pull/545
$ git checkout pull/545

Update a local copy of the PR:
$ git checkout pull/545
$ git pull https://git.openjdk.org/babylon.git pull/545/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 545

View PR using the GUI difftool:
$ git pr show -t 545

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/545.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mabbay! A progress list of the required criteria for merging this PR into code-reflection will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@mabbay This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Add isQuotable attribute to LambdaOp

Reviewed-by: psandoz

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the code-reflection branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the code-reflection branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 06: Full - Incremental (19a3a2ad)
• 05: Full - Incremental (1fa78060)
• 04: Full - Incremental (5f5dfe00)
• 03: Full - Incremental (3259b02a)
• 02: Full - Incremental (3a4da8a4)
• 01: Full - Incremental (29a62de1)
• 00: Full (12eeef41)

[PaulSandoz]

Can we use the boolean primitive type?

[mabbay]

It's a preview feature

[PaulSandoz]

We should be able to use preview features as the jdk.incubator.code module is annotated with @ParticipatesInPreview, unless there is some odd restriction when javac calls code in the incubating module?

[forax]

Paul, there is no real difference at runtime betwen case Boolean and case boolean when you switch on an Object, case boolean just bloat the bytecode a little more.

[PaulSandoz]

I was thinking more subjectively, more clearly indicating the pattern variable cannot be null (i realize the case null, default is also present).

[forax]

Conceptually, a case Foo is like an instanceof Foo, it does not match null.

[liach]

I recall @ParticipatesInPreview only works for preview APIs instead of language features that ClassFile API needed custom backdoors from the JDK. See https://bugs.openjdk.org/browse/JDK-8308093.

[mcimadamore]

The change in this PR looks good. But, I'd like to remind ourselves that we have probably several issues when it comes to dealing with lambda expressions. There are a bunch of reasons where javac wants to use the altMetafactory instead of the regular one:

1. when the target of the lambda has one or more "empty markers" -- e.g. (Runnable & Cloneable) () -> ...
2. when the target of the lambda is serializable -- e.g. `(Runnable & Serializable) () -> ...
3. when the metafactory needs to also implement one or more "bridge" methods

An example of (3) is as follows:

interface A<X> {
    void m(X x);
}

interface B<X extends Number> {
    void m(X x);
}

interface C extends A<Integer>, B<Integer> { }

C c = (i) -> System.out.println(i);

In the above example javac will ask the altMetafactory to generate an additional bridge implementation for the (Ljava/lang/Object;)V signature.

While there's things we can do to detect (1) and (2), I think the kind of analysis required for (3) is rather difficult, and compile-dependent:

https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java#L927

And I'm quite skeptical we can reproduce this at runtime (as the runtime capabilities to detect overriding based on source-level types are rather limited).

So, more generally, I think both Interpreter and BytecodeGenerator might have some issues in this area.

[mcimadamore]

So, more generally, I think both Interpreter and BytecodeGenerator might have some issues in this area.

I tried something like this:

class Test {
    public interface A<X> {
        void m(X x);
    }

    public interface B<X extends Number> {
        void m(X x);
    }

    public interface C extends A<Integer>, B<Integer> { }

    @CodeReflection
    static C getC() {
        C c = i -> System.out.println(i);
        return c;
    }

    public static void main(String[] args) throws Throwable {
        var method = Test.class.getDeclaredMethod("getC");
        var op = (CoreOp.FuncOp) Op.ofMethod(method).get();
        A<?> a = (A<?>)Interpreter.invoke(MethodHandles.lookup(), op);
        a.m(null);
    }
}

This (surprisingly) seems to work. I believe the reason is that Interpreter uses MethodHandleProxies::asInterfaceInstance -- whose javadoc says:

* Because of the possibility of {@linkplain java.lang.reflect.Method#isBridge bridge methods}
     * and other corner cases, the interface may also have several abstract methods
     * with the same name but having distinct descriptors (types of returns and parameters).
     * In this case, all the methods are bound in common to the one given target.
     * The type check and effective {@code asType} conversion is applied to each
     * method type descriptor, and all abstract methods are bound to the target in common.
     * Beyond this type check, no further checks are made to determine that the
     * abstract methods are related in any way.

So, this special "fallback overrides" end up saving the day in this case.

However, a similar example using BytecodeGenerator fails:

class Test {
    public interface A<X> {
        void m(X x);
    }

    public interface B<X extends Number> {
        void m(X x);
    }

    public interface C extends A<Integer>, B<Integer> { }

    @CodeReflection
    static void run() {
        C c = i -> System.out.println(i);
        A<?> a = (A<?>)c;
        a.m(null);
    }

    public static void main(String[] args) throws Throwable {
        var method = Test.class.getDeclaredMethod("run");
        var op = (CoreOp.FuncOp) Op.ofMethod(method).get();
        var handle = BytecodeGenerator.generate(MethodHandles.lookup(), op);
        handle.invokeExact();
    }
}

The error I see is:

WARNING: Using incubator modules: jdk.incubator.code
Exception in thread "main" java.lang.AbstractMethodError: Receiver class run_0x0000000097069000$$Lambda/0x000000009706dbb0 does not define or inherit an implementation of the resolved method 'abstract void m(java.lang.Object)' of interface Test$A.
	at Test.main(Test.java:33)

Which is kind of expected: the generated instance is missing a bridge (as it's generated via regular metafactory).

[PaulSandoz]

@mcimadamore good points. Complete reverse engineering is hard. I also suspected that the bytecode generator does not produce correct bytecode behaviour for these cases, and i don't know if there is sufficient information held by the lambda op instance in these cases to generate the bytecode. Fixing both the generator and lifter together is important.

[PaulSandoz]

In a follow on PR we could adjust the lifter to throw a friendlier error for flags related to serialization and bridges.

[mcimadamore]

@mcimadamore good points. Complete reverse engineering is hard. I also suspected that the bytecode generator does not produce correct bytecode behaviour for these cases, and i don't know if there is sufficient information held by the lambda op instance in these cases to generate the bytecode. Fixing both the generator and lifter together is important.

So... one thing I was positively surprised about was that, even after some extensive poking, MethodHandleProxies is indeed more resilient than it seems. It basically overrides any abstract methods it finds in superclasses.

To me, this suggests that we might be able to do something similar in BytecodeGenerator and just pass all the abstract signatures to the alternate metafactory so that they will be bridged. One complication is that when generating bytecode we might not have a .class, so there might not be a way for us to answer that question. But... if we can resolve the target interface to a j,l.r.Type then we can do the same analysis that MethodHandleProxies does, and call it a day.

With respect to the other issues I enumerated, my feeling is that what's missing from LambdaOp is that it should provide a "list" of all target types it wants to conform to. Not just one. This would allow you quickly to detect whether the lambda is serializable, quotable, or implementing marker interfaces, etc. So effectively you can implement the full spectrum of lambda expressions via code models -- perhaps with some minor incompatibility that nobody will care about.

[mabbay]

/integrate

[openjdk]

Going to push as commit d276cba.

[openjdk]

@mabbay Pushed as commit d276cba.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.