From a5dc91f0a53b22f821240013e0980139d347a778 Mon Sep 17 00:00:00 2001 From: Jen Basch Date: Fri, 20 Feb 2026 12:00:18 -0800 Subject: [PATCH] Command flag behavior improvements (#1432) * Forbid overlap of built-in and command-defined flag names * Allow interleaving built-in and command-defined flags on the command line * List abbreviated flag names first, matching the behavior of built-in flags --- docs/modules/pkl-cli/pages/index.adoc | 13 +++++- .../kotlin/org/pkl/cli/CliCommandRunner.kt | 10 ++++- .../kotlin/org/pkl/cli/commands/RunCommand.kt | 44 ++++++++++++++++--- .../org/pkl/cli/CliCommandRunnerTest.kt | 16 +++++-- .../pkl/commons/cli/commands/BaseOptions.kt | 12 ++++- .../main/java/org/pkl/core/CommandSpec.java | 6 +-- .../src/main/java/org/pkl/core/Evaluator.java | 7 ++- .../main/java/org/pkl/core/EvaluatorImpl.java | 9 +++- .../pkl/core/runtime/CommandSpecParser.java | 27 +++++++++--- .../org/pkl/core/errorMessages.properties | 6 ++- .../pkl/core/runtime/CommandSpecParserTest.kt | 27 ++++++++++-- 11 files changed, 146 insertions(+), 31 deletions(-) diff --git a/docs/modules/pkl-cli/pages/index.adoc b/docs/modules/pkl-cli/pages/index.adoc index 346b4917..d0a8f2e9 100644 --- a/docs/modules/pkl-cli/pages/index.adoc +++ b/docs/modules/pkl-cli/pages/index.adoc @@ -563,7 +563,7 @@ Relative URIs are resolved against the working directory. :: Additional CLI options and arguments defined by ``. -This command also takes <>, but they must be specified before ``. +This command also takes <>. [[command-repl]] === `pkl repl` @@ -999,7 +999,16 @@ Properties may be xref:language-reference:index.adoc#annotations[annotated] to i * Properties with no annotation are treated the same as `@Flag` with no further customization. Flag options may set a `shortName` property to define a single-character abbreviation (`-`). -Flag abbreviations may be combined (e.g. `-a -b -v -v -f some-value` is equivalent to `-abvvf some-value`). +Flag abbreviations may be combined (e.g. `-a -b -v -v -q some-value` is equivalent to `-abvvq some-value`). + +[CAUTION] +==== +Flag names and short names may not conflict with <>. +Future versions of Pkl may introduce additional common options and the names of these options will become forbidden for use in `pkl:Command`. +Thus, any Pkl release that adds common options may introduce breaking changes for commands. + +While unfortunate, this behavior eliminates potentially dangerous or misleading ambiguities between Pkl-defined and user-defined options. +==== A `@Flag` or `@Argument` property's type annotation determines how it is converted from the raw string value: diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliCommandRunner.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliCommandRunner.kt index 69e0d68a..58d743d1 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliCommandRunner.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/CliCommandRunner.kt @@ -45,6 +45,8 @@ class CliCommandRunner @JvmOverloads constructor( private val options: CliBaseOptions, + private val reservedFlagNames: Set, + private val reservedFlagShortNames: Set, private val args: List, private val outputStream: OutputStream = System.out, private val errStream: OutputStream = System.err, @@ -65,7 +67,11 @@ constructor( private fun evalCmd(builder: EvaluatorBuilder) { val evaluator = builder.build() evaluator.use { - evaluator.evaluateCommand(uri(normalizedSourceModule)) { spec -> + evaluator.evaluateCommand( + uri(normalizedSourceModule), + reservedFlagNames, + reservedFlagShortNames, + ) { spec -> try { val root = SynthesizedRunCommand(spec, this, options.sourceModules.first().toString()) root.installCommonOptions(includeVersion = false) @@ -233,7 +239,7 @@ constructor( .mapNotNull { val opt = it as? OptionWithValues<*, *, *> ?: return@mapNotNull null return@mapNotNull if (it.names.contains("--help")) null - else it.names.first().trimStart('-') to opt.value + else it.names.last().trimStart('-') to opt.value } .toMap() + registeredArguments() diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/commands/RunCommand.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/commands/RunCommand.kt index 64c3fdaa..56d6a7cd 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/commands/RunCommand.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/commands/RunCommand.kt @@ -16,11 +16,16 @@ package org.pkl.cli.commands import com.github.ajalt.clikt.completion.CompletionCandidates +import com.github.ajalt.clikt.core.MissingArgument +import com.github.ajalt.clikt.core.PrintHelpMessage import com.github.ajalt.clikt.core.context import com.github.ajalt.clikt.parameters.arguments.argument import com.github.ajalt.clikt.parameters.arguments.convert import com.github.ajalt.clikt.parameters.arguments.multiple +import com.github.ajalt.clikt.parameters.arguments.optional import com.github.ajalt.clikt.parameters.groups.provideDelegate +import com.github.ajalt.clikt.parameters.options.flag +import com.github.ajalt.clikt.parameters.options.option import java.net.URI import org.pkl.cli.CliCommandRunner import org.pkl.commons.cli.commands.BaseCommand @@ -32,19 +37,46 @@ class RunCommand : BaseCommand(name = "run", helpLink = helpLink) { override val treatUnknownOptionsAsArgs = true init { - context { allowInterspersedArgs = false } + context { + // override clikt's built-in help behavior + // the built-in --help is eager so any --help/-h would force printing of pkl run's help + // which is not desired when a command module (or any of its subcommands) are present + // since that would mean command-defined help is gated behind a non-obvious `-- --help` + helpOptionNames = emptySet() + } } - val module: URI by - argument(name = "module", completionCandidates = CompletionCandidates.Path).convert { - BaseOptions.parseModuleName(it) - } + private val showHelp by option("-h", "--help", help = "Show this message and exit").flag() + + val module: URI? by + argument(name = "module", completionCandidates = CompletionCandidates.Path) + .convert { BaseOptions.parseModuleName(it) } + .optional() val args: List by argument(name = "args").multiple() private val projectOptions by ProjectOptions() override fun run() { - CliCommandRunner(baseOptions.baseOptions(listOf(module), projectOptions), args).run() + // if no module is specified but --help is show help, otherwise error becuase module is missing + if (module == null) + if (showHelp) throw PrintHelpMessage(currentContext) + else throw MissingArgument(registeredArguments().find { it.name == "module" }!!) + + val reservedFlagNames = mutableSetOf("help") + val reservedFlagShortNames = mutableSetOf("h") + registeredOptions().forEach { opt -> + (opt.names + opt.secondaryNames).forEach { + if (it.startsWith("--")) reservedFlagNames.add(it.trimStart('-')) + else reservedFlagShortNames.add(it.trimStart('-')) + } + } + CliCommandRunner( + baseOptions.baseOptions(listOf(module!!), projectOptions), + reservedFlagNames, + reservedFlagShortNames, + if (showHelp) args + listOf("--help") else args, + ) + .run() } } 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 fa480907..dd1d2aa1 100644 --- a/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt +++ b/pkl-cli/src/test/kotlin/org/pkl/cli/CliCommandRunnerTest.kt @@ -85,7 +85,15 @@ class CliCommandRunnerTest { private fun runToStdout(options: CliBaseOptions, args: List): String { val outWriter = ByteArrayOutputStream() - CliCommandRunner(options, args, outWriter, ByteArrayOutputStream()).run() + CliCommandRunner( + options, + setOf("root-dir"), + emptySet(), + args, + outWriter, + ByteArrayOutputStream(), + ) + .run() return outWriter.toString(StandardCharsets.UTF_8) } @@ -1071,9 +1079,9 @@ class CliCommandRunnerTest { int16: Int16 @CountedFlag { shortName = "d" } int32: Int32 - @CountedFlag { shortName = "e" } + @CountedFlag { shortName = "x" } uint: UInt - @CountedFlag { shortName = "f" } + @CountedFlag { shortName = "y" } uint8: UInt8 @CountedFlag { shortName = "g" } uint16: UInt16 @@ -1087,7 +1095,7 @@ class CliCommandRunnerTest { val output = runToStdout( CliBaseOptions(sourceModules = listOf(moduleUri)), - listOf("-abbcccddddeeeeeffffffgggggggiiiiiiii"), + listOf("-abbcccddddxxxxxyyyyyygggggggiiiiiiii"), ) assertThat(output) .isEqualTo( diff --git a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt index 0a32dab7..bb17c918 100644 --- a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt +++ b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt @@ -98,7 +98,17 @@ class BaseOptions : OptionGroup() { private val defaults = CliBaseOptions() private val output = - arrayOf("json", "jsonnet", "pcf", "properties", "plist", "textproto", "xml", "yaml") + arrayOf( + "json", + "jsonnet", + "pcf", + "properties", + "plist", + "textproto", + "xml", + "yaml", + "pkl-binary", + ) val allowedModules: List by option( diff --git a/pkl-core/src/main/java/org/pkl/core/CommandSpec.java b/pkl-core/src/main/java/org/pkl/core/CommandSpec.java index 604343e1..3fafa0be 100644 --- a/pkl-core/src/main/java/org/pkl/core/CommandSpec.java +++ b/pkl-core/src/main/java/org/pkl/core/CommandSpec.java @@ -100,7 +100,7 @@ public record CommandSpec( public String[] getNames() { return shortName == null ? new String[] {"--" + name} - : new String[] {"--" + name, "-" + shortName}; + : new String[] {"-" + shortName, "--" + name}; } } @@ -115,7 +115,7 @@ public record CommandSpec( public String[] getNames() { return shortName == null ? new String[] {"--" + name} - : new String[] {"--" + name, "-" + shortName}; + : new String[] {"-" + shortName, "--" + name}; } } @@ -126,7 +126,7 @@ public record CommandSpec( public String[] getNames() { return shortName == null ? new String[] {"--" + name} - : new String[] {"--" + name, "-" + shortName}; + : new String[] {"-" + shortName, "--" + name}; } } diff --git a/pkl-core/src/main/java/org/pkl/core/Evaluator.java b/pkl-core/src/main/java/org/pkl/core/Evaluator.java index b3c35701..137a4826 100644 --- a/pkl-core/src/main/java/org/pkl/core/Evaluator.java +++ b/pkl-core/src/main/java/org/pkl/core/Evaluator.java @@ -16,6 +16,7 @@ package org.pkl.core; import java.util.Map; +import java.util.Set; import java.util.function.Consumer; import org.pkl.core.runtime.VmEvalException; @@ -237,7 +238,11 @@ public interface Evaluator extends AutoCloseable { * @throws PklException if an error occurs during evaluation * @throws IllegalStateException if this evaluator has already been closed */ - void evaluateCommand(ModuleSource moduleSource, Consumer run); + void evaluateCommand( + ModuleSource moduleSource, + Set reservedFlagNames, + Set reservedFlagShortNames, + Consumer run); /** * Releases all resources held by this evaluator. If an {@code evaluate} method is currently 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 9ebacb1c..74ce01e1 100644 --- a/pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java +++ b/pkl-core/src/main/java/org/pkl/core/EvaluatorImpl.java @@ -20,6 +20,7 @@ import java.nio.file.Path; import java.time.Duration; import java.util.Collection; import java.util.Map; +import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -277,7 +278,11 @@ public final class EvaluatorImpl implements Evaluator { } @Override - public void evaluateCommand(ModuleSource moduleSource, Consumer run) { + public void evaluateCommand( + ModuleSource moduleSource, + Set reservedFlagNames, + Set reservedFlagShortNames, + Consumer run) { doEvaluate( moduleSource, (module) -> { @@ -287,6 +292,8 @@ public final class EvaluatorImpl implements Evaluator { securityManager, frameTransformer, color, + reservedFlagNames, + reservedFlagShortNames, (fileOutput) -> new FileOutputImpl(this, fileOutput)); run.accept(commandRunner.parse(module)); return null; 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 b4b0e0ff..d149bde3 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 @@ -75,6 +75,8 @@ public final class CommandSpecParser { private final SecurityManager securityManager; private final StackFrameTransformer frameTransformer; private final boolean color; + private final Set reservedFlagNames; + private final Set reservedFlagShortNames; private final Function makeFileOutput; public CommandSpecParser( @@ -82,11 +84,15 @@ public final class CommandSpecParser { SecurityManager securityManager, StackFrameTransformer frameTransformer, boolean color, + Set reservedFlagNames, + Set reservedFlagShortNames, Function makeFileOutput) { this.moduleResolver = moduleResolver; this.securityManager = securityManager; this.frameTransformer = frameTransformer; this.color = color; + this.reservedFlagNames = reservedFlagNames; + this.reservedFlagShortNames = reservedFlagShortNames; this.makeFileOutput = makeFileOutput; } @@ -249,11 +255,22 @@ public final class CommandSpecParser { } private void checkFlagNames(ClassProperty prop, String name, @Nullable String shortName) { - if ("help".equals(name) || "h".equals(shortName)) { - throw exceptionBuilder() - .withSourceSection(prop.getHeaderSection()) - .evalError("commandFlagHelpCollision", prop.getName()) - .build(); + for (var reserved : reservedFlagNames) { + if (reserved.equals(name)) { + throw exceptionBuilder() + .withSourceSection(prop.getHeaderSection()) + .evalError("commandFlagNameCollision", prop.getName(), "name", "") + .build(); + } + } + for (var reserved : reservedFlagShortNames) { + if (reserved.equals(shortName)) { + throw exceptionBuilder() + .withSourceSection(prop.getHeaderSection()) + .evalError( + "commandFlagNameCollision", prop.getName(), "short name", "`" + shortName + "` ") + .build(); + } } } diff --git a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties index 6d77fa46..f0b13cbb 100644 --- a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties +++ b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties @@ -1121,8 +1121,10 @@ More than one repeated option annotated with `@Argument` found: `{0}` and `{1}`. \n\ Only one repeated argument is permitted per command. -commandFlagHelpCollision=\ -Flag option `{0}` may not have name "help" or short name "h". +commandFlagNameCollision=\ +Flag option `{0}` {1} {2}collides with a reserved flag {1}.\n\ +\n\ +Option {1}s must not overlap with built-in options. commandFlagInvalidType=\ Option `{0}` with annotation `@{1}` has invalid type `{2}`.\n\ diff --git a/pkl-core/src/test/kotlin/org/pkl/core/runtime/CommandSpecParserTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/runtime/CommandSpecParserTest.kt index bfd66aa3..ce30df00 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/runtime/CommandSpecParserTest.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/runtime/CommandSpecParserTest.kt @@ -57,7 +57,7 @@ class CommandSpecParserTest { private fun parse(moduleUri: URI): CommandSpec { var spec: CommandSpec? = null - evaluator.evaluateCommand(uri(moduleUri)) { spec = it } + evaluator.evaluateCommand(uri(moduleUri), setOf("help", "root-dir"), setOf("h")) { spec = it } return spec!! } @@ -440,8 +440,7 @@ class CommandSpecParserTest { val exc = assertThrows { parse(moduleUri) } assertThat(exc.message).contains("help: Boolean") - assertThat(exc.message) - .contains("Flag option `help` may not have name \"help\" or short name \"h\".") + assertThat(exc.message).contains("Flag option `help` name collides with a reserved flag name.") } @Test @@ -462,7 +461,27 @@ class CommandSpecParserTest { val exc = assertThrows { parse(moduleUri) } assertThat(exc.message).contains("showHelp: Boolean") assertThat(exc.message) - .contains("Flag option `showHelp` may not have name \"help\" or short name \"h\".") + .contains("Flag option `showHelp` short name `h` collides with a reserved flag short name.") + } + + @Test + fun `flag with collision on reserved option name`() { + val moduleUri = + writePklFile( + "cmd.pkl", + renderOptions + + """ + class Options { + `root-dir`: String + } + """ + .trimIndent(), + ) + + val exc = assertThrows { parse(moduleUri) } + assertThat(exc.message).contains("`root-dir`: String") + assertThat(exc.message) + .contains("Flag option `root-dir` name collides with a reserved flag name.") } @Test