fix(derive): keep an invalid choice after an override displaces the flag

benches/gate/tests/differential.rs

@@ -25,6 +25,13 @@
 //! surfaced. The third was real and is fixed: `subcommand_required` was in the spec and no
 //! parser read it (#992).
 //!
+//! A later run found the second real one, and it pointed the other way: usage-argv accepted
+//! `mise --log-level=v --trace` because `--trace` declares `overrides --log-level`, and
+//! displacing a flag threw away the record of the word it had been given along with the
+//! value. usage-lib and clap both refused it. Pinned as
+//! `overrides-do-not-erase-an-invalid-choice` in the corpus and as
+//! `a_flag_displaced_by_an_override_still_reports_its_invalid_choice` below.
+//!
 //! Repeats are permissive in usage by default, including at the derive layer. A command that
 //! owns its entire grammar can opt into clap's rule with `args_override_self=false`:
 //!
@@ -521,6 +528,9 @@ fn clap_shadow_keeps_flag_requirements_from_the_shared_spec() {
 
 #[test]
 fn an_override_does_not_erase_an_invalid_choice() {
+    // `--index` is not declared on `tool-alias set` at all, so it falls through to a
+    // positional: what this pins is that a later *token* does not wash out the bad word.
+    // The `overrides` half of the name is the test below, which this one did not reach.
     let outcome = run(
         &SPEC,
         &[
@@ -535,6 +545,62 @@ fn an_override_does_not_erase_an_invalid_choice() {
     assert_eq!(outcome.clap, Verdict::InvalidChoice);
 }
 
+/// The proptest's finding, kept as a line rather than as a seed.
+///
+/// `mise github token --log-level=v --include-global --trace` came out of the generator
+/// reading `Accept` / refuse / `InvalidChoice`, which no arm of `explained` names. Shrunk by
+/// hand to the two tokens that matter: `--include-global` is undeclared here and falls
+/// through to `[HOST]`, and the command path is incidental — `--log-level` is global.
+///
+/// `--trace` declares `overrides --log-level`, and usage-argv resolved that by clearing the
+/// flag *and* the record of the word it had been given, so nothing was left to judge. The
+/// pair is what `overrides` settles; whether `v` is one of the five levels is not, and the
+/// other two parsers said so. Fixed in the derive's post-binding checks and pinned in the
+/// corpus as `overrides-do-not-erase-an-invalid-choice`.
+#[test]
+fn a_flag_displaced_by_an_override_still_reports_its_invalid_choice() {
+    for line in [
+        vec![
+            "github".to_string(),
+            "token".to_string(),
+            "--log-level=v".to_string(),
+            "--include-global".to_string(),
+            "--trace".to_string(),
+        ],
+        // The same disagreement with the command path and the stray flag taken away.
+        vec!["--log-level=v".to_string(), "--trace".to_string()],
+    ] {
+        let outcome = run(&SPEC, &line);
+        assert_eq!(
+            outcome.argv,
+            Verdict::InvalidChoice,
+            "mise {}: the override settles the pair, not the word",
+            line.join(" ")
+        );
+        assert!(!outcome.lib, "mise {}", line.join(" "));
+        assert_eq!(
+            outcome.clap,
+            Verdict::InvalidChoice,
+            "mise {}",
+            line.join(" ")
+        );
+    }
+
+    // The half that was already right, kept beside it: `overrides` holds between the two
+    // flags whichever order they arrive in, so neither order may swallow the bad word.
+    let outcome = run(&SPEC, &["--trace".to_string(), "--log-level=v".to_string()]);
+    assert_eq!(outcome.argv, Verdict::InvalidChoice);
+
+    // And a valid level is still displaced in silence — the fix does not turn `overrides`
+    // into a refusal.
+    let outcome = run(
+        &SPEC,
+        &["--log-level=info".to_string(), "--trace".to_string()],
+    );
+    assert_eq!(outcome.argv, Verdict::Accept);
+    assert!(outcome.lib);
+}
+
 #[test]
 fn a_bare_word_diverges_because_of_the_mount_not_the_parser() {
     // The first reading of this was "usage-argv refuses a lone `-` that clap accepts", filed as

conformance/tests/post_binding.rs

@@ -441,6 +441,12 @@ struct Ovr {
     /// Everything
     #[usage(long)]
     all: bool,
+    /// How much to log
+    #[usage(long, choices("debug", "info"), overrides = "--trace")]
+    level: Option<String>,
+    /// Log everything
+    #[usage(long)]
+    trace: bool,
 }
 
 #[test]
@@ -458,6 +464,43 @@ fn the_last_of_two_overriding_flags_wins() {
     assert!(!ovr.stdin, "displaced by the flag that came after it");
 }
 
+/// The corpus's `overrides-do-not-erase-an-invalid-choice`.
+///
+/// Found by the differential proptest over mise's spec, where
+/// `mise --log-level=v --trace` was accepted here and refused by usage-lib and by clap.
+/// The pair is settled by `overrides`; whether `v` is a word the flag accepts is not.
+#[test]
+fn an_override_does_not_erase_an_invalid_choice() {
+    let a = argv(["--level", "v", "--trace"]);
+    assert!(
+        matches!(
+            Ovr::parse_from(&a),
+            Err(usage_argv::Error::InvalidChoice { name: "level", .. })
+        ),
+        "the displaced flag's word was still never one of the choices"
+    );
+
+    // The other way round, where the flag carrying the bad word is the one that won.
+    let a = argv(["--trace", "--level", "v"]);
+    assert!(matches!(
+        Ovr::parse_from(&a),
+        Err(usage_argv::Error::InvalidChoice { name: "level", .. })
+    ));
+
+    // A word that *is* one of the choices is displaced in silence, as before.
+    let a = argv(["--level", "info", "--trace"]);
+    let ovr = Ovr::parse_from(&a).expect("should parse");
+    assert!(ovr.trace);
+    assert_eq!(ovr.level, None, "displaced by the flag that came after it");
+
+    // And a later occurrence of the flag itself *is* a correction: only that replaces the
+    // word, which is the rule `the_last_scalar_choice_controls_validation` pins.
+    let a = argv(["--level", "v", "--level", "info", "--trace"]);
+    let ovr = Ovr::parse_from(&a).expect("the second occurrence replaced the bad word");
+    assert!(ovr.trace);
+    assert_eq!(ovr.level, None);
+}
+
 #[test]
 fn a_later_override_clears_an_earlier_duplicate() {
     let a = argv(["--file", "a", "--file", "b", "--stdin"]);

corpus/08-choices-and-required.json

@@ -150,6 +150,16 @@
       },
       "layer": "post-binding"
     },
+    {
+      "id": "overrides-do-not-erase-an-invalid-choice",
+      "doc": "`--trace` came last, so `--log-level` lost and holds nothing by the end. That settles which of the two is in effect; it does not make `v` one of the choices. Only a later occurrence of `--log-level` itself can replace the word it was given — a different flag winning the pair is not a correction of what was typed.",
+      "spec": "name \"ex\"\nbin \"ex\"\nflag \"--log-level <level>\" overrides=\"--trace\" {\n  choices \"debug\" \"info\"\n}\nflag \"--trace\"\n",
+      "argv": ["--log-level", "v", "--trace"],
+      "expect": {
+        "error": "invalid_choice"
+      },
+      "layer": "post-binding"
+    },
     {
       "id": "conflicts-both-given",
       "doc": "Two flags declared to conflict cannot be given together. Unlike `overrides`, where the last one wins, a conflict is a mistake to report rather than an order to resolve.",

derive/src/codegen.rs

@@ -9283,19 +9283,27 @@ fn post_binding(cli: &Cli) -> TokenStream {
         let standing_only = unless_standing_only(f);
         let given = format_ident!("__given_{}", ident);
         Some(quote! {
+            // Asked outside the `given` guard below, because a flag that lost an `overrides`
+            // is no longer given and its value has gone back to its default — so the loop has
+            // nothing left to look at, and without this the word would go unreported.
+            // `overrides` settles which of two flags is in effect; it does not make a word
+            // that was never one of the choices into one. usage-lib and clap both refuse
+            // `--log-level=v --trace` for that reason, and mise's spec is where the
+            // divergence showed up. A repeat of the *same* flag clears this ledger itself,
+            // so what reaches here is a word nothing later replaced.
+            if #active && partial.#invalid {
+                return ::std::result::Result::Err(
+                    usage_argv::Error::InvalidChoice {
+                        name: #name,
+                        choices: #choices,
+                    },
+                );
+            }
             // Choices validate lexical input. A typed computed default is already a value of
             // the field's type; its `Display` form exists only so the partial can carry it to
             // the final conversion and need not itself be one of the advertised CLI words.
             // Environment values mark the field given, so they still take this path.
             if #active && partial.#given #standing_only {
-                if partial.#invalid {
-                    return ::std::result::Result::Err(
-                        usage_argv::Error::InvalidChoice {
-                            name: #name,
-                            choices: #choices,
-                        },
-                    );
-                }
                 for value in #values {
                     // Compared as text, since a choice is a word.
                 //

docs/go/generated-code.md

@@ -111,7 +111,9 @@ command's struct is filled with no caller involvement.
 
 Three things `Parse` deliberately does **not** do:
 
-- **`overrides` is not applied.** If your spec uses it, call `argv.ApplyOverrides` yourself.
+- **`overrides` is not applied.** If your spec uses it, call `argv.ApplyOverrides` yourself,
+  then `argv.CheckDisplaced` on each key it reports: losing a pair unsets a flag, it does not
+  unsay the word the flag was given.
 - **Help and version are not printed** — they come back as `*argv.Error` with `CodeHelp` /
   `CodeVersion` for you to render ([Help and errors](/go/help)).
 - **No chain comes back with an error.** The renderers want the command chain; recover it with

docs/go/index.md

@@ -148,7 +148,9 @@ Worth knowing before you commit:
 
 - **`overrides` is not enforced by generated `Parse`.** `conflicts`, `required_if`, and
   `required_unless` are; a spec relying on last-one-wins `overrides` semantics needs to call
-  `argv.ApplyOverrides` itself.
+  `argv.ApplyOverrides` itself, and `argv.CheckDisplaced` on each key it reports — a flag that
+  lost is out of the running for `required`, `env`, and `default`, but the word it was typed is
+  still judged against its `choices`.
 - **Fields are `string`, `bool`, `[]string`, or `int` (for counts).** A spec says what a value is
   called, never what type it is — convert with [`argv.Int`, `argv.Duration`, etc.](/go/binding#typed-values)
 - **`complete` run-scripts, `config` nodes, `group`, `value_hint`, and `mount` are not carried

docs/rust/validation.md

@@ -39,7 +39,10 @@ max_retries: Option<u32>,
 
 `overrides` is the quiet sibling of `conflicts`: a later occurrence of one flag discards an
 earlier occurrence of the other instead of erroring — useful for `--json` / `--yaml` pairs where
-the last one typed should win.
+the last one typed should win. What it discards is the value, not the fact that a word was
+typed: a value outside the loser's `choices` is still an `InvalidChoice`, since `overrides`
+answers which flag is in effect and not whether the word was one that flag accepts. Only a
+later occurrence of the same flag replaces its value.
 
 ## Groups
 

docs/spec/reference/flag.md

@@ -157,6 +157,12 @@ mistake, and silently honouring one of them hides it.
 A conflict holds in either direction, so declaring it once is enough; it applies to flags
 that were actually given, not to defaults.
 
+Losing an override unsets a flag; it does not unsay the word it was given. `--log-level=v
+--trace` is still refused when `v` is outside `--log-level`'s
+[`choices`](/spec/reference/arg) — which
+of the two is in effect is what `overrides` settles, and only a later occurrence of
+`--log-level` itself replaces the word it was handed.
+
 For three or more flags that exclude each other, or for "one of these is required", a
 [`group`](/spec/reference/group) says in one node what `conflicts` says once per pair.
 

go/argv/post.go

@@ -311,6 +311,40 @@ func explicitRelationshipValues(m *Meta, values []string, source Source, negated
 	return values
 }
 
+// CheckDisplaced judges the words an entry was typed, and nothing else about it.
+//
+// It is the half of [Check] that still applies to a flag which lost an
+// `overrides`. Such a flag is out of the running for `required`, for `env` and
+// `default`, and for the rules that read one entry to judge another — but
+// `overrides` settles which of a pair is *in effect*, not whether the word the
+// loser was handed was ever one it accepts. `--log-level=v --trace` is refused
+// for that reason, and usage-lib and the Rust derives agree; the corpus pins it
+// as `overrides-do-not-erase-an-invalid-choice`.
+//
+// Pass the values the command line supplied, before any `env` or `default`
+// fallback: a loser is not filled from either, so there is nothing else to judge.
+func CheckDisplaced(m *Meta, values []string) *Error {
+	if m == nil {
+		return nil
+	}
+	accepted := m.AcceptedChoices
+	if len(accepted) == 0 {
+		accepted = m.Choices
+	}
+	if len(accepted) == 0 || m.AllowUnknownChoices {
+		return nil
+	}
+	// Every value, not just the first: a variadic can be given a good value
+	// and a bad one, and so can a repeatable flag across occurrences.
+	for _, v := range values {
+		if !containsChoice(accepted, v, m.IgnoreCase) {
+			return &Error{Code: CodeInvalidChoice, Name: m.Name,
+				Spelling: m.Spelling, Choices: m.Choices}
+		}
+	}
+	return nil
+}
+
 // Check applies the rules that judge what ended up bound.
 //
 // `values` is the result of [Fill], and `occurrences` is how many times a
@@ -341,19 +375,8 @@ func Check(m *Meta, values []string, occurrences int) *Error {
 		return &Error{Code: code, Name: m.Name, Spelling: m.Spelling}
 	}
 
-	accepted := m.AcceptedChoices
-	if len(accepted) == 0 {
-		accepted = m.Choices
-	}
-	if len(accepted) > 0 && !m.AllowUnknownChoices {
-		// Every value, not just the first: a variadic can be given a good value
-		// and a bad one, and so can a repeatable flag across occurrences.
-		for _, v := range values {
-			if !containsChoice(accepted, v, m.IgnoreCase) {
-				return &Error{Code: CodeInvalidChoice, Name: m.Name,
-					Spelling: m.Spelling, Choices: m.Choices}
-			}
-		}
+	if err := CheckDisplaced(m, values); err != nil {
+		return err
 	}
 
 	if m.Validate != "" {

go/argv/post_test.go

@@ -174,6 +174,31 @@ func TestCheckRichChoices(t *testing.T) {
 	}
 }
 
+// The corpus's `overrides-do-not-erase-an-invalid-choice`.
+//
+// A flag that lost an `overrides` holds nothing, so every other rule in [Check]
+// has nothing to judge — but `overrides` settles which of a pair is in effect,
+// not whether the word the loser was handed was one it accepts.
+func TestCheckDisplacedJudgesOnlyTheWords(t *testing.T) {
+	meta := &Meta{Name: "log-level", Spelling: "--log-level",
+		Choices: []string{"debug", "info"}, Required: true}
+
+	if err := CheckDisplaced(meta, []string{"v"}); err == nil || err.Code != CodeInvalidChoice {
+		t.Fatalf("a word outside the choices survives the displacement: %+v", err)
+	}
+	if err := CheckDisplaced(meta, []string{"info"}); err != nil {
+		t.Fatalf("a word among the choices is displaced in silence: %v", err)
+	}
+	// The half that does *not* survive: a loser is not missing, it is out of the
+	// running. Reporting it would undo the last-one-wins the user asked for.
+	if err := CheckDisplaced(meta, nil); err != nil {
+		t.Fatalf("a loser that was never typed has nothing to answer for: %v", err)
+	}
+	if err := CheckDisplaced(nil, []string{"v"}); err != nil {
+		t.Fatalf("an unknown entry declares no choices: %v", err)
+	}
+}
+
 func TestCheckPortableValidation(t *testing.T) {
 	meta := &Meta{
 		Name:          "port",