Make Quotable a marker interface #302

mabbay · 2024-12-31T10:37:05Z

In this PR, we make Quotable a marker interface. This PR is based on #301.

Progress

Change must not contain extraneous whitespace

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 302

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-31T10:37:48Z

👋 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 · 2024-12-31T10:38:47Z

@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:

Make Quotable a marker interface

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 1 new commit pushed to the code-reflection branch:

fec8903: Re organized code builder hierarchy

Please see this link for an up-to-date comparison between the source branch of this pull request and the code-reflection branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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 · 2024-12-31T10:42:01Z

Webrevs

src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java

mcimadamore · 2025-01-06T14:04:13Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/interpreter/Interpreter.java

            if (Quotable.class.isAssignableFrom(fi)) {
-                return Proxy.newProxyInstance(l.lookupClass().getClassLoader(), new Class<?>[]{fi},
+                // Op.ofQuotable(Quotable q) expect q's class to have the method: Quoted quoted()


This logic seems a bit expensive. I wonder if we could have a non-exported interface which extends Quoted and adds the quoted() method, which then the implementation knows about, but the outside world can't access. That would probably lead to cleaner code, here and elsewhere (e.g. ofQuotable could be implemented w/o using reflection).

I am not very concerned about performance in the Interpreter, but i am all for reducing the complexity!
Having an internal interface will reduce the complexity.

I am not very concerned about performance in the Interpreter, but i am all for reducing the complexity! Having an internal interface will reduce the complexity.

I think the internal interface will also improve performance of the new Op::ofQuotable (which currently uses reflection to discover the magic method it knows to be there)

openjdk · 2025-01-08T11:34:40Z

@mabbay this pull request can not be integrated into code-reflection due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout Quotable-no-methods
git fetch https://git.openjdk.org/babylon.git code-reflection
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge code-reflection"
git push

# Conflicts: # src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java # src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

mcimadamore · 2025-01-08T16:23:06Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java

+        if (q instanceof Quotable2 q2) {
+            return Optional.of(q2.quoted());
+        }
+        Method method;


I believe this part is not required? E.g. we can only support cases where the instance implements Quotable2. (meaning we know that implementation :-) )

The LambdaMetaFactory needs updating so the FI implementation also implements the internal Quoted2. I don't know if there are any access control issues with the non-exported interface being defined outside of java.base. If so we might need an internal interface in java.base whose quoted method returns Object, and is exported to the code module.

FI implementation can't access Quotable2 because it's not exported.

Right, so i think we need to create an interface in java.base in a package exported to jdk.incubator.code e.g., create a package in java.base called jdk.internal.code and add:

// Implementations of this interface also implement jdk.incubator.code.Quotable interface QuotableWithQuotedAccess { // or whatever we call it // Implementations return instances of jdk.incubator.code.Quoted Object quoted(); }

then modify module-info.java in java.base to export package jdk.internal.code to module jdk.incubator.code.

More generally either this or the current approach will fail if the quotable functional interface is explicitly proxied via Proxy.newProxyInstance or MethodHandleProxies.asInterfaceInstance. And, regardless if we have Quotable::quoted proxying would fail for instances of the following:

Runnable r = (Runnable & Quotable) () -> { ... };

I think we have to specify that such proxying results in inaccessible models. Not great a great answer, but not terrible.

There's actually three modules:

the module in which capture occurs (e.g. where the quotable lambda is)

java.base which is where the lambda metafactory is

jdk.incubator.code which is where the code reflection code is

I'm not sure if the problem @mabbay is describing has to do with Quotable2 not being accessible from (1) or (2). Putting the new interface in java.base will obviously address accessibility from (2). But I think there's still issues with respect to (1) ?

E.g. InnerClassLambdaMetafactory, at the end of the day, will call defineClass on the caller lookup (e.g. the one in (1)). If the class being defined contains a symbolic reference to a non-exported interface, wouldn't that be an issue, regardless of whether the non-exported interface is in java.base or jdk.incubator.code ?

Ah yes, that will likely throw an IllegalAccessError. (I seem to recall we ran into this before when trying to add internal marker interfaces to FI implementations?)

@mcimadamore The issue I'm describing is that the class defined by LambdaMetaFactory will be in unnamed module and can't access to Quotable2.

@mcimadamore The issue I'm describing is that the class defined by LambdaMetaFactory will be in unnamed module and can't access to Quotable2.

Thanks for the clarification - I suspected that. In that case, I think it is probably best to leave this Quotable2 idea on the side for the time being. Sorry for having mentioned it -- I did not think through the full implications for the metafactory.

We can probably try to revisit this at a separate point: the magic method we try to detect works ok, but I see some risk - e.g. if the client provided its own implementation of Quotable that also exposed its own quoted method. Then the JDK code will try to call that which is why I was reaching for something more robust. But we can separate that concern out of this work.

mcimadamore · 2025-01-08T16:24:13Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/Quotable2.java

+import jdk.incubator.code.Quotable;
+import jdk.incubator.code.Quoted;
+
+public interface Quotable2 extends Quotable {


A better name would be preferrable. Maybe AccessibleQuotable ?

Yes, something more descriptive.

src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/QuotedHelper.java

PaulSandoz · 2025-01-11T00:20:08Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/interpreter/Interpreter.java

+                // Op.ofQuotable(Quotable q) expect q's class to have the method: Quoted quoted()
+                // that's why we define an interface that contains the method, so that proxy class has it
+                // and the code of Op.ofQuotable works
+                byte[] bytes = ClassFile.of().build(ClassDesc.of("I" + System.nanoTime()), classBuilder -> {


This can potentially generate many many classes that in effect are thrown away but not GC'ed. We should avoid that. Before defining the class we can check if the class is already defined by the lookup class's class loader, and if not define it using the lookup. Ideally we would define a hidden class, but then we cannot query via the class loader if it is defined and would need an alternate tracking mechanism e.g., weak hash map of class loader to class. Regardless this needs to be thread safe. The class name can be "I + [system identify hash code of the class loader]" (perhaps choose a prefix that is not possible to express in source).

There is an alternative simpler compromise that might work (if the accessibility gods are in our favor) where the proxy logic bleeds into the Op:ofQuotable method. Proxy provides methods to check if a class is a proxy class and if so it is possible to obtain the InvocationHandler instance for a proxy whose class is the proxy class. This is currently a lambda expression but we could instead make it an anon inner class and implement the required method.

In Op:ofMethod we would do:

Object oq = q; if (Proxy.isProxyClass(oq.getClass())) { oq = Proxy.getInvocationHandler(oq); } try { method = oq.getClass().getMethod("quoted"); } catch (NoSuchMethodException e) { throw new RuntimeException(e); } ...

It's not as terrible as i initially thought. If the quotable instance is a proxy we instead operate on the proxy's invocation handler.

mabbay added 2 commits December 25, 2024 11:23

Replace opField with opMethod for lambdas.

e552d41

Make Quotable a marker interface

4001413

mabbay requested review from PaulSandoz and mcimadamore December 31, 2024 10:37

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 31, 2024

mcimadamore reviewed Jan 6, 2025

View reviewed changes

src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java Show resolved Hide resolved

mcimadamore reviewed Jan 6, 2025

View reviewed changes

Simplify interpretation LambdaOp

79ed71a

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jan 8, 2025

Merge branch 'code-reflection' into Quotable-no-methods

499531b

# Conflicts: # src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java # src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 8, 2025

Add missing javadoc

f5b541b

mcimadamore reviewed Jan 8, 2025

View reviewed changes

PaulSandoz reviewed Jan 8, 2025

View reviewed changes

src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/QuotedHelper.java Outdated Show resolved Hide resolved

mabbay added 2 commits January 8, 2025 22:35

Fix merge

487d2f5

Revert to defining an interface for Quotable lambda in the interpreter

a027bb7

PaulSandoz reviewed Jan 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Quotable a marker interface #302

Make Quotable a marker interface #302

mabbay commented Dec 31, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 31, 2024

openjdk bot commented Dec 31, 2024 •

edited

Loading

mlbridge bot commented Dec 31, 2024 •

edited

Loading

mcimadamore Jan 6, 2025 •

edited

Loading

PaulSandoz Jan 6, 2025

mcimadamore Jan 7, 2025

openjdk bot commented Jan 8, 2025

mcimadamore Jan 8, 2025 •

edited

Loading

PaulSandoz Jan 8, 2025 •

edited

Loading

mabbay Jan 8, 2025

PaulSandoz Jan 8, 2025

mcimadamore Jan 8, 2025

PaulSandoz Jan 8, 2025

mabbay Jan 9, 2025 •

edited

Loading

mcimadamore Jan 9, 2025

mcimadamore Jan 8, 2025

PaulSandoz Jan 8, 2025

PaulSandoz Jan 11, 2025 •

edited

Loading

Make Quotable a marker interface #302

Are you sure you want to change the base?

Make Quotable a marker interface #302

Conversation

mabbay commented Dec 31, 2024 • edited by openjdk bot Loading

Progress

Reviewing

bridgekeeper bot commented Dec 31, 2024

openjdk bot commented Dec 31, 2024 • edited Loading

mlbridge bot commented Dec 31, 2024 • edited Loading

Webrevs

mcimadamore Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 8, 2025

mcimadamore Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

PaulSandoz Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabbay Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulSandoz Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

mabbay commented Dec 31, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 31, 2024 •

edited

Loading

mlbridge bot commented Dec 31, 2024 •

edited

Loading

mcimadamore Jan 6, 2025 •

edited

Loading

mcimadamore Jan 8, 2025 •

edited

Loading

PaulSandoz Jan 8, 2025 •

edited

Loading

mabbay Jan 9, 2025 •

edited

Loading

PaulSandoz Jan 11, 2025 •

edited

Loading