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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: code-reflection
Choose a base branch
from
Open

Make Quotable a marker interface #302

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e552d41
Replace opField with opMethod for lambdas.
mabbay Dec 25, 2024
4001413
Make Quotable a marker interface
mabbay Dec 31, 2024
79ed71a
Simplify interpretation LambdaOp
mabbay Jan 8, 2025
499531b
Merge branch 'code-reflection' into Quotable-no-methods
mabbay Jan 8, 2025
f5b541b
Add missing javadoc
mabbay Jan 8, 2025
487d2f5
Fix merge
mabbay Jan 8, 2025
a027bb7
Revert to defining an interface for Quotable lambda in the interpreter
mabbay Jan 10, 2025
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
18 changes: 8 additions & 10 deletions src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@
final boolean isSerializable; // Should the returned instance be serializable
final Class<?>[] altInterfaces; // Additional interfaces to be implemented
final MethodType[] altMethods; // Signatures of additional methods to bridge
final MethodHandle quotableOpField; // A getter method handle that is used to retrieve the
// string representation of the quotable lambda's associated
// intermediate representation (can be null).
final MethodHandleInfo quotableOpFieldInfo; // Info about the quotable getter method handle (can be null).
final MethodHandle quotableOpGetter; // A getter method handle that is used to retrieve the
// the quotable lambda's associated intermediate representation (can be null).
final MethodHandleInfo quotableOpGetterInfo; // Info about the quotable getter method handle (can be null).

/**
* Meta-factory constructor.
Expand Down Expand Up @@ -187,7 +186,7 @@
this.isSerializable = isSerializable;
this.altInterfaces = altInterfaces;
this.altMethods = altMethods;
this.quotableOpField = reflectiveField;
this.quotableOpGetter = reflectiveField;

if (interfaceMethodName.isEmpty() ||
interfaceMethodName.indexOf('.') >= 0 ||
Expand Down Expand Up @@ -217,16 +216,15 @@

if (reflectiveField != null) {
try {
quotableOpFieldInfo = caller.revealDirect(reflectiveField); // may throw SecurityException
quotableOpGetterInfo = caller.revealDirect(reflectiveField); // may throw SecurityException
} catch (IllegalArgumentException e) {
throw new LambdaConversionException(implementation + " is not direct or cannot be cracked");
}
if (quotableOpFieldInfo.getReferenceKind() != REF_getField &&
quotableOpFieldInfo.getReferenceKind() != REF_getStatic) {
throw new LambdaConversionException(String.format("Unsupported MethodHandle kind: %s", quotableOpFieldInfo));
if (quotableOpGetterInfo.getReferenceKind() != REF_invokeStatic) {
throw new LambdaConversionException(String.format("Unsupported MethodHandle kind: %s", quotableOpGetterInfo));
}
} else {
quotableOpFieldInfo = null;
quotableOpGetterInfo = null;
}
}

Expand Down
34 changes: 15 additions & 19 deletions src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
View file
Open in desktop
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 @@ -336,7 +330,7 @@ public void accept(ClassBuilder clb) {
}

// if quotable, generate the field that will hold the value of quoted
if (quotableOpField != null) {
if (quotableOpGetter != null) {
clb.withField(quotedInstanceFieldName, CodeReflectionSupport.CD_Quoted, ACC_PRIVATE + ACC_FINAL);
}

Expand Down Expand Up @@ -367,8 +361,8 @@ public void accept(ClassBuilder clb) {
else if (finalAccidentallySerializable)
generateSerializationHostileMethods(clb);

if (quotableOpField != null) {
generateQuotableMethod(clb);
if (quotableOpGetter != null) {
generateQuotedMethod(clb);
}
}
});
Expand All @@ -378,10 +372,10 @@ else if (finalAccidentallySerializable)
try {
// this class is linked at the indy callsite; so define a hidden nestmate
List<?> classdata;
if (useImplMethodHandle || quotableOpField != null) {
classdata = quotableOpField == null ?
if (useImplMethodHandle || quotableOpGetter != null) {
classdata = quotableOpGetter == null ?
List.of(implementation) :
List.of(implementation, quotableOpField, CodeReflectionSupport.HANDLE_MAKE_QUOTED);
List.of(implementation, quotableOpGetter, CodeReflectionSupport.HANDLE_MAKE_QUOTED);
} else {
classdata = null;
}
Expand Down Expand Up @@ -434,7 +428,7 @@ public void accept(CodeBuilder cob) {
cob.loadLocal(TypeKind.from(argType), cob.parameterSlot(i));
cob.putfield(pool.fieldRefEntry(lambdaClassEntry, pool.nameAndTypeEntry(argNames[i], argDescs[i])));
}
if (quotableOpField != null) {
if (quotableOpGetter != null) {
generateQuotedFieldInitializer(cob);
}
cob.return_();
Expand All @@ -451,8 +445,8 @@ private void generateQuotedFieldInitializer(CodeBuilder cob) {
.ldc(cp.constantDynamicEntry(cp.bsmEntry(bsmDataAt, List.of(cp.intEntry(2))), natMH))
// load op string from field
.ldc(cp.constantDynamicEntry(cp.bsmEntry(bsmDataAt, List.of(cp.intEntry(1))), natMH));
MethodType mtype = quotableOpFieldInfo.getMethodType();
if (quotableOpFieldInfo.getReferenceKind() != MethodHandleInfo.REF_getStatic) {
MethodType mtype = quotableOpGetterInfo.getMethodType();
if (quotableOpGetterInfo.getReferenceKind() != MethodHandleInfo.REF_invokeStatic) {
mtype = mtype.insertParameterTypes(0, implClass);
}
cob.invokevirtual(CD_MethodHandle, "invokeExact", mtype.describeConstable().get());
Expand Down Expand Up @@ -492,8 +486,10 @@ static class CodeReflectionSupport {
.loadClass("jdk.incubator.code.Quotable");
Class<?> quotedHelper = layer.findLoader("jdk.incubator.code")
.loadClass("jdk.incubator.code.internal.QuotedHelper");
Class<?> funcOp = layer.findLoader("jdk.incubator.code")
.loadClass("jdk.incubator.code.op.CoreOp$FuncOp");
MethodHandle makeQuoted = Lookup.IMPL_LOOKUP.findStatic(quotedHelper, "makeQuoted",
MethodType.methodType(QUOTED_CLASS, MethodHandles.Lookup.class, String.class, Object[].class));
MethodType.methodType(QUOTED_CLASS, MethodHandles.Lookup.class, funcOp, Object[].class));
HANDLE_MAKE_QUOTED = makeQuoted.bindTo(Lookup.IMPL_LOOKUP);
} catch (Throwable ex) {
throw new ExceptionInInitializerError(ex);
Expand Down Expand Up @@ -578,9 +574,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
7 changes: 4 additions & 3 deletions src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,8 @@ public static CallSite altMetafactory(MethodHandles.Lookup caller,
int flags = extractArg(args, argIndex++, Integer.class);
Class<?>[] altInterfaces = EMPTY_CLASS_ARRAY;
MethodType[] altMethods = EMPTY_MT_ARRAY;
MethodHandle quotableField = null;
// Getter that returns the op of a Quotable instance
MethodHandle quotableOpGetter = null;
if ((flags & FLAG_MARKERS) != 0) {
int altInterfaceCount = extractArg(args, argIndex++, Integer.class);
if (altInterfaceCount < 0) {
Expand All @@ -522,7 +523,7 @@ public static CallSite altMetafactory(MethodHandles.Lookup caller,
}
}
if ((flags & FLAG_QUOTABLE) != 0) {
quotableField = extractArg(args, argIndex++, MethodHandle.class);
quotableOpGetter = extractArg(args, argIndex++, MethodHandle.class);
altInterfaces = Arrays.copyOf(altInterfaces, altInterfaces.length + 1);
altInterfaces[altInterfaces.length-1] = InnerClassLambdaMetafactory.CodeReflectionSupport.QUOTABLE_CLASS;
}
Expand Down Expand Up @@ -551,7 +552,7 @@ public static CallSite altMetafactory(MethodHandles.Lookup caller,
isSerializable,
altInterfaces,
altMethods,
quotableField);
quotableOpGetter);
mf.validateMetafactoryArgs();
return mf.buildCallSite();
}
Expand Down
4 changes: 2 additions & 2 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,8 +879,8 @@ private JCExpression makeMetafactoryIndyCall(JCFunctionalExpression tree,
}
}
if (isQuotable) {
VarSymbol reflectField = (VarSymbol)tree.codeModel;
staticArgs = staticArgs.append(reflectField.asMethodHandle(true));
MethodSymbol opMethodSym = (MethodSymbol)tree.codeModel;
staticArgs = staticArgs.append(opMethodSym.asHandle());
}
if (isSerializable) {
int prevPos = make.pos;
Expand Down
19 changes: 18 additions & 1 deletion src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
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.nio.charset.StandardCharsets;
import java.util.*;
Expand Down Expand Up @@ -472,6 +473,22 @@ public String toText() {
}


public static Optional<Quoted> ofQuotable(Quotable q) {
mabbay marked this conversation as resolved.
Show resolved Hide resolved
Method method;
Copy link
Collaborator

@mcimadamore mcimadamore Jan 8, 2025 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 = q.getClass().getMethod("quoted");
} catch (NoSuchMethodException e) {
throw new RuntimeException(e);
}
method.setAccessible(true);
Quoted quoted;
try {
quoted = (Quoted) method.invoke(q);
} catch (InvocationTargetException | IllegalAccessException e) {
throw new RuntimeException(e);
}
return Optional.of(quoted);
}

/**
* Returns the code model of the method body, if present.
Expand All @@ -494,7 +511,7 @@ private static Optional<FuncOp> createCodeModel(Method method) {
case '.', ';', '[', '/': sig[i] = '$';
}
}
String opMethodName = "method$op$" + new String(sig);
String opMethodName = "op$" + new String(sig);
Method opMethod;
try {
// @@@ Use method handle with full power mode
Expand Down
5 changes: 1 addition & 4 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/Quotable.java
View file
Open in desktop
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();
}
}
21 changes: 14 additions & 7 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/bytecode/BytecodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import jdk.incubator.code.Value;
import jdk.incubator.code.op.CoreOp;
import jdk.incubator.code.op.CoreOp.*;
import jdk.incubator.code.parser.OpParser;
import jdk.incubator.code.type.ArrayType;
import jdk.incubator.code.type.FieldRef;
import jdk.incubator.code.type.FunctionType;
Expand Down Expand Up @@ -163,9 +164,15 @@ public static <O extends Op & Op.Invokable> byte[] generateClassData(MethodHandl
for (int i = 0; i < lambdaSink.size(); i++) {
LambdaOp lop = lambdaSink.get(i);
if (quotable.get(i)) {
clb.withField("lambda$" + i + "$op", CD_String, fb -> fb
.withFlags(ClassFile.ACC_STATIC)
.with(ConstantValueAttribute.of(quote(lop).toText())));
// return (FuncOp) OpParser.fromOpString(opText)
clb.withMethod("op$lambda$" + i, MethodTypeDesc.of(FuncOp.class.describeConstable().get()),
ClassFile.ACC_PUBLIC | ClassFile.ACC_STATIC | ClassFile.ACC_SYNTHETIC, mb -> mb.withCode(cb -> cb
.loadConstant(quote(lop).toText())
.invoke(Opcode.INVOKESTATIC, OpParser.class.describeConstable().get(),
"fromStringOfFuncOp",
MethodTypeDesc.of(Op.class.describeConstable().get(), CD_String), false)
.checkcast(FuncOp.class.describeConstable().get())
.areturn()));
}
generateMethod(lookup, className, "lambda$" + i, lop, clb, lambdaSink, quotable);
}
Expand Down Expand Up @@ -891,10 +898,10 @@ private void generate() {
mtd.insertParameterTypes(0, captureTypes)),
mtd,
LambdaMetafactory.FLAG_QUOTABLE,
MethodHandleDesc.ofField(DirectMethodHandleDesc.Kind.STATIC_GETTER,
className,
"lambda$" + lambdaIndex + "$op",
CD_String)));
MethodHandleDesc.ofMethod(DirectMethodHandleDesc.Kind.STATIC,
className,
"op$lambda$" + lambdaIndex,
MethodTypeDesc.of(FuncOp.class.describeConstable().get()))));
quotable.set(lambdaSink.size());
} else {
cob.invokedynamic(DynamicCallSiteDesc.of(
Expand Down
3 changes: 3 additions & 0 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/QuotedHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,7 @@ public static Quoted makeQuoted(MethodHandles.Lookup lookup, String opText, Obje
FuncOp op = (FuncOp)OpParser.fromStringOfFuncOp(opText);
return (Quoted)Interpreter.invoke(lookup, op, args);
}
public static Quoted makeQuoted(MethodHandles.Lookup lookup, FuncOp op, Object[] args) {
return (Quoted)Interpreter.invoke(lookup, op, args);
}
mabbay marked this conversation as resolved.
Show resolved Hide resolved
}
Loading