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`.

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 @@ public abstract class ArrayLikeObjectTestCase extends ScriptTestCase {
     }
 
     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 class BasicExpressionTests extends ScriptTestCase {
         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 class BasicExpressionTests extends ScriptTestCase {
     }
 
     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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class ImplementInterfacesTests extends ScriptTestCase {
         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 class LambdaTests extends ScriptTestCase {
 
     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 class LambdaTests extends ScriptTestCase {
 
     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.nio.CharBuffer;
 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 class RegexTests extends ScriptTestCase {
         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 org.junit.Before;
 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 abstract class ScriptTestCase extends ESTestCase {
 
     /** 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 abstract class ScriptTestCase extends ESTestCase {
         }
         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 class StringTests extends ScriptTestCase {
         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]."));

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

@@ -19,7 +19,10 @@
 
 package org.elasticsearch.painless;
 
+import junit.framework.AssertionFailedError;
+
 import org.apache.lucene.util.Constants;
+import org.elasticsearch.script.ScriptException;
 
 import java.lang.invoke.WrongMethodTypeException;
 import java.util.Arrays;
@@ -27,52 +30,93 @@ import java.util.Collections;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
+import static org.hamcrest.Matchers.instanceOf;
 
 public class WhenThingsGoWrongTests extends ScriptTestCase {
     public void testNullPointer() {
         expectScriptThrows(NullPointerException.class, () -> {
             exec("int x = params['missing']; return x;");
         });
+        expectScriptThrows(NullPointerException.class, () -> {
+            exec("Double.parseDouble(params['missing'])");
+        });
     }
 
-    /** test "line numbers" in the bytecode, which are really 1-based offsets */
-    public void testLineNumbers() {
-        // trigger NPE at line 1 of the script
-        NullPointerException exception = expectScriptThrows(NullPointerException.class, () -> {
-            exec("String x = null; boolean y = x.isEmpty();\n" +
-                 "return y;");
-        });
-        // null deref at x.isEmpty(), the '.' is offset 30 (+1)
-        assertEquals(30 + 1, exception.getStackTrace()[0].getLineNumber());
+    /**
+     * Test that the scriptStack looks good. By implication this tests that we build proper "line numbers" in stack trace. These line
+     * numbers are really 1 based character numbers.
+     */
+    public void testScriptStack() {
+        for (String type : new String[] {"String", "def   "}) {
+            // trigger NPE at line 1 of the script
+            ScriptException exception = expectThrows(ScriptException.class, () -> {
+                exec(type + " x = null; boolean y = x.isEmpty();\n" +
+                     "return y;");
+            });
+            // null deref at x.isEmpty(), the '.' is offset 30
+            assertScriptElementColumn(30, exception);
+            assertScriptStack(exception,
+                    "y = x.isEmpty();\n",
+                    "     ^---- HERE");
+            assertThat(exception.getCause(), instanceOf(NullPointerException.class));
+
+            // trigger NPE at line 2 of the script
+            exception = expectThrows(ScriptException.class, () -> {
+                exec(type + " x = null;\n" +
+                     "return x.isEmpty();");
+            });
+            // null deref at x.isEmpty(), the '.' is offset 25
+            assertScriptElementColumn(25, exception);
+            assertScriptStack(exception,
+                    "return x.isEmpty();",
+                    "        ^---- HERE");
+            assertThat(exception.getCause(), instanceOf(NullPointerException.class));
+
+            // trigger NPE at line 3 of the script
+            exception = expectThrows(ScriptException.class, () -> {
+                exec(type + " x = null;\n" +
+                     type + " y = x;\n" +
+                     "return y.isEmpty();");
+            });
+            // null deref at y.isEmpty(), the '.' is offset 39
+            assertScriptElementColumn(39, exception);
+            assertScriptStack(exception,
+                    "return y.isEmpty();",
+                    "        ^---- HERE");
+            assertThat(exception.getCause(), instanceOf(NullPointerException.class));
+
+            // trigger NPE at line 4 in script (inside conditional)
+            exception = expectThrows(ScriptException.class, () -> {
+                exec(type + " x = null;\n" +
+                     "boolean y = false;\n" +
+                     "if (!y) {\n" +
+                     "  y = x.isEmpty();\n" +
+                     "}\n" +
+                     "return y;");
+            });
+            // null deref at x.isEmpty(), the '.' is offset 53
+            assertScriptElementColumn(53, exception);
+            assertScriptStack(exception,
+                    "y = x.isEmpty();\n}\n",
+                    "     ^---- HERE");
+            assertThat(exception.getCause(), instanceOf(NullPointerException.class));
+        }
+    }
 
-        // trigger NPE at line 2 of the script
-        exception = expectScriptThrows(NullPointerException.class, () -> {
-            exec("String x = null;\n" +
-                 "return x.isEmpty();");
-        });
-        // null deref at x.isEmpty(), the '.' is offset 25 (+1)
-        assertEquals(25 + 1, exception.getStackTrace()[0].getLineNumber());
-
-        // trigger NPE at line 3 of the script
-        exception = expectScriptThrows(NullPointerException.class, () -> {
-            exec("String x = null;\n" +
-                 "String y = x;\n" +
-                 "return y.isEmpty();");
-        });
-        // null deref at y.isEmpty(), the '.' is offset 39 (+1)
-        assertEquals(39 + 1, exception.getStackTrace()[0].getLineNumber());
-
-        // trigger NPE at line 4 in script (inside conditional)
-        exception = expectScriptThrows(NullPointerException.class, () -> {
-            exec("String x = null;\n" +
-                 "boolean y = false;\n" +
-                 "if (!y) {\n" +
-                 "  y = x.isEmpty();\n" +
-                 "}\n" +
-                 "return y;");
-        });
-        // null deref at x.isEmpty(), the '.' is offset 53 (+1)
-        assertEquals(53 + 1, exception.getStackTrace()[0].getLineNumber());
+    private void assertScriptElementColumn(int expectedColumn, ScriptException exception) {
+        StackTraceElement[] stackTrace = exception.getCause().getStackTrace();
+        for (int i = 0; i < stackTrace.length; i++) {
+            if (WriterConstants.CLASS_NAME.equals(stackTrace[i].getClassName())) {
+                if (expectedColumn + 1 != stackTrace[i].getLineNumber()) {
+                    AssertionFailedError assertion = new AssertionFailedError("Expected column to be [" + expectedColumn + "] but was ["
+                            + stackTrace[i].getLineNumber() + "]");
+                    assertion.initCause(exception);
+                    throw assertion;
+                }
+                return;
+            }
+        }
+        fail("didn't find script stack element");
     }
 
     public void testInvalidShift() {
@@ -161,7 +205,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase {
         final char[] tooManyChars = new char[Compiler.MAXIMUM_SOURCE_LENGTH + 1];
         Arrays.fill(tooManyChars, '0');
 
-        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
+        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> {
             exec(new String(tooManyChars));
         });
         assertTrue(expected.getMessage().contains("Scripts may be no longer than"));
@@ -282,5 +326,4 @@ public class WhenThingsGoWrongTests extends ScriptTestCase {
         e = expectScriptThrows(IllegalArgumentException.class, () -> exec("'cat", false));
         assertEquals("unexpected character ['cat].", e.getMessage());
     }
-
 }