From b2fcc9d9733ca22b0e0182830997a93164f4c1e5 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 14 Jun 2018 14:31:16 -0700 Subject: [PATCH 1/2] Painless: Fix bug for static method calls on interfaces. --- .../java/org/elasticsearch/painless/Def.java | 3 ++- .../org/elasticsearch/painless/Definition.java | 16 ++++++++++++++-- .../org/elasticsearch/painless/FunctionRef.java | 7 +++++++ .../elasticsearch/painless/LambdaBootstrap.java | 9 ++++++--- .../elasticsearch/painless/WriterConstants.java | 4 ++-- .../painless/node/ECapturingFunctionRef.java | 3 ++- .../painless/node/EFunctionRef.java | 3 ++- .../org/elasticsearch/painless/node/ELambda.java | 3 ++- .../painless/BasicExpressionTests.java | 5 +++++ 9 files changed, 42 insertions(+), 11 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java index 661af1b6c9137..9c9d612479c1a 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java @@ -376,7 +376,8 @@ private static MethodHandle lookupReferenceInternal(Definition definition, Looku ref.delegateClassName, ref.delegateInvokeType, ref.delegateMethodName, - ref.delegateMethodType + ref.delegateMethodType, + ref.isDelegateInterface ); return callSite.dynamicInvoker().asType(MethodType.methodType(clazz.clazz, captures)); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java index f97df128f15e5..91f1269044661 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java @@ -20,6 +20,7 @@ package org.elasticsearch.painless; import org.elasticsearch.painless.spi.Whitelist; +import org.objectweb.asm.Opcodes; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; @@ -202,16 +203,27 @@ public MethodType getMethodType() { public void write(MethodWriter writer) { final org.objectweb.asm.Type type; + final Class clazz; if (augmentation != null) { assert java.lang.reflect.Modifier.isStatic(modifiers); + clazz = augmentation; type = org.objectweb.asm.Type.getType(augmentation); } else { + clazz = owner.clazz; type = owner.type; } if (java.lang.reflect.Modifier.isStatic(modifiers)) { - writer.invokeStatic(type, method); - } else if (java.lang.reflect.Modifier.isInterface(owner.clazz.getModifiers())) { + // special case for interfaces where the interface function needs to be set to true + // to reference the appropriate class file when calling a static interface method + // java 8 did not check, but java 9 and 10 do + if (java.lang.reflect.Modifier.isInterface(clazz.getModifiers())) { + writer.visitMethodInsn(Opcodes.INVOKESTATIC, + type.getInternalName(), name, getMethodType().toMethodDescriptorString(), true); + } else { + writer.invokeStatic(type, method); + } + } else if (java.lang.reflect.Modifier.isInterface(clazz.getModifiers())) { writer.invokeInterface(type, method); } else { writer.invokeVirtual(type, method); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java index 66cf78e857220..06b446318bff2 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java @@ -66,6 +66,9 @@ public class FunctionRef { /** delegate method type method as type */ public final Type delegateType; + /** whether a call is made on a delegate interface */ + public final int isDelegateInterface; + /** * Creates a new FunctionRef, which will resolve {@code type::call} from the whitelist. * @param definition the whitelist against which this script is being compiled @@ -97,10 +100,13 @@ public FunctionRef(Class expected, Method interfaceMethod, Method delegateMet // the Painless$Script class can be inferred if owner is null if (delegateMethod.owner == null) { delegateClassName = CLASS_NAME; + isDelegateInterface = 0; } else if (delegateMethod.augmentation != null) { delegateClassName = delegateMethod.augmentation.getName(); + isDelegateInterface = delegateMethod.augmentation.isInterface() ? 1 : 0; } else { delegateClassName = delegateMethod.owner.clazz.getName(); + isDelegateInterface = delegateMethod.owner.clazz.isInterface() ? 1 : 0; } if ("".equals(delegateMethod.name)) { @@ -139,6 +145,7 @@ public FunctionRef(Class expected, delegateInvokeType = H_INVOKESTATIC; this.delegateMethodName = delegateMethodName; this.delegateMethodType = delegateMethodType.dropParameterTypes(0, numCaptures); + isDelegateInterface = 0; this.interfaceMethod = null; delegateMethod = null; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java index 7a2ec9da34e29..4554437ceb25c 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java @@ -188,6 +188,7 @@ private Capture(int count, Class type) { * @param delegateMethodName The name of the method to be called in the Painless script class * @param delegateMethodType The type of method call in the Painless script class without * the captured types + * @param isDelegateInterface If the method to be called is owned by an interface * @return A {@link CallSite} linked to a factory method for creating a lambda class * that implements the expected functional interface * @throws LambdaConversionException Thrown when an illegal type conversion occurs at link time @@ -200,7 +201,8 @@ public static CallSite lambdaBootstrap( String delegateClassName, int delegateInvokeType, String delegateMethodName, - MethodType delegateMethodType) + MethodType delegateMethodType, + int isDelegateInterface) throws LambdaConversionException { Loader loader = (Loader)lookup.lookupClass().getClassLoader(); String lambdaClassName = Type.getInternalName(lookup.lookupClass()) + "$$Lambda" + loader.newLambdaIdentifier(); @@ -225,7 +227,7 @@ public static CallSite lambdaBootstrap( generateInterfaceMethod(cw, factoryMethodType, lambdaClassType, interfaceMethodName, interfaceMethodType, delegateClassType, delegateInvokeType, - delegateMethodName, delegateMethodType, captures); + delegateMethodName, delegateMethodType, isDelegateInterface == 1, captures); endLambdaClass(cw); @@ -369,6 +371,7 @@ private static void generateInterfaceMethod( int delegateInvokeType, String delegateMethodName, MethodType delegateMethodType, + boolean isDelegateInterface, Capture[] captures) throws LambdaConversionException { @@ -434,7 +437,7 @@ private static void generateInterfaceMethod( Handle delegateHandle = new Handle(delegateInvokeType, delegateClassType.getInternalName(), delegateMethodName, delegateMethodType.toMethodDescriptorString(), - delegateInvokeType == H_INVOKEINTERFACE); + isDelegateInterface); iface.invokeDynamic(delegateMethodName, Type.getMethodType(interfaceMethodType .toMethodDescriptorString()).getDescriptor(), DELEGATE_BOOTSTRAP_HANDLE, delegateHandle); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java index 9150e2609b700..18d7d94492e67 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java @@ -141,8 +141,8 @@ public final class WriterConstants { /** invokedynamic bootstrap for lambda expression/method references */ public static final MethodType LAMBDA_BOOTSTRAP_TYPE = - MethodType.methodType(CallSite.class, MethodHandles.Lookup.class, String.class, - MethodType.class, MethodType.class, String.class, int.class, String.class, MethodType.class); + MethodType.methodType(CallSite.class, MethodHandles.Lookup.class, String.class, MethodType.class, + MethodType.class, String.class, int.class, String.class, MethodType.class, int.class); public static final Handle LAMBDA_BOOTSTRAP_HANDLE = new Handle(Opcodes.H_INVOKESTATIC, Type.getInternalName(LambdaBootstrap.class), "lambdaBootstrap", LAMBDA_BOOTSTRAP_TYPE.toMethodDescriptorString(), false); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java index 724679d3f8538..d18bdc639297c 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java @@ -121,7 +121,8 @@ void write(MethodWriter writer, Globals globals) { ref.delegateClassName, ref.delegateInvokeType, ref.delegateMethodName, - ref.delegateType + ref.delegateType, + ref.isDelegateInterface ); } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java index 636623004c982..61b28e7d275a6 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java @@ -112,7 +112,8 @@ void write(MethodWriter writer, Globals globals) { ref.delegateClassName, ref.delegateInvokeType, ref.delegateMethodName, - ref.delegateType + ref.delegateType, + ref.isDelegateInterface ); } else { // TODO: don't do this: its just to cutover :) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java index c37ff435f566f..2f432822cd2a3 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java @@ -222,7 +222,8 @@ void write(MethodWriter writer, Globals globals) { ref.delegateClassName, ref.delegateInvokeType, ref.delegateMethodName, - ref.delegateType + ref.delegateType, + ref.isDelegateInterface ); } else { // placeholder diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java index 97e1f01fdfc94..6ff727d987cdd 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java @@ -264,6 +264,11 @@ public void testNullSafeDeref() { // assertEquals(null, exec("def a = ['thing': 'bar']; a.other?.cat?.dog = 'wombat'; return a.other?.cat?.dog")); } + // test to ensure static interface methods are called correctly + public void testStaticInterfaceMethod() { + assertEquals(4, exec("def values = [1, 4, 3, 2]; values.sort(Comparator.comparing(p -> p)); return values[3]")); + } + private void assertMustBeNullable(String script) { Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> exec(script)); assertEquals("Result of null safe operator must be nullable", e.getMessage()); From 20e0aa1d3d2932e030aacdeacb9e620124300167 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 14 Jun 2018 15:56:14 -0700 Subject: [PATCH 2/2] Fixes based on PR comments. --- .../src/main/java/org/elasticsearch/painless/Def.java | 2 +- .../java/org/elasticsearch/painless/Definition.java | 7 ++++--- .../java/org/elasticsearch/painless/FunctionRef.java | 10 +++++----- .../org/elasticsearch/painless/LambdaBootstrap.java | 5 ++++- .../painless/node/ECapturingFunctionRef.java | 2 +- .../org/elasticsearch/painless/node/EFunctionRef.java | 2 +- .../java/org/elasticsearch/painless/node/ELambda.java | 2 +- .../org/elasticsearch/painless/FunctionRefTests.java | 5 +++++ 8 files changed, 22 insertions(+), 13 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java index 9c9d612479c1a..988a31a24ee27 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java @@ -377,7 +377,7 @@ private static MethodHandle lookupReferenceInternal(Definition definition, Looku ref.delegateInvokeType, ref.delegateMethodName, ref.delegateMethodType, - ref.isDelegateInterface + ref.isDelegateInterface ? 1 : 0 ); return callSite.dynamicInvoker().asType(MethodType.methodType(clazz.clazz, captures)); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java index 91f1269044661..75575d6f12568 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java @@ -214,9 +214,10 @@ public void write(MethodWriter writer) { } if (java.lang.reflect.Modifier.isStatic(modifiers)) { - // special case for interfaces where the interface function needs to be set to true - // to reference the appropriate class file when calling a static interface method - // java 8 did not check, but java 9 and 10 do + // invokeStatic assumes that the owner class is not an interface, so this is a + // special case for interfaces where the interface method boolean needs to be set to + // true to reference the appropriate class constant when calling a static interface + // method since java 8 did not check, but java 9 and 10 do if (java.lang.reflect.Modifier.isInterface(clazz.getModifiers())) { writer.visitMethodInsn(Opcodes.INVOKESTATIC, type.getInternalName(), name, getMethodType().toMethodDescriptorString(), true); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java index 06b446318bff2..0b698dd244192 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java @@ -67,7 +67,7 @@ public class FunctionRef { public final Type delegateType; /** whether a call is made on a delegate interface */ - public final int isDelegateInterface; + public final boolean isDelegateInterface; /** * Creates a new FunctionRef, which will resolve {@code type::call} from the whitelist. @@ -100,13 +100,13 @@ public FunctionRef(Class expected, Method interfaceMethod, Method delegateMet // the Painless$Script class can be inferred if owner is null if (delegateMethod.owner == null) { delegateClassName = CLASS_NAME; - isDelegateInterface = 0; + isDelegateInterface = false; } else if (delegateMethod.augmentation != null) { delegateClassName = delegateMethod.augmentation.getName(); - isDelegateInterface = delegateMethod.augmentation.isInterface() ? 1 : 0; + isDelegateInterface = delegateMethod.augmentation.isInterface(); } else { delegateClassName = delegateMethod.owner.clazz.getName(); - isDelegateInterface = delegateMethod.owner.clazz.isInterface() ? 1 : 0; + isDelegateInterface = delegateMethod.owner.clazz.isInterface(); } if ("".equals(delegateMethod.name)) { @@ -145,7 +145,7 @@ public FunctionRef(Class expected, delegateInvokeType = H_INVOKESTATIC; this.delegateMethodName = delegateMethodName; this.delegateMethodType = delegateMethodType.dropParameterTypes(0, numCaptures); - isDelegateInterface = 0; + isDelegateInterface = false; this.interfaceMethod = null; delegateMethod = null; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java index 4554437ceb25c..3fc8554b271e2 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java @@ -188,7 +188,10 @@ private Capture(int count, Class type) { * @param delegateMethodName The name of the method to be called in the Painless script class * @param delegateMethodType The type of method call in the Painless script class without * the captured types - * @param isDelegateInterface If the method to be called is owned by an interface + * @param isDelegateInterface If the method to be called is owned by an interface where + * if the value is '1' if the delegate is an interface and '0' + * otherwise; note this is an int because the bootstrap method + * cannot convert constants to boolean * @return A {@link CallSite} linked to a factory method for creating a lambda class * that implements the expected functional interface * @throws LambdaConversionException Thrown when an illegal type conversion occurs at link time diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java index d18bdc639297c..e6f2f7ebf91f9 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java @@ -122,7 +122,7 @@ void write(MethodWriter writer, Globals globals) { ref.delegateInvokeType, ref.delegateMethodName, ref.delegateType, - ref.isDelegateInterface + ref.isDelegateInterface ? 1 : 0 ); } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java index 61b28e7d275a6..c82b1003a55f1 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java @@ -113,7 +113,7 @@ void write(MethodWriter writer, Globals globals) { ref.delegateInvokeType, ref.delegateMethodName, ref.delegateType, - ref.isDelegateInterface + ref.isDelegateInterface ? 1 : 0 ); } else { // TODO: don't do this: its just to cutover :) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java index 2f432822cd2a3..a7213e75ca485 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ELambda.java @@ -223,7 +223,7 @@ void write(MethodWriter writer, Globals globals) { ref.delegateInvokeType, ref.delegateMethodName, ref.delegateType, - ref.isDelegateInterface + ref.isDelegateInterface ? 1 : 0 ); } else { // placeholder diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java index 7c49d042108ef..fd47db6b83d41 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java @@ -184,6 +184,11 @@ public void testInterfaceDefaultMethodDef() { "def map = new HashMap(); f(map::getOrDefault)")); } + public void testInterfaceStaticMethod() { + assertEquals(-1, exec("Supplier get(Supplier supplier) { return supplier }" + + "Supplier s = get(Comparator::naturalOrder); s.get().compare(1, 2)")); + } + public void testMethodMissing() { Exception e = expectScriptThrows(IllegalArgumentException.class, () -> { exec("List l = [2, 1]; l.sort(Integer::bogus); return l.get(0);");