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

@@ -563,7 +563,7 @@ Relative URIs are resolved against the working directory.
 <command options>::
 Additional CLI options and arguments defined by `<module>`.
 
-This command also takes <<common-options, common options>>, but they must be specified before `<module>`. 
+This command also takes <<common-options, common options>>.
 
 [[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 (`-<short name>`).
-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 <<common-options,common options>>.
+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:
 

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<String>,
+  private val reservedFlagShortNames: Set<String>,
   private val args: List<String>,
   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()

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
+  private val showHelp by option("-h", "--help", help = "Show this message and exit").flag()
-    argument(name = "module", completionCandidates = CompletionCandidates.Path).convert {
+
-      BaseOptions.parseModuleName(it)
+  val module: URI? by
-    }
+    argument(name = "module", completionCandidates = CompletionCandidates.Path)
+      .convert { BaseOptions.parseModuleName(it) }
+      .optional()
 
   val args: List<String> 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()
   }
 }

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

@@ -85,7 +85,15 @@ class CliCommandRunnerTest {
 
   private fun runToStdout(options: CliBaseOptions, args: List<String>): 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(

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<Pattern> by
     option(

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};
     }
   }
 

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<CommandSpec> run);
+  void evaluateCommand(
+      ModuleSource moduleSource,
+      Set<String> reservedFlagNames,
+      Set<String> reservedFlagShortNames,
+      Consumer<CommandSpec> run);
 
   /**
    * Releases all resources held by this evaluator. If an {@code evaluate} method is currently

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<CommandSpec> run) {
+  public void evaluateCommand(
+      ModuleSource moduleSource,
+      Set<String> reservedFlagNames,
+      Set<String> reservedFlagShortNames,
+      Consumer<CommandSpec> 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;

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<String> reservedFlagNames;
+  private final Set<String> reservedFlagShortNames;
   private final Function<VmTyped, FileOutput> makeFileOutput;
 
   public CommandSpecParser(
@@ -82,11 +84,15 @@ public final class CommandSpecParser {
       SecurityManager securityManager,
       StackFrameTransformer frameTransformer,
       boolean color,
+      Set<String> reservedFlagNames,
+      Set<String> reservedFlagShortNames,
       Function<VmTyped, FileOutput> 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)) {
+    for (var reserved : reservedFlagNames) {
-      throw exceptionBuilder()
+      if (reserved.equals(name)) {
-          .withSourceSection(prop.getHeaderSection())
+        throw exceptionBuilder()
-          .evalError("commandFlagHelpCollision", prop.getName())
+            .withSourceSection(prop.getHeaderSection())
-          .build();
+            .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();
+      }
     }
   }
 

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=\
+commandFlagNameCollision=\
-Flag option `{0}` may not have name "help" or short name "h".
+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\

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<PklException> { parse(moduleUri) }
     assertThat(exc.message).contains("help: Boolean")
-    assertThat(exc.message)
+    assertThat(exc.message).contains("Flag option `help` name collides with a reserved flag name.")
-      .contains("Flag option `help` may not have name \"help\" or short name \"h\".")
   }
 
   @Test
@@ -462,7 +461,27 @@ class CommandSpecParserTest {
     val exc = assertThrows<PklException> { 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<PklException> { 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