honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix Painless's implementation of interfaces returning primitives (#23298) 

Fixes Painless to properly implement scripts that return primitives
and void. Adds some simple tests that we emit sane opcodes and some
other tests that we implement primitives as expected.

Mostly this is just a fix following up from #22983 but there is one
thing I did really worth talking about, I think. So, before this script
Painless scripts could only ever return Object and they did would always
return null for paths that didn't return any values. Now that they
can return primitives the question is "what should Painless return
from paths that don't return any values?" And I answered that with
"whatever the JLS default value is". So 0/0L/0f/0d/false.
This commit is contained in:
Nik Everett 2017-02-21 17:10:55 -05:00 committed by GitHub
parent ca38e88148
commit 38d25a0369
4 changed files with 225 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
modules/lang-painless/src/main/java/org/elasticsearch/painless/Locals.java
View File

@ -92,12 +92,12 @@ public final class Locals {
/** Creates a new main method scope */
public static Locals newMainMethodScope(ScriptInterface scriptInterface, Locals programScope, int maxLoopCounter) {
Locals locals = new Locals(programScope, Definition.OBJECT_TYPE, KEYWORDS);
Locals locals = new Locals(programScope, scriptInterface.getExecuteMethodReturnType(), KEYWORDS);
// This reference. Internal use only.
locals.defineVariable(null, Definition.getType("Object"), THIS, true);
// Method arguments
for (MethodArgument arg : scriptInterface.getArguments()) {
for (MethodArgument arg : scriptInterface.getExecuteArguments()) {
locals.defineVariable(null, arg.getType(), arg.getName(), true);
}

59
modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptInterface.java
View File

@ -25,6 +25,7 @@ import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import static java.util.Collections.unmodifiableList;
import static org.elasticsearch.painless.WriterConstants.USES_PARAMETER_METHOD_TYPE;
@ -35,7 +36,8 @@ import static org.elasticsearch.painless.WriterConstants.USES_PARAMETER_METHOD_T
public class ScriptInterface {
private final Class<?> iface;
private final org.objectweb.asm.commons.Method executeMethod;
private final List<MethodArgument> arguments;
private final Definition.Type executeMethodReturnType;
private final List<MethodArgument> executeArguments;
private final List<org.objectweb.asm.commons.Method> usesMethods;
public ScriptInterface(Class<?> iface) {
@ -75,6 +77,9 @@ public class ScriptInterface {
}
MethodType methodType = MethodType.methodType(executeMethod.getReturnType(), executeMethod.getParameterTypes());
this.executeMethod = new org.objectweb.asm.commons.Method(executeMethod.getName(), methodType.toMethodDescriptorString());
executeMethodReturnType = definitionTypeForClass(executeMethod.getReturnType(),
componentType -> "Painless can only implement execute methods returning a whitelisted type but [" + iface.getName()
+ "#execute] returns [" + componentType.getName() + "] which isn't whitelisted.");
// Look up the argument names
Set<String> argumentNames = new LinkedHashSet<>();
@ -86,10 +91,10 @@ public class ScriptInterface {
+ iface.getName() + "#execute] takes [1] argument.");
}
for (int arg = 0; arg < types.length; arg++) {
arguments.add(new MethodArgument(argType(argumentNamesConstant[arg], types[arg]), argumentNamesConstant[arg]));
arguments.add(methodArgument(types[arg], argumentNamesConstant[arg]));
argumentNames.add(argumentNamesConstant[arg]);
}
this.arguments = unmodifiableList(arguments);
this.executeArguments = unmodifiableList(arguments);
// Validate that the uses$argName methods reference argument names
for (org.objectweb.asm.commons.Method usesMethod : usesMethods) {
@ -101,22 +106,46 @@ public class ScriptInterface {
this.usesMethods = unmodifiableList(usesMethods);
}
/**
* The interface that the Painless script should implement.
*/
public Class<?> getInterface() {
return iface;
}
/**
* An asm method descriptor for the {@code execute} method.
*/
public org.objectweb.asm.commons.Method getExecuteMethod() {
return executeMethod;
}
public List<MethodArgument> getArguments() {
return arguments;
/**
* The Painless {@link Definition.Type} or the return type of the {@code execute} method. This is used to generate the appropriate
* return bytecode.
*/
public Definition.Type getExecuteMethodReturnType() {
return executeMethodReturnType;
}
/**
* Painless {@link Definition.Type}s and names of the arguments to the {@code execute} method. The names are exposed to the Painless
* script.
*/
public List<MethodArgument> getExecuteArguments() {
return executeArguments;
}
/**
* The {@code uses$varName} methods that must be implemented by Painless to complete implementing the interface.
*/
public List<org.objectweb.asm.commons.Method> getUsesMethods() {
return usesMethods;
}
/**
* Painless {@link Definition.Type}s and name of the argument to the {@code execute} method.
*/
public static class MethodArgument {
private final Definition.Type type;
private final String name;
@ -135,20 +164,26 @@ public class ScriptInterface {
}
}
private static Definition.Type argType(String argName, Class<?> type) {
private static MethodArgument methodArgument(Class<?> type, String argName) {
Definition.Type defType = definitionTypeForClass(type, componentType -> "[" + argName + "] is of unknown type ["
+ componentType.getName() + ". Painless interfaces can only accept arguments that are of whitelisted types.");
return new MethodArgument(defType, argName);
}
private static Definition.Type definitionTypeForClass(Class<?> type, Function<Class<?>, String> unknownErrorMessageSource) {
int dimensions = 0;
while (type.isArray()) {
Class<?> componentType = type;
while (componentType.isArray()) {
dimensions++;
type = type.getComponentType();
componentType = componentType.getComponentType();
}
Definition.Struct struct;
if (type.equals(Object.class)) {
if (componentType.equals(Object.class)) {
struct = Definition.DEF_TYPE.struct;
} else {
Definition.RuntimeClass runtimeClass = Definition.getRuntimeClass(type);
Definition.RuntimeClass runtimeClass = Definition.getRuntimeClass(componentType);
if (runtimeClass == null) {
throw new IllegalArgumentException("[" + argName + "] is of unknown type [" + type.getName()
+ ". Painless interfaces can only accept arguments that are of whitelisted types.");
throw new IllegalArgumentException(unknownErrorMessageSource.apply(componentType));
}
struct = runtimeClass.getStruct();
}

12
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java
View File

@ -312,7 +312,17 @@ public final class SSource extends AStatement {
}
if (!methodEscape) {
writer.visitInsn(Opcodes.ACONST_NULL);
switch (scriptInterface.getExecuteMethod().getReturnType().getSort()) {
case org.objectweb.asm.Type.VOID: break;
case org.objectweb.asm.Type.BOOLEAN: writer.push(false); break;
case org.objectweb.asm.Type.BYTE: writer.push(0); break;
case org.objectweb.asm.Type.SHORT: writer.push(0); break;
case org.objectweb.asm.Type.INT: writer.push(0); break;
case org.objectweb.asm.Type.LONG: writer.push(0L); break;
case org.objectweb.asm.Type.FLOAT: writer.push(0f); break;
case org.objectweb.asm.Type.DOUBLE: writer.push(0d); break;
default: writer.visitInsn(Opcodes.ACONST_NULL);
}
writer.returnValue();
}

165
modules/lang-painless/src/test/java/org/elasticsearch/painless/ImplementInterfacesTests.java
View File

@ -24,6 +24,8 @@ import java.util.Map;
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
/**
@ -45,6 +47,12 @@ public class ImplementInterfacesTests extends ScriptTestCase {
e = expectScriptThrows(IllegalArgumentException.class, () ->
scriptEngine.compile(NoArgs.class, null, "_score", emptyMap()));
assertEquals("Variable [_score] is not defined.", e.getMessage());
String debug = Debugger.toString(NoArgs.class, "int i = 0", new CompilerSettings());
/* Elasticsearch requires that scripts that return nothing return null. We hack that together by returning null from scripts that
* return Object if they don't return anything. */
assertThat(debug, containsString("ACONST_NULL"));
assertThat(debug, containsString("ARETURN"));
}
public interface OneArg {
@ -165,6 +173,152 @@ public class ImplementInterfacesTests extends ScriptTestCase {
assertEquals(singletonMap("a", "foo"), map);
scriptEngine.compile(ReturnsVoid.class, null, "map.remove('a')", emptyMap()).execute(map);
assertEquals(emptyMap(), map);
String debug = Debugger.toString(ReturnsVoid.class, "int i = 0", new CompilerSettings());
// The important thing is that this contains the opcode for returning void
assertThat(debug, containsString(" RETURN"));
// We shouldn't contain any weird "default to null" logic
assertThat(debug, not(containsString("ACONST_NULL")));
}
public interface ReturnsPrimitiveBoolean {
String[] ARGUMENTS = new String[] {};
boolean execute();
}
public void testReturnsPrimitiveBoolean() {
assertEquals(true, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "true", emptyMap()).execute());
assertEquals(false, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "false", emptyMap()).execute());
assertEquals(true, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "Boolean.TRUE", emptyMap()).execute());
assertEquals(false, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "Boolean.FALSE", emptyMap()).execute());
assertEquals(true, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "def i = true; i", emptyMap()).execute());
assertEquals(true, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "def i = Boolean.TRUE; i", emptyMap()).execute());
assertEquals(true, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "true || false", emptyMap()).execute());
String debug = Debugger.toString(ReturnsPrimitiveBoolean.class, "false", new CompilerSettings());
assertThat(debug, containsString("ICONST_0"));
// The important thing here is that we have the bytecode for returning an integer instead of an object. booleans are integers.
assertThat(debug, containsString("IRETURN"));
Exception e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "1L", emptyMap()).execute());
assertEquals("Cannot cast from [long] to [boolean].", e.getMessage());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "1.1f", emptyMap()).execute());
assertEquals("Cannot cast from [float] to [boolean].", e.getMessage());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "1.1d", emptyMap()).execute());
assertEquals("Cannot cast from [double] to [boolean].", e.getMessage());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "def i = 1L; i", emptyMap()).execute());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "def i = 1.1f; i", emptyMap()).execute());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "def i = 1.1d; i", emptyMap()).execute());
assertEquals(false, scriptEngine.compile(ReturnsPrimitiveBoolean.class, null, "int i = 0", emptyMap()).execute());
}
public interface ReturnsPrimitiveInt {
String[] ARGUMENTS = new String[] {};
int execute();
}
public void testReturnsPrimitiveInt() {
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "1", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "(int) 1L", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "(int) 1.1d", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "(int) 1.1f", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "Integer.valueOf(1)", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "def i = 1; i", emptyMap()).execute());
assertEquals(1, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "def i = Integer.valueOf(1); i", emptyMap()).execute());
assertEquals(2, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "1 + 1", emptyMap()).execute());
String debug = Debugger.toString(ReturnsPrimitiveInt.class, "1", new CompilerSettings());
assertThat(debug, containsString("ICONST_1"));
// The important thing here is that we have the bytecode for returning an integer instead of an object
assertThat(debug, containsString("IRETURN"));
Exception e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "1L", emptyMap()).execute());
assertEquals("Cannot cast from [long] to [int].", e.getMessage());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "1.1f", emptyMap()).execute());
assertEquals("Cannot cast from [float] to [int].", e.getMessage());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "1.1d", emptyMap()).execute());
assertEquals("Cannot cast from [double] to [int].", e.getMessage());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "def i = 1L; i", emptyMap()).execute());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "def i = 1.1f; i", emptyMap()).execute());
expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveInt.class, null, "def i = 1.1d; i", emptyMap()).execute());
assertEquals(0, scriptEngine.compile(ReturnsPrimitiveInt.class, null, "int i = 0", emptyMap()).execute());
}
public interface ReturnsPrimitiveFloat {
String[] ARGUMENTS = new String[] {};
float execute();
}
public void testReturnsPrimitiveFloat() {
assertEquals(1.1f, scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "1.1f", emptyMap()).execute(), 0);
assertEquals(1.1f, scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "(float) 1.1d", emptyMap()).execute(), 0);
assertEquals(1.1f, scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "def d = 1.1f; d", emptyMap()).execute(), 0);
assertEquals(1.1f,
scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "def d = Float.valueOf(1.1f); d", emptyMap()).execute(), 0);
assertEquals(1.1f + 6.7f, scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "1.1f + 6.7f", emptyMap()).execute(), 0);
Exception e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "1.1d", emptyMap()).execute());
assertEquals("Cannot cast from [double] to [float].", e.getMessage());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "def d = 1.1d; d", emptyMap()).execute());
e = expectScriptThrows(ClassCastException.class, () ->
scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "def d = Double.valueOf(1.1); d", emptyMap()).execute());
String debug = Debugger.toString(ReturnsPrimitiveFloat.class, "1f", new CompilerSettings());
assertThat(debug, containsString("FCONST_1"));
// The important thing here is that we have the bytecode for returning a float instead of an object
assertThat(debug, containsString("FRETURN"));
assertEquals(0.0f, scriptEngine.compile(ReturnsPrimitiveFloat.class, null, "int i = 0", emptyMap()).execute(), 0);
}
public interface ReturnsPrimitiveDouble {
String[] ARGUMENTS = new String[] {};
double execute();
}
public void testReturnsPrimitiveDouble() {
assertEquals(1.0, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "1", emptyMap()).execute(), 0);
assertEquals(1.0, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "1L", emptyMap()).execute(), 0);
assertEquals(1.1, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "1.1d", emptyMap()).execute(), 0);
assertEquals((double) 1.1f, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "1.1f", emptyMap()).execute(), 0);
assertEquals(1.1, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "Double.valueOf(1.1)", emptyMap()).execute(), 0);
assertEquals((double) 1.1f,
scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "Float.valueOf(1.1f)", emptyMap()).execute(), 0);
assertEquals(1.0, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = 1; d", emptyMap()).execute(), 0);
assertEquals(1.0, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = 1L; d", emptyMap()).execute(), 0);
assertEquals(1.1, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = 1.1d; d", emptyMap()).execute(), 0);
assertEquals((double) 1.1f, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = 1.1f; d", emptyMap()).execute(), 0);
assertEquals(1.1,
scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = Double.valueOf(1.1); d", emptyMap()).execute(), 0);
assertEquals((double) 1.1f,
scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "def d = Float.valueOf(1.1f); d", emptyMap()).execute(), 0);
assertEquals(1.1 + 6.7, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "1.1 + 6.7", emptyMap()).execute(), 0);
String debug = Debugger.toString(ReturnsPrimitiveDouble.class, "1", new CompilerSettings());
assertThat(debug, containsString("DCONST_1"));
// The important thing here is that we have the bytecode for returning a double instead of an object
assertThat(debug, containsString("DRETURN"));
assertEquals(0.0, scriptEngine.compile(ReturnsPrimitiveDouble.class, null, "int i = 0", emptyMap()).execute(), 0);
}
public interface NoArgumentsConstant {
@ -210,6 +364,17 @@ public class ImplementInterfacesTests extends ScriptTestCase {
+ "that are of whitelisted types.", e.getMessage());
}
public interface UnknownReturnType {
String[] ARGUMENTS = new String[] {"foo"};
UnknownReturnType execute(String foo);
}
public void testUnknownReturnType() {
Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
scriptEngine.compile(UnknownReturnType.class, null, "1", emptyMap()));
assertEquals("Painless can only implement execute methods returning a whitelisted type but [" + UnknownReturnType.class.getName()
+ "#execute] returns [" + UnknownReturnType.class.getName() + "] which isn't whitelisted.", e.getMessage());
}
public interface UnknownArgTypeInArray {
String[] ARGUMENTS = new String[] {"foo"};
Object execute(UnknownArgTypeInArray[] foo);