From 12915f520f85a87e442b0b8c1dc973443d2609ac Mon Sep 17 00:00:00 2001 From: Jen Basch Date: Tue, 24 Feb 2026 08:09:25 -0800 Subject: [PATCH] Fix command error and import handling (#1436) --- .../org/pkl/cli/CliCommandRunnerTest.kt | 30 +++++++++-- .../org/pkl/commons/cli/CliCommandTest.kt | 2 +- .../pkl/core/runtime/CommandSpecParser.java | 53 ++++++++++--------- 3 files changed, 55 insertions(+), 30 deletions(-) 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 dd1d2aa1..8356c13c 100644 --- a/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt +++ b/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt @@ -819,14 +819,25 @@ class CliCommandRunnerTest { val moduleUri = writePklFile( "cmd.pkl", - renderOptions + - """ + """ + extends "pkl:Command" + + options: Options + + output { + value = (options) { + fromImport { + baz = true // assert that imported modules are not forced + } + } + } + class Options { @Argument { convert = (it) -> new Import{ uri = it } } fromImport: Module } """ - .trimIndent(), + .trimIndent(), ) val importUri = @@ -835,6 +846,7 @@ class CliCommandRunnerTest { """ foo = 1 bar = "baz" + baz: Boolean """ .trimIndent(), ) @@ -847,6 +859,7 @@ class CliCommandRunnerTest { fromImport { foo = 1 bar = "baz" + baz = true } """ @@ -866,7 +879,13 @@ class CliCommandRunnerTest { options: Options output { - value = options + value = (options) { + fromGlobImport { + [[true]] { + baz = true // assert that imported modules are not forced + } + } + } } class Options { @@ -883,6 +902,7 @@ class CliCommandRunnerTest { """ foo: Int bar: String + baz: Boolean """ .trimIndent(), ) @@ -919,10 +939,12 @@ class CliCommandRunnerTest { ["file://glob1.pkl"] { foo = 1 bar = "baz" + baz = true } ["file://glob2.pkl"] { foo = 2 bar = "qux" + baz = true } } diff --git a/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt b/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt index 9fd07c6e..61e9f150 100644 --- a/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt +++ b/pkl-commons-cli/src/test/kotlin/org/pkl/commons/cli/CliCommandTest.kt @@ -148,7 +148,7 @@ class CliCommandTest { amends "pkl:Project" dependencies { - ["foo"] = import("$depDir/PklProject") + ["foo"] = import("${depDir.toUri().resolve("PklProject")}") } """ .trimIndent() 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 d149bde3..6e25d529 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 @@ -126,7 +126,7 @@ public final class CommandSpecParser { // instance of CommandSpec.State previously returned from a prior invocation // of CommandSpec.apply. parent == null ? null : (SubcommandState) parent.contents()), - (it) -> evaluateResult(command, (SubcommandState) it))); + (it) -> handleErrors(() -> evaluateResult(command, (SubcommandState) it)))); } private List collectSubcommands(VmTyped commandInfo) { @@ -488,12 +488,12 @@ public final class CommandSpecParser { ? null : VmUtils.readMember(annotation, Identifier.CONVERT) instanceof VmFunction func ? (rawValue, workingDirUri) -> - handleErrors(() -> handleImports(func.apply(rawValue), workingDirUri)) + handleBadValue(() -> handleImports(func.apply(rawValue), workingDirUri)) : null, annotation == null ? null : VmUtils.readMember(annotation, Identifier.TRANSFORM_ALL) instanceof VmFunction func - ? (it) -> handleErrors(() -> func.apply(VmList.create(it))) + ? (it) -> handleBadValue(() -> func.apply(VmList.create(it))) : null, annotation == null ? null @@ -957,7 +957,6 @@ public final class CommandSpecParser { private SubcommandState buildExecutionModule( VmTyped module, VmTyped options, @Nullable SubcommandState parent) { EconomicMap properties = EconomicMaps.create(parent != null ? 2 : 1); - options.force(false, true); properties.put( Identifier.OPTIONS, VmUtils.createSyntheticObjectProperty(Identifier.OPTIONS, "", options)); @@ -1079,31 +1078,35 @@ public final class CommandSpecParser { } // handle errors from convert/transformAll and correctly format them for the CLI - private Object handleErrors(Supplier f) { + private Object handleBadValue(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); - } + return handleErrors(f); } 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 + private T handleErrors(Supplier f) { + 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); + } + } + + // for convert, handle imports by replacing Command.Import values // with imported module or Mapping values // Command.Import instances in returned Pair, List, Set, or Map values are replaced as well // other types or nested instances of the above are not affected @@ -1158,13 +1161,13 @@ public final class CommandSpecParser { try { // Can't just use URI constructor, because URI(null, null, "C:/foo/bar", null) turns // into `URI("C", null, "/foo/bar", null)`. - var modulePath = Path.of(moduleName); + @SuppressWarnings("DuplicateExpressions") var uri = IoUtils.isUriLike(moduleName) ? new URI(moduleName) : IoUtils.isWindowsAbsolutePath(moduleName) - ? modulePath.toUri() - : new URI(null, null, IoUtils.toNormalizedPathString(modulePath), null); + ? Path.of(moduleName).toUri() + : new URI(null, null, IoUtils.toNormalizedPathString(Path.of(moduleName)), null); uriString = uri.isAbsolute() ? uri.toString() : IoUtils.resolve(workingDirUri, uri).toString(); } catch (URISyntaxException e) {