From 8999104b14baeaad58215363c113087a517c3adb Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Fri, 2 Jun 2017 14:50:51 -0700 Subject: [PATCH] Change ScriptContexts to use needs instead of uses$. (#25036) --- .../painless/GenericElasticsearchScript.java | 4 +- .../painless/PainlessScriptEngine.java | 2 +- .../painless/ScriptClassInfo.java | 32 +++------ .../elasticsearch/painless/ScriptImpl.java | 4 +- .../painless/WriterConstants.java | 2 +- .../elasticsearch/painless/node/SSource.java | 11 +-- .../painless/BaseClassTests.java | 71 +++++-------------- 7 files changed, 37 insertions(+), 89 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/GenericElasticsearchScript.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/GenericElasticsearchScript.java index 06dd7df68cf..ef2c9513b8e 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/GenericElasticsearchScript.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/GenericElasticsearchScript.java @@ -34,6 +34,6 @@ public abstract class GenericElasticsearchScript { public abstract Object execute( Map params, double _score, Map> doc, Object _value, Map ctx); - public abstract boolean uses$_score(); - public abstract boolean uses$ctx(); + public abstract boolean needs_score(); + public abstract boolean needsCtx(); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java index c93017d489f..021f6a30b81 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java @@ -125,7 +125,7 @@ public final class PainlessScriptEngine extends AbstractComponent implements Scr } @Override public boolean needsScores() { - return painlessScript.uses$_score(); + return painlessScript.needs_score(); } }; return context.factoryClazz.cast(factory); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptClassInfo.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptClassInfo.java index 3703aae2ab4..abf834c8acb 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptClassInfo.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptClassInfo.java @@ -29,7 +29,7 @@ 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; +import static org.elasticsearch.painless.WriterConstants.NEEDS_PARAMETER_METHOD_TYPE; /** * Information about the interface being implemented by the painless script. @@ -40,7 +40,7 @@ public class ScriptClassInfo { private final org.objectweb.asm.commons.Method executeMethod; private final Definition.Type executeMethodReturnType; private final List executeArguments; - private final List usesMethods; + private final List needsMethods; private final List getMethods; private final List getReturns; @@ -49,7 +49,7 @@ public class ScriptClassInfo { // Find the main method and the uses$argName methods java.lang.reflect.Method executeMethod = null; - List usesMethods = new ArrayList<>(); + List needsMethods = new ArrayList<>(); List getMethods = new ArrayList<>(); List getReturns = new ArrayList<>(); for (java.lang.reflect.Method m : baseClass.getMethods()) { @@ -65,16 +65,8 @@ public class ScriptClassInfo { + "] has more than one."); } } - if (m.getName().startsWith("uses$")) { - if (false == m.getReturnType().equals(boolean.class)) { - throw new IllegalArgumentException("Painless can only implement uses$ methods that return boolean but [" - + baseClass.getName() + "#" + m.getName() + "] returns [" + m.getReturnType().getName() + "]."); - } - if (m.getParameterTypes().length > 0) { - throw new IllegalArgumentException("Painless can only implement uses$ methods that do not take parameters but [" - + baseClass.getName() + "#" + m.getName() + "] does."); - } - usesMethods.add(new org.objectweb.asm.commons.Method(m.getName(), USES_PARAMETER_METHOD_TYPE.toMethodDescriptorString())); + if (m.getName().startsWith("needs") && m.getReturnType().equals(boolean.class) && m.getParameterTypes().length == 0) { + needsMethods.add(new org.objectweb.asm.commons.Method(m.getName(), NEEDS_PARAMETER_METHOD_TYPE.toMethodDescriptorString())); } if (m.getName().startsWith("get") && m.getName().equals("getClass") == false && Modifier.isStatic(m.getModifiers()) == false) { getReturns.add( @@ -106,15 +98,7 @@ public class ScriptClassInfo { argumentNames.add(argumentNamesConstant[arg]); } this.executeArguments = unmodifiableList(arguments); - - // Validate that the uses$argName methods reference argument names - for (org.objectweb.asm.commons.Method usesMethod : usesMethods) { - if (false == argumentNames.contains(usesMethod.getName().substring("uses$".length()))) { - throw new IllegalArgumentException("Painless can only implement uses$ methods that match a parameter name but [" - + baseClass.getName() + "#" + usesMethod.getName() + "] doesn't match any of " + argumentNames + "."); - } - } - this.usesMethods = unmodifiableList(usesMethods); + this.needsMethods = unmodifiableList(needsMethods); this.getMethods = unmodifiableList(getMethods); this.getReturns = unmodifiableList(getReturns); } @@ -152,8 +136,8 @@ public class ScriptClassInfo { /** * The {@code uses$varName} methods that must be implemented by Painless to complete implementing the interface. */ - public List getUsesMethods() { - return usesMethods; + public List getNeedsMethods() { + return needsMethods; } /** diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java index 5d5405f1849..067bf38cb36 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java @@ -81,8 +81,8 @@ final class ScriptImpl extends SearchScript { variables.putAll(leafLookup.asMap()); } - scoreLookup = script.uses$_score() ? this::getScore : () -> 0.0; - ctxLookup = script.uses$ctx() ? variables -> (Map) variables.get("ctx") : variables -> null; + scoreLookup = script.needs_score() ? this::getScore : () -> 0.0; + ctxLookup = script.needsCtx() ? variables -> (Map) variables.get("ctx") : variables -> null; } @Override 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 aca9973efa9..60eff7754e8 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 @@ -84,7 +84,7 @@ public final class WriterConstants { public static final Type COLLECTIONS_TYPE = Type.getType(Collections.class); public static final Method EMPTY_MAP_METHOD = getAsmMethod(Map.class, "emptyMap"); - public static final MethodType USES_PARAMETER_METHOD_TYPE = MethodType.methodType(boolean.class); + public static final MethodType NEEDS_PARAMETER_METHOD_TYPE = MethodType.methodType(boolean.class); public static final Type MAP_TYPE = Type.getType(Map.class); public static final Method MAP_GET = getAsmMethod(Object.class, "get", Object.class); diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java index f7654897c23..882b018bc41 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java @@ -347,11 +347,14 @@ public final class SSource extends AStatement { clinit.endMethod(); } - // Write any uses$varName methods for used variables - for (org.objectweb.asm.commons.Method usesMethod : scriptClassInfo.getUsesMethods()) { - MethodWriter ifaceMethod = new MethodWriter(Opcodes.ACC_PUBLIC, usesMethod, visitor, globals.getStatements(), settings); + // Write any needsVarName methods for used variables + for (org.objectweb.asm.commons.Method needsMethod : scriptClassInfo.getNeedsMethods()) { + String name = needsMethod.getName(); + name = name.substring(5); + name = Character.toLowerCase(name.charAt(0)) + name.substring(1); + MethodWriter ifaceMethod = new MethodWriter(Opcodes.ACC_PUBLIC, needsMethod, visitor, globals.getStatements(), settings); ifaceMethod.visitCode(); - ifaceMethod.push(reserved.getUsedVariables().contains(usesMethod.getName().substring("uses$".length()))); + ifaceMethod.push(reserved.getUsedVariables().contains(name)); ifaceMethod.returnValue(); ifaceMethod.endMethod(); } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BaseClassTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BaseClassTests.java index eef541590a4..e4f04b23529 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BaseClassTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BaseClassTests.java @@ -161,10 +161,10 @@ public class BaseClassTests extends ScriptTestCase { public abstract static class ManyArgs { public static final String[] PARAMETERS = new String[] {"a", "b", "c", "d"}; public abstract Object execute(int a, int b, int c, int d); - public abstract boolean uses$a(); - public abstract boolean uses$b(); - public abstract boolean uses$c(); - public abstract boolean uses$d(); + public abstract boolean needsA(); + public abstract boolean needsB(); + public abstract boolean needsC(); + public abstract boolean needsD(); } public void testManyArgs() { Compiler compiler = new Compiler(ManyArgs.class, Definition.BUILTINS); @@ -174,20 +174,20 @@ public class BaseClassTests extends ScriptTestCase { // While we're here we can verify that painless correctly finds used variables ManyArgs script = (ManyArgs)scriptEngine.compile(compiler, null, "a", emptyMap()); - assertTrue(script.uses$a()); - assertFalse(script.uses$b()); - assertFalse(script.uses$c()); - assertFalse(script.uses$d()); + assertTrue(script.needsA()); + assertFalse(script.needsB()); + assertFalse(script.needsC()); + assertFalse(script.needsD()); script = (ManyArgs)scriptEngine.compile(compiler, null, "a + b + c", emptyMap()); - assertTrue(script.uses$a()); - assertTrue(script.uses$b()); - assertTrue(script.uses$c()); - assertFalse(script.uses$d()); + assertTrue(script.needsA()); + assertTrue(script.needsB()); + assertTrue(script.needsC()); + assertFalse(script.needsD()); script = (ManyArgs)scriptEngine.compile(compiler, null, "a + b + c + d", emptyMap()); - assertTrue(script.uses$a()); - assertTrue(script.uses$b()); - assertTrue(script.uses$c()); - assertTrue(script.uses$d()); + assertTrue(script.needsA()); + assertTrue(script.needsB()); + assertTrue(script.needsC()); + assertTrue(script.needsD()); } public abstract static class VarargTest { @@ -473,43 +473,4 @@ public class BaseClassTests extends ScriptTestCase { assertEquals("Painless can only implement interfaces that have a single method named [execute] but [" + TwoExecuteMethods.class.getName() + "] has more than one.", e.getMessage()); } - - public abstract static class BadUsesReturn { - public static final String[] PARAMETERS = new String[] {"foo"}; - public abstract Object execute(String foo); - public abstract Object uses$foo(); - } - public void testBadUsesReturn() { - Compiler compiler = new Compiler(BadUsesReturn.class, Definition.BUILTINS); - Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> - scriptEngine.compile(compiler, null, "null", emptyMap())); - assertEquals("Painless can only implement uses$ methods that return boolean but [" + BadUsesReturn.class.getName() - + "#uses$foo] returns [java.lang.Object].", e.getMessage()); - } - - public abstract static class BadUsesParameter { - public static final String[] PARAMETERS = new String[] {"foo", "bar"}; - public abstract Object execute(String foo, String bar); - public abstract boolean uses$bar(boolean foo); - } - public void testBadUsesParameter() { - Compiler compiler = new Compiler(BadUsesParameter.class, Definition.BUILTINS); - Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> - scriptEngine.compile(compiler, null, "null", emptyMap())); - assertEquals("Painless can only implement uses$ methods that do not take parameters but [" + BadUsesParameter.class.getName() - + "#uses$bar] does.", e.getMessage()); - } - - public abstract static class BadUsesName { - public static final String[] PARAMETERS = new String[] {"foo", "bar"}; - public abstract Object execute(String foo, String bar); - public abstract boolean uses$baz(); - } - public void testBadUsesName() { - Compiler compiler = new Compiler(BadUsesName.class, Definition.BUILTINS); - Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> - scriptEngine.compile(compiler, null, "null", emptyMap())); - assertEquals("Painless can only implement uses$ methods that match a parameter name but [" + BadUsesName.class.getName() - + "#uses$baz] doesn't match any of [foo, bar].", e.getMessage()); - } }