Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Quotable a marker interface #302

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,36 +34,30 @@
import java.lang.classfile.ClassBuilder;
import java.lang.classfile.ClassFile;
import java.lang.classfile.CodeBuilder;
import java.lang.classfile.FieldBuilder;
import java.lang.classfile.MethodBuilder;
import java.lang.classfile.Opcode;
import java.lang.classfile.TypeKind;
import java.lang.classfile.constantpool.MethodHandleEntry;
import java.lang.classfile.constantpool.NameAndTypeEntry;
import java.lang.constant.ClassDesc;
import java.lang.constant.DynamicConstantDesc;
import java.lang.constant.MethodTypeDesc;
import java.lang.invoke.MethodHandles.Lookup;
import java.lang.module.Configuration;
import java.lang.module.ModuleFinder;
import java.lang.reflect.Modifier;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.Consumer;

import static java.lang.classfile.ClassFile.*;
import java.lang.classfile.attribute.ExceptionsAttribute;
import java.lang.classfile.constantpool.ClassEntry;
import java.lang.classfile.constantpool.ConstantPoolBuilder;
import java.lang.classfile.constantpool.MethodRefEntry;

import static java.lang.constant.ConstantDescs.*;
import static java.lang.invoke.MethodHandleNatives.Constants.NESTMATE_CLASS;
import static java.lang.invoke.MethodHandleNatives.Constants.STRONG_LOADER_LINK;
import static java.lang.invoke.MethodHandles.Lookup.ClassOption.NESTMATE;
import static java.lang.invoke.MethodHandles.Lookup.ClassOption.STRONG;
import static java.lang.invoke.MethodType.methodType;
import jdk.internal.constant.ConstantUtils;
import jdk.internal.constant.MethodTypeDescImpl;
Expand Down Expand Up @@ -368,7 +362,7 @@ else if (finalAccidentallySerializable)
generateSerializationHostileMethods(clb);

if (quotableOpGetter != null) {
generateQuotableMethod(clb);
generateQuotedMethod(clb);
}
}
});
Expand Down Expand Up @@ -577,9 +571,9 @@ public void accept(CodeBuilder cob) {
}

/**
* Generate a writeReplace method that supports serialization
* Generate method #quoted()
*/
private void generateQuotableMethod(ClassBuilder clb) {
private void generateQuotedMethod(ClassBuilder clb) {
clb.withMethod(NAME_METHOD_QUOTED, CodeReflectionSupport.MTD_Quoted, ACC_PUBLIC + ACC_FINAL, new MethodBody(new Consumer<CodeBuilder>() {
@Override
public void accept(CodeBuilder cob) {
Expand Down
30 changes: 30 additions & 0 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@
import javax.annotation.processing.ProcessingEnvironment;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.Modifier;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.nio.charset.StandardCharsets;
import java.util.*;
import java.util.function.BiFunction;
Expand Down Expand Up @@ -472,6 +474,34 @@ public String toText() {
}


/**
* Returns the code model of the Quotable passed in.
* @param q the Quotable we want to get its code model.
* @return the code model of the Quotable passed in.
* @since 99
*/
public static Optional<Quoted> ofQuotable(Quotable q) {
mabbay marked this conversation as resolved.
Show resolved Hide resolved
Object oq = q;
if (Proxy.isProxyClass(oq.getClass())) {
oq = Proxy.getInvocationHandler(oq);
}

Method method;
Copy link
Collaborator

@mcimadamore mcimadamore Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@PaulSandoz PaulSandoz Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually three modules:

  1. the module in which capture occurs (e.g. where the quotable lambda is)
  2. java.base which is where the lambda metafactory is
  3. 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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member Author

@mabbay mabbay Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
method = oq.getClass().getMethod("quoted");
} catch (NoSuchMethodException e) {
throw new RuntimeException(e);
}
method.setAccessible(true);

Quoted quoted;
try {
quoted = (Quoted) method.invoke(oq);
} catch (InvocationTargetException | IllegalAccessException e) {
throw new RuntimeException(e);
}
return Optional.of(quoted);
}

/**
* Returns the code model of the method body, if present.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,8 @@

/**
* Classes implementing this interface support code reflection. That is, they can obtain
* a {@link Quoted} object using {@link #quoted()}, which returns the intermediate
* a {@link Quoted} object using {@link Op#ofQuotable(Quotable)}, which returns the intermediate
* representation associated with a lambda expression or method reference.
*/
public interface Quotable {
default Quoted quoted() {
throw new UnsupportedOperationException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

import java.lang.invoke.*;
import java.lang.reflect.Array;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import jdk.incubator.code.*;
import jdk.incubator.code.op.CoreOp;
Expand Down Expand Up @@ -487,16 +489,23 @@ static Object interpretOp(MethodHandles.Lookup l, OpContext oc, Op o) {
.asCollector(Object[].class, lo.parameters().size());
Object fiInstance = MethodHandleProxies.asInterfaceInstance(fi, fProxy);

// If a quotable lambda proxy again to implement Quotable
// If a quotable lambda proxy again to add method Quoted quoted()
if (Quotable.class.isAssignableFrom(fi)) {
return Proxy.newProxyInstance(l.lookupClass().getClassLoader(), new Class<?>[]{fi},
(_, method, args) -> {
if (method.getDeclaringClass() == Quotable.class) {
// Implement Quotable::quoted
return new Quoted(lo, capturedValuesAndArguments);
} else {
// Delegate to FI instance
return method.invoke(fiInstance, args);
new InvocationHandler() {
private final Quoted quoted = new Quoted(lo, capturedValuesAndArguments);
@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
if (Objects.equals(method.getName(), "quoted") && method.getParameterCount() == 0) {
return quoted();
} else {
// Delegate to FI instance
return method.invoke(fiInstance, args);
}
}

public Quoted quoted() {
return quoted;
}
});
} else {
Expand Down
2 changes: 1 addition & 1 deletion test/jdk/java/lang/reflect/code/TestBuild.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class TestBuild {
public LambdaOp f() {
IntBinaryOperator ibo = (IntBinaryOperator & Quotable) (a, b) -> a + b;
Quotable iboq = (Quotable) ibo;
return SSA.transform((LambdaOp) iboq.quoted().op());
return SSA.transform((LambdaOp) Op.ofQuotable(iboq).get().op());
}

@Test
Expand Down
8 changes: 4 additions & 4 deletions test/jdk/java/lang/reflect/code/TestLambdaOps.java
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ static int f(int i) {
@Test
public void testQuotableModel() {
Quotable quotable = (Runnable & Quotable) () -> {};
Op qop = quotable.quoted().op();
Op qop = Op.ofQuotable(quotable).get().op();
Op top = qop.ancestorBody().parentOp().ancestorBody().parentOp();
Assert.assertTrue(top instanceof CoreOp.FuncOp);

Expand All @@ -180,7 +180,7 @@ public void testQuote() {
QuotableIntSupplier op = (QuotableIntSupplier) Interpreter.invoke(MethodHandles.lookup(), g, 42);
Assert.assertEquals(op.getAsInt(), 42);

Quoted q = op.quoted();
Quoted q = Op.ofQuotable(op).get();
q.op().writeTo(System.out);
Assert.assertEquals(q.capturedValues().size(), 1);
Assert.assertEquals(((Var<?>)q.capturedValues().values().iterator().next()).value(), 42);
Expand All @@ -198,7 +198,7 @@ public void testQuote() {
QuotableIntSupplier op = quote(42);
Assert.assertEquals(op.getAsInt(), 42);

Quoted q = op.quoted();
Quoted q = Op.ofQuotable(op).get();
q.op().writeTo(System.out);
System.out.print(q.capturedValues().values());
Assert.assertEquals(q.capturedValues().size(), 1);
Expand Down Expand Up @@ -235,7 +235,7 @@ Iterator<Quotable> methodRefLambdas() {

@Test(dataProvider = "methodRefLambdas")
public void testIsMethodReference(Quotable q) {
Quoted quoted = q.quoted();
Quoted quoted = Op.ofQuotable(q).get();
CoreOp.LambdaOp lop = (CoreOp.LambdaOp) quoted.op();
Assert.assertTrue(lop.methodReference().isPresent());
}
Expand Down
6 changes: 3 additions & 3 deletions test/jdk/java/lang/reflect/code/bytecode/TestBytecode.java
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,9 @@ static int consume(int i, Func f) {
}

static int consumeQuotable(int i, QuotableFunc f) {
Assert.assertNotNull(f.quoted());
Assert.assertNotNull(f.quoted().op());
Assert.assertTrue(f.quoted().op() instanceof CoreOp.LambdaOp);
Assert.assertNotNull(Op.ofQuotable(f).get());
Assert.assertNotNull(Op.ofQuotable(f).get().op());
Assert.assertTrue(Op.ofQuotable(f).get().op() instanceof CoreOp.LambdaOp);
return f.apply(i + 1);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ static public int f(int a) {

static void test(QIntSupplier s, int a) {
@SuppressWarnings("unchecked")
CoreOp.Var<Integer> capture = (CoreOp.Var<Integer>) s.quoted().capturedValues().values().iterator().next();
CoreOp.Var<Integer> capture = (CoreOp.Var<Integer>) Op.ofQuotable(s).get().capturedValues().values().iterator().next();
Assert.assertEquals(capture.value().intValue(), a);
}

Expand Down
4 changes: 2 additions & 2 deletions test/jdk/java/lang/reflect/code/linq/Queryable.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ public interface Queryable<T> {

@SuppressWarnings("unchecked")
default Queryable<T> where(QuotablePredicate<T> f) {
LambdaOp l = (LambdaOp) f.quoted().op();
LambdaOp l = (LambdaOp) Op.ofQuotable(f).get().op();
return (Queryable<T>) insertQuery(elementType(), "where", l);
}

@SuppressWarnings("unchecked")
default <R> Queryable<R> select(QuotableFunction<T, R> f) {
LambdaOp l = (LambdaOp) f.quoted().op();
LambdaOp l = (LambdaOp) Op.ofQuotable(f).get().op();
return (Queryable<R>) insertQuery((JavaType) l.invokableType().returnType(), "select", l);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ static class StreamOp {
final LambdaOp lambdaOp;

StreamOp(Quotable quotedLambda) {
if (!(quotedLambda.quoted().op() instanceof LambdaOp lambdaOp)) {
if (!(Op.ofQuotable(quotedLambda).get().op() instanceof LambdaOp lambdaOp)) {
throw new IllegalArgumentException("Quotable operation is not lambda operation");
}
if (!(quotedLambda.quoted().capturedValues().isEmpty())) {
if (!(Op.ofQuotable(quotedLambda).get().capturedValues().isEmpty())) {
throw new IllegalArgumentException("Quotable operation captures values");
}
this.lambdaOp = lambdaOp;
Expand Down Expand Up @@ -197,10 +197,10 @@ void fuseIntermediateOperation(int i, Block.Builder body, Value element, Block.B
}

public FuncOp forEach(QuotableConsumer<T> quotableConsumer) {
if (!(quotableConsumer.quoted().op() instanceof LambdaOp consumer)) {
if (!(Op.ofQuotable(quotableConsumer).get().op() instanceof LambdaOp consumer)) {
throw new IllegalArgumentException("Quotable consumer is not lambda operation");
}
if (!(quotableConsumer.quoted().capturedValues().isEmpty())) {
if (!(Op.ofQuotable(quotableConsumer).get().capturedValues().isEmpty())) {
throw new IllegalArgumentException("Quotable consumer captures values");
}

Expand All @@ -224,16 +224,16 @@ public FuncOp forEach(QuotableConsumer<T> quotableConsumer) {
}

public <C> FuncOp collect(QuotableSupplier<C> quotableSupplier, QuotableBiConsumer<C, T> quotableAccumulator) {
if (!(quotableSupplier.quoted().op() instanceof LambdaOp supplier)) {
if (!(Op.ofQuotable(quotableSupplier).get().op() instanceof LambdaOp supplier)) {
throw new IllegalArgumentException("Quotable supplier is not lambda operation");
}
if (!(quotableSupplier.quoted().capturedValues().isEmpty())) {
if (!(Op.ofQuotable(quotableSupplier).get().capturedValues().isEmpty())) {
throw new IllegalArgumentException("Quotable supplier captures values");
}
if (!(quotableAccumulator.quoted().op() instanceof LambdaOp accumulator)) {
if (!(Op.ofQuotable(quotableAccumulator).get().op() instanceof LambdaOp accumulator)) {
throw new IllegalArgumentException("Quotable accumulator is not lambda operation");
}
if (!(quotableAccumulator.quoted().capturedValues().isEmpty())) {
if (!(Op.ofQuotable(quotableAccumulator).get().capturedValues().isEmpty())) {
throw new IllegalArgumentException("Quotable accumulator captures values");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ static void check(Field field) throws ReflectiveOperationException {
}
} else if (Quotable.class.isAssignableFrom(field.getType())) {
Quotable quotable = (Quotable) field.get(null);
String found = canonicalizeModel(field, getModelOfQuotedOp(quotable.quoted()));
String found = canonicalizeModel(field, getModelOfQuotedOp(Op.ofQuotable(quotable).get()));
String expected = canonicalizeModel(field, ir.value());
if (!found.equals(expected)) {
error("Bad IR\nFound:\n%s\n\nExpected:\n%s", found, expected);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import jdk.incubator.code.Op;
import org.testng.Assert;
import org.testng.annotations.Test;

Expand All @@ -7,7 +8,7 @@

/*
* @test
* @summary test that invoking Quotable#quoted returns the same instance
* @summary test that invoking Op#ofQuotable returns the same instance
* @modules jdk.incubator.code
* @run testng QuotedSameInstanceTest
*/
Expand All @@ -19,15 +20,15 @@ public class QuotedSameInstanceTest {

@Test
void testWithOneThread() {
Assert.assertSame(q1.quoted(), q1.quoted());
Assert.assertSame(Op.ofQuotable(q1).get(), Op.ofQuotable(q1).get());
}

interface QuotableIntUnaryOperator extends IntUnaryOperator, Quotable { }
private static final QuotableIntUnaryOperator q2 = x -> x;

@Test
void testWithMultiThreads() {
Object[] quotedObjects = IntStream.range(0, 1024).parallel().mapToObj(__ -> q2.quoted()).toArray();
Object[] quotedObjects = IntStream.range(0, 1024).parallel().mapToObj(__ -> Op.ofQuotable(q2).get()).toArray();
for (int i = 1; i < quotedObjects.length; i++) {
Assert.assertSame(quotedObjects[i], quotedObjects[i - 1]);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public class TestCaptureQuotable {
@Test(dataProvider = "ints")
public void testCaptureIntParam(int x) {
Quotable quotable = (Quotable & IntUnaryOperator)y -> x + y;
Quoted quoted = quotable.quoted();
Quoted quoted = Op.ofQuotable(quotable).get();
assertEquals(quoted.capturedValues().size(), 1);
assertEquals(((Var)quoted.capturedValues().values().iterator().next()).value(), x);
List<Object> arguments = new ArrayList<>();
Expand All @@ -67,7 +67,7 @@ public void testCaptureThisRefAndIntConstant() {
final int x = 100;
String hello = "hello";
Quotable quotable = (Quotable & ToIntFunction<Number>)y -> y.intValue() + hashCode() + hello.length() + x;
Quoted quoted = quotable.quoted();
Quoted quoted = Op.ofQuotable(quotable).get();
assertEquals(quoted.capturedValues().size(), 3);
Iterator<Object> it = quoted.capturedValues().values().iterator();
assertEquals(it.next(), this);
Expand All @@ -94,7 +94,7 @@ public void testCaptureMany() {
int i8 = ia[7] = 7;

Quotable quotable = (Quotable & IntSupplier) () -> i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8;
Quoted quoted = quotable.quoted();
Quoted quoted = Op.ofQuotable(quotable).get();
assertEquals(quoted.capturedValues().size(), ia.length);
assertEquals(quoted.op().capturedValues(), new ArrayList<>(quoted.capturedValues().keySet()));
Iterator<Object> it = quoted.capturedValues().values().iterator();
Expand All @@ -120,7 +120,7 @@ Quotable quotable() {
}
Context context = new Context(x);
Quotable quotable = context.quotable();
Quoted quoted = quotable.quoted();
Quoted quoted = Op.ofQuotable(quotable).get();
assertEquals(quoted.capturedValues().size(), 1);
assertEquals(quoted.capturedValues().values().iterator().next(), context);
List<Object> arguments = new ArrayList<>();
Expand All @@ -142,7 +142,7 @@ public Object[][] ints() {
public void testCaptureReferenceReceiver(int i) {
int prevCount = Box.count;
Quotable quotable = (Quotable & IntUnaryOperator)new Box(i)::add;
Quoted quoted = quotable.quoted();
Quoted quoted = Op.ofQuotable(quotable).get();
assertEquals(Box.count, prevCount + 1); // no duplicate receiver computation!
assertEquals(quoted.capturedValues().size(), 1);
assertEquals(((Box)((Var)quoted.capturedValues().values().iterator().next()).value()).i, i);
Expand Down