Improve support for recursive lazy paraemeters (#474)

Allow lazy options and arguments to reference parameters from groups, and allow unlimited recursion.

The way we support parameters referencing other parameters is by collecting any parameters that
failed to finalize and finalizing them again after all other parameters have succeeded.

Prior to this PR, we finalized options, groups, and arguments in separate passes. These passes were
interleaved and carefully ordered to work correctly.

This PR does away with the manual pass ordering and instead finalizes all parameters repeatedly,
iterating until a fixed point is reached.

.github/workflows/gradle-wrapper-validation.yml

@@ -11,5 +11,5 @@ jobs:
     name: "Validation"
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: gradle/wrapper-validation-action@v1

.github/workflows/publish.yml

@@ -24,7 +24,7 @@ jobs:
             TEST_TASK: check
     runs-on: ${{matrix.os}}
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Cache Kotlin Native Compiler
         uses: actions/cache@v3
         with:
@@ -52,7 +52,7 @@ jobs:
     runs-on: macos-latest
     if: github.repository == 'ajalt/clikt'
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Fetch git tags
         run: git fetch origin +refs/tags/*:refs/tags/*
       - name: Cache Kotlin Native Compiler

.github/workflows/test.yml

@@ -20,7 +20,7 @@ jobs:
             TEST_TASK: check
     runs-on: ${{matrix.os}}
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Cache Kotlin Native Compiler
         uses: actions/cache@v3
         with:

clikt/src/commonMain/kotlin/com/github/ajalt/clikt/core/CliktCommand.kt

@@ -62,8 +62,7 @@ abstract class CliktCommand(
     private val autoCompleteEnvvar: String? = "",
     /**
      * If true, allow multiple of this command's subcommands to be called sequentially. This will
-     * disable `allowInterspersedArgs` on the context of this command and its descendants. This
-     * functionality is experimental, and may change in a future release.
+     * disable `allowInterspersedArgs` on the context of this command and its descendants.
      */
     internal val allowMultipleSubcommands: Boolean = false,
     /**

clikt/src/commonMain/kotlin/com/github/ajalt/clikt/internal/Finalization.kt

@@ -4,6 +4,8 @@ import com.github.ajalt.clikt.core.Abort
 import com.github.ajalt.clikt.core.Context
 import com.github.ajalt.clikt.core.MultiUsageError
 import com.github.ajalt.clikt.core.UsageError
+import com.github.ajalt.clikt.parameters.arguments.Argument
+import com.github.ajalt.clikt.parameters.groups.ParameterGroup
 import com.github.ajalt.clikt.parameters.options.Option
 import com.github.ajalt.clikt.parsers.Invocation
 
@@ -12,45 +14,66 @@ internal fun finalizeOptions(
     options: List<Option>,
     invocationsByOption: Map<Option, List<Invocation>>,
 ) {
-    val errors = mutableListOf<UsageError>()
-    // Finalize invoked options
-    for ((option, invocations) in invocationsByOption) {
-        try {
-            option.finalize(context, invocations)
-        } catch (e: UsageError) {
-            errors += e
-            context.errorEncountered = true
-        }
+    finalizeParameters(
+        context, options, emptyList(), mapOf(null to invocationsByOption), emptyList()
+    )
+}
+
+private sealed class Param
+private class Opt(val option: Option, val invs: List<Invocation>) : Param()
+private class Arg(val argument: Argument, val invs: List<String>) : Param()
+private class Group(val group: ParameterGroup, val invs: Map<Option, List<Invocation>>) : Param()
+
+internal fun finalizeParameters(
+    context: Context,
+    options: List<Option>,
+    groups: List<ParameterGroup>,
+    invocationsByOptionByGroup: Map<ParameterGroup?, Map<Option, List<Invocation>>>,
+    argsAndTokens: List<Pair<Argument, List<String>>>,
+) {
+    // Add uninvoked params last so that e.g. we can skip prompting if there's an error in an
+    // invoked option
+    val allGroups = invocationsByOptionByGroup.toMutableMap()
+    for (group in groups) {
+        if (group !in allGroups) allGroups[group] = emptyMap()
     }
 
-    // Finalize uninvoked options
-    val retries = mutableListOf<Option>()
-    for (o in options.filter { it !in invocationsByOption }) {
-        try {
-            o.finalize(context, emptyList())
-        } catch (e: IllegalStateException) {
-            retries += o
-        } catch (e: UsageError) {
-            errors += e
-        } catch (e: Abort) {
-            // ignore Abort if we already encountered an error
-            if (!context.errorEncountered) throw e
-        }
+    val allOptions = (invocationsByOptionByGroup[null] ?: emptyMap()).toMutableMap()
+    for (opt in options) {
+        if (opt !in allOptions) allOptions[opt] = emptyList()
     }
 
-    // If an uninvoked option triggers an ISE, retry it once after other options have been finalized
-    // so that lazy defaults can reference other options.
-    for (o in retries) {
-        try {
-            o.finalize(context, emptyList())
-        } catch (e: IllegalStateException) {
-            // If we had an earlier usage error, throw that instead of the ISE
-            MultiUsageError.buildOrNull(errors)?.let { throw it }
-            throw e
-        } catch (e: UsageError) {
-            errors += e
-            context.errorEncountered = true
+    val allParams: List<Param> =
+        argsAndTokens.map { Arg(it.first, it.second) } +
+                allOptions.map { Opt(it.key, it.value) } +
+                allGroups.mapNotNull { it.key?.let { k -> Group(k, it.value) } }
+
+
+    val errors = mutableListOf<UsageError>()
+    var currentRound = allParams.toList()
+    val nextRound = mutableListOf<Param>()
+
+    while (true) {
+        for (it in currentRound) {
+            try {
+                when (it) {
+                    is Arg -> it.argument.finalize(context, it.invs)
+                    is Opt -> it.option.finalize(context, it.invs)
+                    is Group -> it.group.finalize(context, it.invs)
+                }
+            } catch (e: IllegalStateException) {
+                nextRound += it
+            } catch (e: UsageError) {
+                errors += e
+                context.errorEncountered = true
+            } catch (e: Abort) {
+                // ignore Abort if we already encountered an error
+                if (!context.errorEncountered) throw e
+            }
         }
+        if (currentRound.size <= nextRound.size) break
+        currentRound = nextRound.toList()
+        nextRound.clear()
     }
 
     MultiUsageError.buildOrNull(errors)?.let { throw it }

clikt/src/commonMain/kotlin/com/github/ajalt/clikt/parsers/Parser.kt

@@ -1,7 +1,7 @@
 package com.github.ajalt.clikt.parsers
 
 import com.github.ajalt.clikt.core.*
-import com.github.ajalt.clikt.internal.finalizeOptions
+import com.github.ajalt.clikt.internal.finalizeParameters
 import com.github.ajalt.clikt.parameters.arguments.Argument
 import com.github.ajalt.clikt.parameters.options.Option
 import com.github.ajalt.clikt.parameters.options.splitOptionPrefix
@@ -11,7 +11,8 @@ private data class Err(val e: UsageError, val i: Int, val includeInMulti: Boolea
 
 private data class ArgsParseResult(
     val excessCount: Int,
-    val args: Map<Argument, List<String>>,
+    /** All arguments in the command and their parsed tokens (if any) */
+    val args: List<Pair<Argument, List<String>>>,
     val err: Err?,
 )
 
@@ -242,48 +243,20 @@ internal object Parser {
 
                 i = excessResult.first
 
-                // Finalize arguments before options, so that options can reference them
-                val (retries, argErrs) = finalizeArguments(argsParseResult.args, context)
-                usageErrors += argErrs
-                if (argErrs.isNotEmpty()) context.errorEncountered = true
-
-                // Finalize un-grouped options
+                // Finalize arguments, groups, and options
                 gatherErrors(usageErrors, context) {
-                    finalizeOptions(
+                    finalizeParameters(
                         context,
                         ungroupedOptions,
-                        invocationsByOptionByGroup[null] ?: emptyMap()
+                        command._groups,
+                        invocationsByOptionByGroup,
+                        argsParseResult.args,
                     )
                 }
 
-                // Since groups like ChoiceGroup may depend on an ungrouped option's value, we can't finalize groups if
-                // any options failed to validate, so throw errors now
-                MultiUsageError.buildOrNull(usageErrors)?.let { throw it }
-                usageErrors.clear()
-
-                // Finalize groups after ungrouped options so that the groups can use their values
-                for ((group, invs) in invocationsByOptionByGroup) {
-                    gatherErrors(usageErrors, context) { group?.finalize(context, invs) }
-                }
-
-                // Finalize groups with no invocations
-                for (g in command._groups) {
-                    if (g !in invocationsByGroup) gatherErrors(usageErrors, context) {
-                        g.finalize(context, emptyMap())
-                    }
-                }
-
                 // We can't validate a param that didn't finalize successfully, and we don't keep
                 // track of which ones are finalized, so throw any errors now
                 MultiUsageError.buildOrNull(usageErrors)?.let { throw it }
-                usageErrors.clear()
-
-                // Retry any failed args now that they can reference option values
-                retries.forEach { (arg, v) ->
-                    gatherErrors(usageErrors, context) {
-                        arg.finalize(context, v)
-                    }
-                }
 
                 // Now that all parameters have been finalized, we can validate everything
                 ungroupedOptions.forEach { gatherErrors(usageErrors, context) { it.postValidate(context) } }
@@ -485,6 +458,7 @@ internal object Parser {
 
     private fun parseArguments(
         argvIndex: Int,
+        /** Parsed argument tokens and their index in argv */
         positionalArgs: List<Pair<Int, String>>,
         arguments: List<Argument>,
     ): ArgsParseResult {
@@ -512,7 +486,7 @@ internal object Parser {
                 }
                 return ArgsParseResult(
                     0,
-                    out,
+                    out.toList(),
                     Err(e, positionalArgs.getOrNull(i)?.first ?: argvIndex)
                 )
             }
@@ -522,33 +496,9 @@ internal object Parser {
         }
 
         val excess = positionalArgs.size - i
-        return ArgsParseResult(excess, out, null)
-    }
-
-    /** Returns map of arguments that need retries to their values */
-    private fun finalizeArguments(
-        parsedArgs: Map<Argument, List<String>>,
-        context: Context,
-    ): Pair<Map<Argument, List<String>>, List<UsageError>> {
-        val retries = mutableMapOf<Argument, List<String>>()
-        val usageErrors = mutableListOf<UsageError>()
-        for ((it, v) in parsedArgs) {
-            try {
-                it.finalize(context, v)
-            } catch (e: IllegalStateException) {
-                retries[it] = v
-            } catch (e: UsageError) {
-                usageErrors += e
-                context.errorEncountered = true
-            } catch (e: Abort) {
-                // ignore Abort if we already encountered an error
-                if (!context.errorEncountered) throw e
-            }
-        }
-        return retries to usageErrors
+        return ArgsParseResult(excess, out.toList(), null)
     }
 
-
     private fun loadArgFile(filename: String, context: Context): List<String> {
         return shlex(filename, context.argumentFileReader!!(filename), context)
     }

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/core/CliktCommandTest.kt

@@ -6,6 +6,7 @@ import com.github.ajalt.clikt.parameters.groups.OptionGroup
 import com.github.ajalt.clikt.parameters.groups.cooccurring
 import com.github.ajalt.clikt.parameters.groups.mutuallyExclusiveOptions
 import com.github.ajalt.clikt.parameters.groups.provideDelegate
+import com.github.ajalt.clikt.parameters.options.defaultLazy
 import com.github.ajalt.clikt.parameters.options.flag
 import com.github.ajalt.clikt.parameters.options.option
 import com.github.ajalt.clikt.parameters.options.required

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/parameters/ArgumentTest.kt

@@ -51,17 +51,15 @@ class ArgumentTest {
 
     @Test
     @JsName("one_default_argument")
-    fun `one default argument`() = forAll(
-        row("", "def")
-    ) { argv, expected ->
+    fun `one default argument`() {
         class C : TestCommand() {
             val x by argument().default("def")
             override fun run_() {
-                x shouldBe expected
+                x shouldBe "def"
             }
         }
 
-        C().parse(argv)
+        C().parse("")
     }
 
     @Test

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/parameters/OptionTest.kt

@@ -455,6 +455,19 @@ class OptionTest {
         C().parse("")
     }
 
+    @Test
+    @JsName("defaultLazy_option_mutual_recursion")
+    fun `defaultLazy option mutual recursion`() {
+        class C : TestCommand() {
+            val y: String by option().defaultLazy { x }
+            val x: String by option().defaultLazy { y }
+            override fun run_() {
+                y shouldBe "def"
+            }
+        }
+        shouldThrow<IllegalStateException> { C().parse("") }
+    }
+
     @Test
     @JsName("required_option")
     fun `required option`() {

clikt/src/commonTest/kotlin/com/github/ajalt/clikt/parameters/groups/OptionGroupsTest.kt

@@ -2,15 +2,13 @@
 
 package com.github.ajalt.clikt.parameters.groups
 
-import com.github.ajalt.clikt.core.BadParameterValue
-import com.github.ajalt.clikt.core.MissingOption
-import com.github.ajalt.clikt.core.MutuallyExclusiveGroupException
-import com.github.ajalt.clikt.core.UsageError
+import com.github.ajalt.clikt.core.*
 import com.github.ajalt.clikt.parameters.options.*
 import com.github.ajalt.clikt.parameters.types.int
 import com.github.ajalt.clikt.testing.TestCommand
 import com.github.ajalt.clikt.testing.formattedMessage
 import com.github.ajalt.clikt.testing.parse
+import com.github.ajalt.clikt.testing.test
 import io.kotest.assertions.throwables.shouldThrow
 import io.kotest.data.blocking.forAll
 import io.kotest.data.row
@@ -579,6 +577,21 @@ class OptionGroupsTest {
         shouldThrow<UsageError> { C(3).parse("--g=a --opt=3") }
         shouldThrow<UsageError> { C(3).parse("--opt=3") }
     }
+
+    @Test
+    @JsName("defaultLazy_option_referencing_group")
+    fun `defaultLazy option referencing group`() {
+        class C : TestCommand() {
+            val g by Group1()
+            val o by option().int().defaultLazy { g.g11 }
+
+            override fun run_() {
+                o shouldBe 1
+            }
+        }
+
+        C().parse("--g11=1")
+    }
 }
 
 private class Group1 : OptionGroup() {

docs/advanced.md

@@ -186,9 +186,11 @@ To implement git-style aliases:
     Committing with message: my message
     ```
 
-Note that aliases are not expanded recursively: none of the tokens that
-an alias expands to will be expanded again, even if they match another
-alias.
+!!! note
+
+    Aliases are not expanded recursively: none of the tokens that
+    an alias expands to will be expanded again, even if they match another
+    alias.
 
 You also use this functionality to implement command prefixes: