Painless: more testing for script_stack (#24168)

`script_stack` is super useful when debugging Painless scripts
because it skips all the "weird" stuff involved that obfuscates
where the actual error is. It skips Painless's internals and
call site bootstrapping.

It works fine, but it didn't have many tests. This converts a
test that we had for line numbers into a test for the
`script_stack`. The line numbers test was an indirect test
for `script_stack`.

Author: nik9000

modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java

@@ -43,7 +43,8 @@ public final class CompilerSettings {
     public static final String PICKY = "picky";
 
     /**
-     * For testing: do not use.
+     * Hack to set the initial "depth" for the {@link DefBootstrap.PIC} and {@link DefBootstrap.MIC}. Only used for testing: do not
+     * overwrite.
      */
     public static final String INITIAL_CALL_SITE_DEPTH = "initialCallSiteDepth";
 

modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java

@@ -77,8 +77,8 @@ private void arrayLoadStoreTestCase(boolean declareAsDef, String valueType, Obje
     }
 
     private void expectOutOfBounds(int index, String script, Object val) {
-        IndexOutOfBoundsException e = expectScriptThrows(IndexOutOfBoundsException.class,
-                () -> exec(script, singletonMap("val", val), true));
+        IndexOutOfBoundsException e = expectScriptThrows(IndexOutOfBoundsException.class, () ->
+            exec(script, singletonMap("val", val), true));
         try {
             assertThat(e.getMessage(), outOfBoundsExceptionMessageMatcher(index, 5));
         } catch (AssertionError ae) {

modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java

@@ -186,7 +186,7 @@ public void testNullSafeDeref() {
         assertNull(         exec("def    a = null;  return a?.toString()"));
         assertEquals("foo", exec("def    a = 'foo'; return a?.toString()"));
         //   Call with primitive result
-        assertMustBeNullable(    "String a = null; return a?.length()");
+        assertMustBeNullable(    "String a = null;  return a?.length()");
         assertMustBeNullable(    "String a = 'foo'; return a?.length()");
         assertNull(         exec("def    a = null;  return a?.length()"));
         assertEquals(3,     exec("def    a = 'foo'; return a?.length()"));
@@ -265,7 +265,7 @@ public void testNullSafeDeref() {
     }
 
     private void assertMustBeNullable(String script) {
-        Exception e = expectScriptThrows(IllegalArgumentException.class , () -> exec(script));
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> exec(script));
         assertEquals("Result of null safe operator must be nullable", e.getMessage());
     }
 }

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

@@ -325,7 +325,7 @@ public interface NoArgumentsConstant {
         Object execute(String foo);
     }
     public void testNoArgumentsConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(NoArgumentsConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("Painless needs a constant [String[] ARGUMENTS] on all interfaces it implements with the "
                 + "names of the method arguments but [" + NoArgumentsConstant.class.getName() + "] doesn't have one."));
@@ -336,7 +336,7 @@ public interface WrongArgumentsConstant {
         Object execute(String foo);
     }
     public void testWrongArgumentsConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(WrongArgumentsConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("Painless needs a constant [String[] ARGUMENTS] on all interfaces it implements with the "
                 + "names of the method arguments but [" + WrongArgumentsConstant.class.getName() + "] doesn't have one."));
@@ -347,7 +347,7 @@ public interface WrongLengthOfArgumentConstant {
         Object execute(String foo);
     }
     public void testWrongLengthOfArgumentConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(WrongLengthOfArgumentConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("[" + WrongLengthOfArgumentConstant.class.getName() + "#ARGUMENTS] has length [2] but ["
                 + WrongLengthOfArgumentConstant.class.getName() + "#execute] takes [1] argument."));
@@ -358,7 +358,7 @@ public interface UnknownArgType {
         Object execute(UnknownArgType foo);
     }
     public void testUnknownArgType() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(UnknownArgType.class, null, "1", emptyMap()));
         assertEquals("[foo] is of unknown type [" + UnknownArgType.class.getName() + ". Painless interfaces can only accept arguments "
                 + "that are of whitelisted types.", e.getMessage());
@@ -369,7 +369,7 @@ public interface UnknownReturnType {
         UnknownReturnType execute(String foo);
     }
     public void testUnknownReturnType() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             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());
@@ -380,7 +380,7 @@ public interface UnknownArgTypeInArray {
         Object execute(UnknownArgTypeInArray[] foo);
     }
     public void testUnknownArgTypeInArray() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(UnknownArgTypeInArray.class, null, "1", emptyMap()));
         assertEquals("[foo] is of unknown type [" + UnknownArgTypeInArray.class.getName() + ". Painless interfaces can only accept "
                 + "arguments that are of whitelisted types.", e.getMessage());
@@ -391,7 +391,7 @@ public interface TwoExecuteMethods {
         Object execute(boolean foo);
     }
     public void testTwoExecuteMethods() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(TwoExecuteMethods.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement interfaces that have a single method named [execute] but ["
                 + TwoExecuteMethods.class.getName() + "] has more than one.", e.getMessage());
@@ -401,7 +401,7 @@ public interface BadMethod {
         Object something();
     }
     public void testBadMethod() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadMethod.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement methods named [execute] and [uses$argName] but [" + BadMethod.class.getName()
                 + "] contains a method named [something]", e.getMessage());
@@ -413,7 +413,7 @@ public interface BadUsesReturn {
         Object uses$foo();
     }
     public void testBadUsesReturn() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesReturn.class, 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());
@@ -425,7 +425,7 @@ public interface BadUsesParameter {
         boolean uses$bar(boolean foo);
     }
     public void testBadUsesParameter() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesParameter.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement uses$ methods that do not take parameters but [" + BadUsesParameter.class.getName()
                 + "#uses$bar] does.", e.getMessage());
@@ -437,7 +437,7 @@ public interface BadUsesName {
         boolean uses$baz();
     }
     public void testBadUsesName() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesName.class, 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());

modules/lang-painless/src/test/java/org/elasticsearch/painless/LambdaTests.java

@@ -204,7 +204,7 @@ public void testNestedCaptureParams() {
 
     public void testWrongArity() {
         assumeFalse("JDK is JDK 9", Constants.JRE_IS_MINIMUM_JAVA9);
-        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
+        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> {
             exec("Optional.empty().orElseGet(x -> x);");
         });
         assertTrue(expected.getMessage().contains("Incorrect number of parameters"));
@@ -220,7 +220,7 @@ public void testWrongArityDef() {
 
     public void testWrongArityNotEnough() {
         assumeFalse("JDK is JDK 9", Constants.JRE_IS_MINIMUM_JAVA9);
-        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
+        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> {
             exec("List l = new ArrayList(); l.add(1); l.add(1); "
                + "return l.stream().mapToInt(() -> 5).sum();");
         });

modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java

@@ -26,10 +26,8 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.regex.Pattern;
-import java.util.regex.PatternSyntaxException;
 
 import static java.util.Collections.singletonMap;
-import static org.hamcrest.Matchers.containsString;
 
 public class RegexTests extends ScriptTestCase {
     @Override
@@ -264,8 +262,9 @@ public void testBadRegexPattern() {
         assertEquals("Error compiling regex: Illegal Unicode escape sequence", e.getCause().getMessage());
 
         // And make sure the location of the error points to the offset inside the pattern
-        assertEquals("/\\ujjjj/", e.getScriptStack().get(0));
-        assertEquals("   ^---- HERE", e.getScriptStack().get(1));
+        assertScriptStack(e,
+                "/\\ujjjj/",
+                "   ^---- HERE");
     }
 
     public void testRegexAgainstNumber() {

modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java

@@ -35,6 +35,8 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.hamcrest.Matchers.hasSize;
+
 /**
  * Base test case for scripting unit tests.
  * <p>
@@ -114,10 +116,29 @@ public void assertBytecodeHasPattern(String script, String pattern) {
 
     /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */
     public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType, ThrowingRunnable runnable) {
+        return expectScriptThrows(expectedType, true, runnable);
+    }
+
+    /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */
+    public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType, boolean shouldHaveScriptStack,
+            ThrowingRunnable runnable) {
         try {
             runnable.run();
         } catch (Throwable e) {
             if (e instanceof ScriptException) {
+                boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty();
+                if (shouldHaveScriptStack && hasEmptyScriptStack) {
+                    if (0 != e.getCause().getStackTrace().length) {
+                        // Without -XX:-OmitStackTraceInFastThrow the jvm can eat the stack trace which causes us to ignore script_stack
+                        AssertionFailedError assertion = new AssertionFailedError("ScriptException should have a scriptStack");
+                        assertion.initCause(e);
+                        throw assertion;
+                    }
+                } else if (false == shouldHaveScriptStack && false == hasEmptyScriptStack) {
+                    AssertionFailedError assertion = new AssertionFailedError("ScriptException shouldn't have a scriptStack");
+                    assertion.initCause(e);
+                    throw assertion;
+                }
                 e = e.getCause();
                 if (expectedType.isInstance(e)) {
                     return expectedType.cast(e);
@@ -134,4 +155,21 @@ public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType,
         }
         throw new AssertionFailedError("Expected exception " + expectedType.getSimpleName());
     }
+
+    /**
+     * Asserts that the script_stack looks right.
+     */
+    public static void assertScriptStack(ScriptException e, String... stack) {
+        // This particular incantation of assertions makes the error messages more useful
+        try {
+            assertThat(e.getScriptStack(), hasSize(stack.length));
+            for (int i = 0; i < stack.length; i++) {
+                assertEquals(stack[i], e.getScriptStack().get(i));
+            }
+        } catch (AssertionError assertion) {
+            assertion.initCause(e);
+            throw assertion;
+        }
+    }
+
 }

modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java

@@ -165,12 +165,12 @@ public void testStringAndCharacter() {
         assertEquals('c', exec("String s = \"c\"; (char)s"));
         assertEquals('c', exec("String s = 'c'; (char)s"));
 
-        ClassCastException expected = expectScriptThrows(ClassCastException.class, () -> {
+        ClassCastException expected = expectScriptThrows(ClassCastException.class, false, () -> {
             assertEquals("cc", exec("return (String)(char)\"cc\""));
         });
         assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char]."));
 
-        expected = expectScriptThrows(ClassCastException.class, () -> {
+        expected = expectScriptThrows(ClassCastException.class, false, () -> {
             assertEquals("cc", exec("return (String)(char)'cc'"));
         });
         assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char]."));