diff --git a/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt b/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt index e258062f..fa480907 100644 --- a/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt +++ b/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt @@ -942,7 +942,52 @@ class CliCommandRunnerTest { assertThrows { 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 { + 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 { + runToStdout(CliBaseOptions(sourceModules = listOf(moduleUri)), listOf("hi")) + } + assertThat(exc.message).contains("A stack overflow occurred.") } @Test diff --git a/pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java b/pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java index 7fde2747..9ebacb1c 100644 --- a/pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java +++ b/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` diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java b/pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java index d1562975..b4b0e0ff 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java +++ b/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 makeFileOutput; public CommandSpecParser( ModuleResolver moduleResolver, SecurityManager securityManager, + StackFrameTransformer frameTransformer, + boolean color, Function 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) -> { - try { - return handleImports(func.apply(rawValue), workingDirUri); - } catch (VmException e) { - throw new BadValue(e.getMessage()); - } - } + ? (rawValue, workingDirUri) -> + handleErrors(() -> handleImports(func.apply(rawValue), workingDirUri)) : null, annotation == null ? null : VmUtils.readMember(annotation, Identifier.TRANSFORM_ALL) instanceof VmFunction func - ? (it) -> { - try { - return func.apply(VmList.create(it)); - } catch (VmException e) { - throw new BadValue(e.getMessage()); - } - } + ? (it) -> handleErrors(() -> func.apply(VmList.create(it))) : 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 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 :" + throw new BadValue("\n" + e.getMessage()); + } + } + // for convert handle imports by replace Command.Import values // with imported module or Mapping values // Command.Import instances in returned Pair, List, Set, or Map values are replaced as well diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java index 0729007f..880be9d9 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmUtils.java +++ b/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; + } }