diff --git a/benches/gate/tests/differential.rs b/benches/gate/tests/differential.rs index 08c642fae..f358f676a 100644 --- a/benches/gate/tests/differential.rs +++ b/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 diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index e5ffe8b0b..56f5d61b8 100644 --- a/conformance/tests/post_binding.rs +++ b/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, + /// 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"]); diff --git a/corpus/08-choices-and-required.json b/corpus/08-choices-and-required.json index a3daf7766..95045dbb7 100644 --- a/corpus/08-choices-and-required.json +++ b/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 \" 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.", diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index fc1423505..90aba94c5 100644 --- a/derive/src/codegen.rs +++ b/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. // diff --git a/docs/go/generated-code.md b/docs/go/generated-code.md index 8dfa8606d..aba68200c 100644 --- a/docs/go/generated-code.md +++ b/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 diff --git a/docs/go/index.md b/docs/go/index.md index e80db38ff..7f13ca78d 100644 --- a/docs/go/index.md +++ b/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 diff --git a/docs/rust/validation.md b/docs/rust/validation.md index e0073a70a..d957704c5 100644 --- a/docs/rust/validation.md +++ b/docs/rust/validation.md @@ -39,7 +39,10 @@ max_retries: Option, `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 diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index 97737ba66..eb0135944 100644 --- a/docs/spec/reference/flag.md +++ b/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. diff --git a/go/argv/post.go b/go/argv/post.go index 76605da94..5c8dde74a 100644 --- a/go/argv/post.go +++ b/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 != "" { diff --git a/go/argv/post_test.go b/go/argv/post_test.go index e05fb0a33..ea07ed452 100644 --- a/go/argv/post_test.go +++ b/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", diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index ed1e07661..939495841 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -354,7 +354,14 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa for key, b := range got { order[key] = b.at } + // Kept even though the binding is dropped: a loser is not judged on what it + // *holds* — it holds nothing — but the words it was typed are still its own, + // and `choices` is asked about them below. + displaced := map[uint64][]string{} for key := range argv.ApplyOverrides(meta, order) { + if b := got[key]; b != nil { + displaced[key] = b.values + } delete(got, key) lost[key] = true } @@ -397,20 +404,29 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa return r } + // Every entry the command declares, in declaration order, losers included. + // `scope` is what has a final state to compare and fill; this is the order the + // per-entry pass reports failures in, and a loser has one question left to + // answer even though it has no state. + var ordered []uint64 + for _, cmd := range path { for _, f := range cmd.Flags { + ordered = append(ordered, f.Key) // A flag that lost an override is out of the running rather than - // merely absent: it is not filled from `env` or `default`, and it is - // not judged either. A `required` loser reported as missing would undo - // the last-one-wins the user asked for by typing the other flag, and - // usage-lib skips overridden flags in the requirement pass for exactly - // that reason. + // merely absent: it is not filled from `env` or `default`, and the + // rules that judge what it *holds* do not apply. A `required` loser + // reported as missing would undo the last-one-wins the user asked for + // by typing the other flag, and usage-lib skips overridden flags in the + // requirement pass for exactly that reason. Its `choices` are still + // asked about the words it was typed — see [argv.CheckDisplaced]. if lost[f.Key] { continue } fill(f.Key, f.TakesValue).flag = f } for _, a := range cmd.Args { + ordered = append(ordered, a.Key) fill(a.Key, true).arg = a } } @@ -432,7 +448,16 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa } // What one entry ended up with, judged on its own. - for _, key := range scope { + for _, key := range ordered { + if lost[key] { + // Nothing to end up with, so only the words it was typed are left to + // judge. In declaration order with the rest, so which failure a line + // with more than one fault reports does not depend on who won a pair. + if err := argv.CheckDisplaced(meta.Lookup(key), displaced[key]); err != nil { + return nil, err + } + continue + } r := final[key] occurrences := r.occurrences if r.arg != nil {