Fix display of evaluation errors thrown by command convert/transformAll (#1431)

pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt

@@ -942,7 +942,52 @@ class CliCommandRunnerTest {
       assertThrows<CliktError> {
         runToStdout(CliBaseOptions(sourceModules = listOf(moduleUri)), listOf("hi"))
       }
-    assertThat(exc.message).isEqualTo("oops!")
+    assertThat(exc.message).contains("oops!")
+  }
+
+  @Test
+  fun `convert with eval error`() {
+    val moduleUri =
+      writePklFile(
+        "cmd.pkl",
+        renderOptions +
+          """
+      class Options {
+        @Argument { convert = (it) -> it.noSuchMethod() }
+        foo: String
+      }
+    """
+            .trimIndent(),
+      )
+
+    val exc =
+      assertThrows<CliktError> {
+        runToStdout(CliBaseOptions(sourceModules = listOf(moduleUri)), listOf("hi"))
+      }
+    assertThat(exc.message).contains("Cannot find method `noSuchMethod` in class `String`.")
+  }
+
+  @Test
+  fun `convert with stack overflow`() {
+    val moduleUri =
+      writePklFile(
+        "cmd.pkl",
+        renderOptions +
+          """
+      const function overflow(it) = overflow(it)
+      class Options {
+        @Argument { convert = (it) -> overflow(it) }
+        foo: String
+      }
+    """
+            .trimIndent(),
+      )
+
+    val exc =
+      assertThrows<CliktError> {
+        runToStdout(CliBaseOptions(sourceModules = listOf(moduleUri)), listOf("hi"))
+      }
+    assertThat(exc.message).contains("A stack overflow occurred.")
   }
 
   @Test

pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java

@@ -15,7 +15,6 @@
  */
 package org.pkl.core;
 
-import com.oracle.truffle.api.TruffleStackTrace;
 import java.io.IOException;
 import java.nio.file.Path;
 import java.time.Duration;
@@ -286,6 +285,8 @@ public final class EvaluatorImpl implements Evaluator {
               new CommandSpecParser(
                   moduleResolver,
                   securityManager,
+                  frameTransformer,
+                  color,
                   (fileOutput) -> new FileOutputImpl(this, fileOutput));
           run.accept(commandRunner.parse(module));
           return null;
@@ -358,7 +359,7 @@ public final class EvaluatorImpl implements Evaluator {
     try {
       evalResult = supplier.get();
     } catch (VmStackOverflowException e) {
-      if (isPklBug(e)) {
+      if (VmUtils.isPklBug(e)) {
         throw new VmExceptionBuilder()
             .bug("Stack overflow")
             .withCause(e.getCause())
@@ -387,7 +388,7 @@ public final class EvaluatorImpl implements Evaluator {
       if (e.getClass()
           .getName()
           .equals("com.oracle.truffle.polyglot.PolyglotEngineImpl$CancelExecution")) {
-        // Truffle cancelled evaluation in response to polyglotContext.close(true) triggered by
+        // Truffle canceled evaluation in response to polyglotContext.close(true) triggered by
         // TimeoutTask
         handleTimeout(timeoutTask);
         throw PklBugException.unreachableCode();
@@ -398,7 +399,7 @@ public final class EvaluatorImpl implements Evaluator {
       try {
         polyglotContext.leave();
       } catch (IllegalStateException ignored) {
-        // happens if evaluation has already been cancelled with polyglotContext.close(true)
+        // happens if evaluation has already been canceled with polyglotContext.close(true)
       }
     }
 
@@ -464,15 +465,6 @@ public final class EvaluatorImpl implements Evaluator {
     }
   }
 
-  private boolean isPklBug(VmStackOverflowException e) {
-    // There's no good way to tell if a StackOverflowError came from Pkl, or from our
-    // implementation.
-    // This is a simple heuristic; it's pretty likely that any stack overflow error that occurs
-    // if there's less than 100 truffle frames is due to our own doing.
-    var truffleStackTraceElements = TruffleStackTrace.getStackTrace(e);
-    return truffleStackTraceElements != null && truffleStackTraceElements.size() < 100;
-  }
-
   // ScheduledFuture.cancel() is problematic, so let's handle cancellation on our own
   private final class TimeoutTask implements Runnable {
     // both fields guarded by synchronizing on `this`

pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java

@@ -31,6 +31,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import org.graalvm.collections.EconomicMap;
 import org.pkl.core.CommandSpec;
@@ -45,6 +46,7 @@ import org.pkl.core.PNull;
 import org.pkl.core.PklBugException;
 import org.pkl.core.SecurityManager;
 import org.pkl.core.SecurityManagerException;
+import org.pkl.core.StackFrameTransformer;
 import org.pkl.core.ast.ExpressionNode;
 import org.pkl.core.ast.VmModifier;
 import org.pkl.core.ast.expression.literal.AmendModuleNodeGen;
@@ -71,14 +73,20 @@ public final class CommandSpecParser {
 
   private final ModuleResolver moduleResolver;
   private final SecurityManager securityManager;
+  private final StackFrameTransformer frameTransformer;
+  private final boolean color;
   private final Function<VmTyped, FileOutput> makeFileOutput;
 
   public CommandSpecParser(
       ModuleResolver moduleResolver,
       SecurityManager securityManager,
+      StackFrameTransformer frameTransformer,
+      boolean color,
       Function<VmTyped, FileOutput> makeFileOutput) {
     this.moduleResolver = moduleResolver;
     this.securityManager = securityManager;
+    this.frameTransformer = frameTransformer;
+    this.color = color;
     this.makeFileOutput = makeFileOutput;
   }
 
@@ -462,24 +470,13 @@ public final class CommandSpecParser {
           annotation == null
               ? null
               : VmUtils.readMember(annotation, Identifier.CONVERT) instanceof VmFunction func
-                  ? (rawValue, workingDirUri) -> {
+                  ? (rawValue, workingDirUri) ->
-                    try {
+                      handleErrors(() -> handleImports(func.apply(rawValue), workingDirUri))
-                      return handleImports(func.apply(rawValue), workingDirUri);
-                    } catch (VmException e) {
-                      throw new BadValue(e.getMessage());
-                    }
-                  }
                   : null,
           annotation == null
               ? null
               : VmUtils.readMember(annotation, Identifier.TRANSFORM_ALL) instanceof VmFunction func
-                  ? (it) -> {
+                  ? (it) -> handleErrors(() -> func.apply(VmList.create(it)))
-                    try {
-                      return func.apply(VmList.create(it));
-                    } catch (VmException e) {
-                      throw new BadValue(e.getMessage());
-                    }
-                  }
                   : null,
           annotation == null
               ? null
@@ -1064,6 +1061,31 @@ public final class CommandSpecParser {
         && vmTyped.getVmClass() == CommandModule.getImportClass();
   }
 
+  // handle errors from convert/transformAll and correctly format them for the CLI
+  private Object handleErrors(Supplier<Object> f) {
+    try {
+      try {
+        return f.get();
+      } catch (VmStackOverflowException e) {
+        if (VmUtils.isPklBug(e)) {
+          throw new VmExceptionBuilder()
+              .bug("Stack overflow")
+              .withCause(e.getCause())
+              .build()
+              .toPklException(frameTransformer, color);
+        }
+        throw e.toPklException(frameTransformer, color);
+      } catch (VmException e) {
+        throw e.toPklException(frameTransformer, color);
+      } catch (Exception e) {
+        throw new PklBugException(e);
+      }
+    } catch (Throwable e) {
+      // add a newline so this prints nicely under "Error: invalid value for <name>:"
+      throw new BadValue("\n" + e.getMessage());
+    }
+  }
+
   // for convert handle imports by replace Command.Import values
   // with imported module or Mapping<String, Module> values
   // Command.Import instances in returned Pair, List, Set, or Map values are replaced as well

pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java

@@ -20,6 +20,7 @@ import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.Truffle;
 import com.oracle.truffle.api.TruffleLanguage;
+import com.oracle.truffle.api.TruffleStackTrace;
 import com.oracle.truffle.api.frame.*;
 import com.oracle.truffle.api.nodes.*;
 import com.oracle.truffle.api.source.Source;
@@ -975,4 +976,13 @@ public final class VmUtils {
     }
     return value;
   }
+
+  public static boolean isPklBug(VmStackOverflowException e) {
+    // There's no good way to tell if a StackOverflowError came from Pkl, or from our
+    // implementation.
+    // This is a simple heuristic; it's pretty likely that any stack overflow error that occurs
+    // if there's less than 100 truffle frames is due to our own doing.
+    var truffleStackTraceElements = TruffleStackTrace.getStackTrace(e);
+    return truffleStackTraceElements != null && truffleStackTraceElements.size() < 100;
+  }
 }