Initial attempt at embedding the traceback into a ScriptException (#300)

commit-id:d1a78ea5

larky/src/main/java/com/verygood/security/larky/jsr223/LarkyCompiledScript.java

@@ -1,6 +1,9 @@
 package com.verygood.security.larky.jsr223;
 
 import com.google.common.io.CharStreams;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.Map;
 
 import com.verygood.security.larky.parser.DefaultLarkyInterpreter;
 import com.verygood.security.larky.parser.InMemMapBackedStarFile;
@@ -9,16 +12,13 @@
 import com.verygood.security.larky.parser.StarFile;
 
 import net.starlark.java.eval.EvalException;
-
-import java.io.IOException;
-import java.io.Reader;
-import java.util.Map;
+import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkEvalWrapper;
 
 import javax.script.Bindings;
 import javax.script.CompiledScript;
 import javax.script.ScriptContext;
 import javax.script.ScriptEngine;
-import javax.script.ScriptException;
 
 
 public class LarkyCompiledScript extends CompiledScript {
@@ -44,20 +44,21 @@ public ScriptEngine getEngine() {
   }
 
   @Override
-  public Object eval(ScriptContext context) throws ScriptException {
-    try(Reader reader = context.getReader()) {
-      final StarFile script = InMemMapBackedStarFile.createStarFile(DEFAULT_SCRIPT_NAME, CharStreams.toString(reader));
-      Bindings globalBindings = context.getBindings(ScriptContext.GLOBAL_SCOPE);
-      Bindings engineBindings = context.getBindings(ScriptContext.ENGINE_SCOPE);
+  public Object eval(ScriptContext context) throws LarkyEvaluationScriptException {
+    ParsedStarFile result;
+    Bindings globalBindings = context.getBindings(ScriptContext.GLOBAL_SCOPE);
+    Bindings engineBindings = context.getBindings(ScriptContext.ENGINE_SCOPE);
 
+    try (Reader reader = context.getReader()) {
+      final StarFile script = InMemMapBackedStarFile.createStarFile(DEFAULT_SCRIPT_NAME, CharStreams.toString(reader));
       final DefaultLarkyInterpreter larkyInterpreter = new DefaultLarkyInterpreter(LARKY_MODE, globalBindings, engineBindings);
-
-      ParsedStarFile result = larkyInterpreter.evaluate(script, context.getWriter());
-      setBindingsValue(globalBindings, engineBindings, result.getGlobals());
-      return result;
-    } catch (IOException | EvalException e) {
-      throw new ScriptException(e);
+      result = larkyInterpreter.evaluate(script, context.getWriter());
+    } catch (IOException | StarlarkEvalWrapper.Exc.RuntimeEvalException | Starlark.UncheckedEvalException |
+             EvalException e) {
+      throw LarkyEvaluationScriptException.of(e);
     }
+    setBindingsValue(globalBindings, engineBindings, result.getGlobals());
+    return result;
   }
 
 

larky/src/main/java/com/verygood/security/larky/jsr223/LarkyEvaluationScriptException.java

@@ -0,0 +1,93 @@
+package com.verygood.security.larky.jsr223;
+
+import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkEvalWrapper;
+import net.starlark.java.syntax.Location;
+
+import org.jetbrains.annotations.Contract;
+import org.jetbrains.annotations.NotNull;
+
+import javax.script.ScriptException;
+
+public class LarkyEvaluationScriptException extends ScriptException {
+
+  private final @NotNull String excToString;
+
+  private LarkyEvaluationScriptException(Exception e) {
+    super(e); // do not construct this exception directly.
+    excToString = super.getMessage();
+  }
+
+  private LarkyEvaluationScriptException(EvalException evalException) {
+    this(evalException, null, -1, -1); // do not construct this exception directly.
+  }
+
+  private LarkyEvaluationScriptException(@NotNull EvalException message, String fileName, int lineNumber, int columnNumber) {
+    super(message.getMessage(), fileName, lineNumber, columnNumber); // do not construct this exception directly.
+    excToString = super.getMessage() + System.lineSeparator() + message.getMessageWithStack();
+  }
+
+  @Contract("_ -> new")
+  public static @NotNull LarkyEvaluationScriptException of(@NotNull Exception e) {
+    if (e instanceof StarlarkEvalWrapper.Exc.RuntimeEvalException
+          || e instanceof Starlark.UncheckedEvalException) {
+      return onUnchecked(e);
+    } else if (e instanceof EvalException) {
+      return onEvalException((EvalException) e);
+    } else {
+      return new LarkyEvaluationScriptException(e);
+    }
+  }
+
+  /**
+   * Both {@link Starlark.UncheckedEvalException} and {@link StarlarkEvalWrapper.Exc.RuntimeEvalException} may not have
+   * stacktraces as they are derivative of {@link RuntimeException}.
+   *
+   * As a result, {@link net.starlark.java.eval.StarlarkThread}'s stacktrace might be buried as the second frame instead
+   * of the first one.
+   */
+  @Contract("_ -> new")
+  private static @NotNull LarkyEvaluationScriptException onUnchecked(final @NotNull Exception e) {
+    if ((e.getCause() instanceof EvalException)) {
+      final EvalException cause = (EvalException) e.getCause();
+      final LarkyEvaluationScriptException scriptException = onEvalException(cause);
+//      scriptException.fillInLarkyStackTrace(cause);
+      return scriptException;
+    }
+    return new LarkyEvaluationScriptException(e);
+  }
+
+  @Contract("_ -> new")
+  private static @NotNull LarkyEvaluationScriptException onEvalException(final @NotNull EvalException larkyException) {
+    final LarkyEvaluationScriptException exception;
+    final Location errorLoc = StarlarkEvalWrapper.Exc.getErrorLocation(larkyException);
+    if (errorLoc != null) {
+      exception = new LarkyEvaluationScriptException(
+        larkyException,
+        errorLoc.file(),
+        errorLoc.line(),
+        errorLoc.column()
+      );
+    } else {
+      exception = new LarkyEvaluationScriptException(larkyException);
+    }
+    return exception;
+  }
+
+  @Override
+  public String toString() {
+    return excToString;
+  }
+
+  /**
+   * Helper method that helps fill in the stack trace from an external {@link EvalException}. This will just invoke
+   * {@link StarlarkEvalWrapper.Exc#fillInLarkyStackTrace(EvalException, Throwable)} with the current exception.
+   *
+   * @param larkyException - The {@link EvalException} that contains the Larky stacktrace
+   */
+  public void fillInLarkyStackTrace(@NotNull EvalException larkyException) {
+    StarlarkEvalWrapper.Exc.fillInLarkyStackTrace(larkyException, this);
+  }
+
+}

larky/src/main/java/net/starlark/java/eval/StarlarkEvalWrapper.java

@@ -2,6 +2,10 @@
 
 import static com.google.common.base.Strings.isNullOrEmpty;
 
+import com.google.common.collect.ImmutableList;
+
+import net.starlark.java.syntax.Location;
+
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -63,6 +67,49 @@ public static Object getAttrFromMethodAnnotations(
   }
 
   public interface Exc {
+
+    /**
+     * Given a {@link EvalException}, will return, if applicable, the metadata surrounding the location of the exception
+     * for the evaluated script.
+     *
+     * @param larkyException - The {@link EvalException} that contains the Larky stacktrace
+     * @return a {@link Location} detailing the filename, line, and row where the error occurred.
+     */
+    static @Nullable Location getErrorLocation(@NotNull final EvalException larkyException) {
+
+      final ImmutableList<StarlarkThread.CallStackEntry> callStack = larkyException.getCallStack();
+      final int n = callStack.size();
+      if (callStack.isEmpty()) {
+        return null;
+      }
+      final StarlarkThread.CallStackEntry leaf = callStack.get(n - 1);
+      return leaf.location;
+    }
+
+    /**
+     * Sets the given {@link Throwable}'s stack trace to a Java-style version of {@link StarlarkThread#getCallStack}.
+     * This is useful to expose the underlying larky callstack to the caller for a simpler way to identify Larky
+     * errors.
+     *
+     * @param larkyException - The {@link EvalException} that contains the Larky stacktrace
+     * @param throwable      - The {@link Throwable} class to hoist the Larky stacktrace above the Java exception
+     *                       callstack.
+     */
+    static void fillInLarkyStackTrace(@NotNull EvalException larkyException, @NotNull Throwable throwable) {
+      final ImmutableList<StarlarkThread.CallStackEntry> callStack = larkyException.getCallStack();
+      final int callStackSize = callStack.size();
+      StackTraceElement[] trace = new StackTraceElement[callStackSize];
+      for (int i = 0; i < callStackSize; i++) {
+        StarlarkThread.CallStackEntry frame = callStack.get(i);
+        trace[trace.length - i - 1] = new StackTraceElement(
+          /*declaringClass=*/ "<larky>",
+          frame.name,
+          frame.location.file(),
+          frame.location.line());
+      }
+      throwable.setStackTrace(trace);
+    }
+
     static String createUncheckedEvalMessage(Throwable cause, @Nullable StarlarkThread thread) {
       String msg = cause.getClass().getSimpleName() + " thrown during Starlark evaluation";
       String context = null;

larky/src/test/java/com/verygood/security/larky/jsr223/LarkyCompiledScriptTest.java

@@ -1,12 +1,17 @@
 package com.verygood.security.larky.jsr223;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import com.google.common.base.Throwables;
+import java.io.StringWriter;
 
 import com.verygood.security.larky.parser.ParsedStarFile;
 
 import org.junit.Test;
 
-import java.io.StringWriter;
 import javax.script.Bindings;
 import javax.script.ScriptContext;
 import javax.script.ScriptException;
@@ -35,8 +40,8 @@ public void testEval() throws ScriptException {
         "" +
         "def main():",
         "    return 'Hello World'",
-        "",
-        "output = main()"
+      "",
+      "output = main()"
     );
 
     LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script);
@@ -45,6 +50,91 @@ public void testEval() throws ScriptException {
     assertEquals(expResult, result.getGlobalEnvironmentVariable("output", String.class));
   }
 
+  @Test
+  public void testEval_withUncheckedException() {
+    LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory();
+    LarkyScriptEngine engine = (LarkyScriptEngine) factory.getScriptEngine();
+    String script = String.join("\n",
+      "" +
+        "def process(input, ctx):",
+      "    foo = 'bar'",
+      "    # expecting error below!",
+      "    input.body = foo",
+      "    return input",
+      "",
+      "output = process(None, {})"
+    );
+
+    LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script);
+
+    ParsedStarFile result = null;
+    LarkyEvaluationScriptException scriptException = null;
+    try {
+      result = (ParsedStarFile) instance.eval();
+      fail("should not have gotten here");
+    } catch (LarkyEvaluationScriptException e) {
+      scriptException = e;
+    } catch (Exception e) {
+      fail("Unexpected exception thrown!");
+    }
+    assertNotNull(scriptException);
+    assertTrue(scriptException.getMessage().contains("cannot set .body field of NoneType value"));
+    assertTrue(
+      Throwables.getStackTraceAsString(scriptException)
+        .contains(
+          /*substring*/
+          "Traceback (most recent call last):\n" +
+            "\tFile \"larky.star\", line 7, column 17, in <toplevel>\n" +
+            "\tFile \"larky.star\", line 4, column 10, in process\n" +
+            "Error: cannot set .body field of NoneType value")
+    );
+  }
+
+  @Test
+  public void testEval_withCheckedException() {
+
+    class OperationException extends Exception {
+
+      public OperationException(Exception e) {
+        super(e);
+      }
+    }
+
+    LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory();
+    LarkyScriptEngine engine = (LarkyScriptEngine) factory.getScriptEngine();
+    String script = String.join("\n",
+      "" +
+        "def process(input, ctx):",
+      "    fail('boom')",
+      "    return input",
+      "",
+      "output = process(None, {})"
+    );
+
+    LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script);
+
+    OperationException operationException = null;
+    try {
+      try {
+        instance.eval();
+      } catch (ScriptException e) {
+        throw new OperationException(e);
+      }
+    } catch (OperationException e) {
+      operationException = e;
+    }
+    assertNotNull(operationException);
+    assertTrue(
+      operationException.getMessage()
+        .contains(
+          /*substring*/
+          "Traceback (most recent call last):\n" +
+            "\tFile \"larky.star\", line 5, column 17, in <toplevel>\n" +
+            "\tFile \"larky.star\", line 2, column 9, in process\n" +
+            "Error in fail: boom")
+    );
+  }
+
   @Test
   public void testEval_context() throws Exception {
     LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory();