From 3b423723ec968c4332b7406e91f530785adb2219 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 04:30:30 +0000 Subject: [PATCH 1/4] feat(go): decide the rules that compare one flag against another, finishing the corpus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `conflicts`, `overrides`, `required_if` and `required_unless` — the last seven vectors. **The Go implementation now answers all 152 the corpus has**, and the suite asserts nothing was skipped so that stays a measurement rather than a claim. These are separate from post.go because each needs a *second* entry to answer at all: a name in the declaration has to be resolved to the entry it refers to first, and that happens where the whole command is visible rather than per parse. The tables carry resolved keys, so nothing downstream searches by name. `overrides` is the odd one and is applied first, before anything fills from `env` or `default`. It asks which of two flags came last, which only the arriving tokens know — and a flag that lost is not merely unset. Refilling it from the environment afterwards would leave both standing and undo the last-one-wins the user asked for by typing the second one, which is exactly what `overrides-loser-is-not-refilled-from-env` pins. The relationship is symmetric however it was declared. `--file overrides --stdin` establishes the pair; it does not mean `--file` wins. The corpus is pointed about this: with `--file` declaring it and `--stdin` typed last, `--file` is the one that loses, and a test says so in those words. `conflicts` asks only whether a flag *has* a value, never how it got one, so a value from the environment counts on both sides — two vectors cover the one-sided and the neither-side-typed cases. Unlike `overrides`, it is a mistake to report rather than an order to resolve, and the error carries both names because either alone reads as a puzzle: which flag is unwelcome depends on what else was given. `required_if` has no corpus vector and is implemented anyway, since it is the mirror of `required_unless` and the emitter will have to carry it either way. It is tested here rather than left to be discovered. Co-Authored-By: Claude Opus 5 --- go/README.md | 20 ++-- go/argv/argv.go | 9 ++ go/argv/post.go | 17 +++ go/argv/relationships.go | 144 +++++++++++++++++++++++ go/argv/relationships_test.go | 183 +++++++++++++++++++++++++++++ go/conformance/conformance_test.go | 178 +++++++++++++++++++--------- go/internal/spec/spec.go | 63 +++++++++- 7 files changed, 546 insertions(+), 68 deletions(-) create mode 100644 go/argv/relationships.go create mode 100644 go/argv/relationships_test.go diff --git a/go/README.md b/go/README.md index 61a667d61..f8514c967 100644 --- a/go/README.md +++ b/go/README.md @@ -64,6 +64,12 @@ how it accumulated — generated code assigns to a field, a harness with no targ type appends to a slice, and inventing a value model here would force both through it. +The four rules that compare one entry against another — `conflicts`, `overrides`, +`required_if`, `required_unless` — live in `relationships.go`, because a name in +the declaration has to be resolved to the entry it refers to before any of them +can be checked. `overrides` is the odd one out and is applied first: it asks which +of two flags came _last_, which only the arriving tokens know. + ## Using it ```go @@ -117,14 +123,14 @@ an implementation in any language can run it. `go/conformance` runs all of it: mise run test:go ``` -**145 of 152 vectors pass.** The seven left are relationships _between_ flags — -`conflicts`, `overrides`, `required_unless` — which need a resolution step turning -a name into the entry it refers to. They are listed by id in `notYet` with a -reason, and the count is asserted, so that set cannot quietly grow. +**All 152 vectors pass** — every one the corpus has, binding and post-binding +alike. The suite asserts that nothing was skipped, so that stays a measurement +rather than a claim. The number is worth watching rather than quoting: 101 when this module landed, 122 once the corpus imported the argv questions clap's suite answers (which the Go -parser answered without a change), and 145 once the post-binding rules arrived. A vector's spec is KDL, and this module deliberately has no KDL parser — +parser answered without a change), 145 once the post-binding rules arrived, and 152 +with the relationships between flags. A vector's spec is KDL, and this module deliberately has no KDL parser — `usage generate json` does the lowering, which is why the suite needs the CLI built. That split is the same one an adopter gets: tables are generated once at build time by a maintainer who has the usage CLI, and the shipped binary never @@ -138,10 +144,6 @@ claim is measured at real scale rather than against a fixture with four flags: ## What is missing -- **Relationships between flags.** `conflicts`, `overrides`, `required_if` and - `required_unless` — the seven corpus vectors still unanswered. Unlike everything - in `post.go` they are not a property of one entry: a name has to be resolved to - the entry it refers to first. - **The cold table in generated code.** `usage generate go` emits the parse tables but not the `Meta` ones yet, so the post-binding rules are reachable today from a spec lowered at run time rather than from a generated package. Proving them diff --git a/go/argv/argv.go b/go/argv/argv.go index 6dc3d03ae..079a121c2 100644 --- a/go/argv/argv.go +++ b/go/argv/argv.go @@ -258,6 +258,8 @@ const ( CodeVarTooFew // CodeVarTooMany means more occurrences than a repeatable flag's var_max. CodeVarTooMany + // CodeConflictingFlags means two flags declared to conflict were both given. + CodeConflictingFlags ) var codeNames = [...]string{ @@ -273,6 +275,7 @@ var codeNames = [...]string{ CodeInvalidChoice: "invalid_choice", CodeVarTooFew: "var_too_few", CodeVarTooMany: "var_too_many", + CodeConflictingFlags: "conflicting_flags", } // String gives the code the corpus spells it with. @@ -322,6 +325,10 @@ type Error struct { // two var codes. Bound uint32 Got int + // Other is the flag [Name] cannot be given with, for CodeConflictingFlags. + // Both are carried because either alone reads as a puzzle: which flag is + // unwelcome depends on what else was given. + Other string } func (e *Error) Error() string { @@ -348,6 +355,8 @@ func (e *Error) Error() string { return "too few values for " + e.Name case CodeVarTooMany: return "too many occurrences of " + e.Name + case CodeConflictingFlags: + return e.Name + " cannot be given with " + e.Other } return "parse error" } diff --git a/go/argv/post.go b/go/argv/post.go index 296d7d4bf..addafa144 100644 --- a/go/argv/post.go +++ b/go/argv/post.go @@ -52,6 +52,23 @@ type Meta struct { // binding applies, and lives on [Flag.VarMax] and [Arg.VarMax] instead — a // value bound here would fail an invocation that never broke it. VarMax uint32 + + // The four that need a second entry to answer, all resolved to keys rather + // than left as the names the spec writes. Resolution happens where the whole + // command is visible — the generator, or the table builder — so that nothing + // downstream has to search by name, and a declaration naming a flag that does + // not exist is caught there rather than silently doing nothing. + // See relationships.go. + + // Conflicts names entries this one cannot be given alongside. + Conflicts []uint64 + // Overrides names entries this one is mutually exclusive with, resolved by + // whichever was given last rather than reported as a mistake. + Overrides []uint64 + // RequiredUnless makes this required when none of them is present. + RequiredUnless []uint64 + // RequiredIf makes this required when any of them is present. + RequiredIf []uint64 } // Metadata is the cold table, indexed by key. diff --git a/go/argv/relationships.go b/go/argv/relationships.go new file mode 100644 index 000000000..e8e69970d --- /dev/null +++ b/go/argv/relationships.go @@ -0,0 +1,144 @@ +package argv + +// The rules that compare one entry against another. +// +// Everything in post.go judges an entry on its own: is it there, is its value +// allowed, are there enough of them. These four need a second entry to answer at +// all, which is why they are separate — a name in the declaration has to be +// resolved to the entry it refers to before any of it can be checked, and that +// resolution happens where the whole command is visible rather than here. +// +// `overrides` is the odd one. The other three are decided once the last token has +// been read; this one asks which of two flags came *last*, which only the +// arriving tokens know. So it is applied first, on the order binding reports, and +// what it removes is removed before anything else looks. + +// ApplyOverrides decides last-one-wins between flags declared to override each +// other, returning the keys that lost. +// +// `order` gives the position of each key's last occurrence on the command line. +// A key absent from it was not typed, and cannot win or lose: the declaration is +// about which of two *given* flags survives. +// +// A loser must be treated as absent by everything downstream, and in particular +// must not be refilled from `env` or `default`. Filling it afterwards would leave +// both flags standing and undo the last-one-wins the user asked for by typing the +// second one. +// +// The relationship is symmetric however it was declared. `--file overrides +// --stdin` establishes the pair; it does not mean `--file` always wins. The +// corpus pins that directly: with `--file` declaring it and `--stdin` typed last, +// `--file` is the one that loses. +func ApplyOverrides(meta Metadata, order map[uint64]int) map[uint64]bool { + if len(order) < 2 { + return nil + } + var lost map[uint64]bool + drop := func(key uint64) { + if lost == nil { + lost = map[uint64]bool{} + } + lost[key] = true + } + + for key, at := range order { + m := meta.Lookup(key) + if m == nil { + continue + } + for _, other := range m.Overrides { + otherAt, given := order[other] + if !given { + continue + } + // Equal positions cannot happen — two flags cannot share a token — but + // were it ever to, dropping neither is the safer answer than dropping + // both and losing the value entirely. + if otherAt < at { + drop(other) + } else if at < otherAt { + drop(key) + } + } + } + return lost +} + +// CheckRelationships verifies the rules that read one entry's state to judge +// another, once every entry's final state is known. +// +// `entries` is every key in scope, so each declaration is visited once, and +// `isSet` reports whether an entry ended up with a value from any source. That +// last part is the whole of `conflicts`: a value from the environment counts as +// given, because the check asks whether a flag has a value rather than how it got +// one. clap, argparse and usage all agree on that, and the corpus pins both the +// one-sided and the neither-side-typed case. +func CheckRelationships(meta Metadata, entries []uint64, isSet func(uint64) bool) *Error { + for _, key := range entries { + m := meta.Lookup(key) + if m == nil { + continue + } + + if isSet(key) { + for _, other := range m.Conflicts { + if isSet(other) { + // Both names, because either alone reads as a puzzle: which flag + // is unwelcome depends entirely on what else was given. + return &Error{ + Code: CodeConflictingFlags, + Name: m.Name, + Other: nameOf(meta, other), + } + } + } + continue + } + + // Absent, so the two conditional requirements are the question. Both are + // skipped where the entry is already `Required`, since that has been + // answered by Check and reporting it twice helps nobody. + if m.Required { + continue + } + + // Required unless one of these is present. With none of them present the + // requirement stands. + if len(m.RequiredUnless) > 0 && !anySet(m.RequiredUnless, isSet) { + return missingRequired(m) + } + + // Required because one of these is present. + if len(m.RequiredIf) > 0 && anySet(m.RequiredIf, isSet) { + return missingRequired(m) + } + } + return nil +} + +func missingRequired(m *Meta) *Error { + code := CodeMissingRequiredArg + if m.Flag { + code = CodeMissingRequiredFlag + } + return &Error{Code: code, Name: m.Name} +} + +func anySet(keys []uint64, isSet func(uint64) bool) bool { + for _, k := range keys { + if isSet(k) { + return true + } + } + return false +} + +// nameOf renders the other side of a relationship, falling back to nothing rather +// than to a number: an error naming `key 7` is worse than one naming only the +// flag the reader already knows about. +func nameOf(meta Metadata, key uint64) string { + if m := meta.Lookup(key); m != nil { + return m.Name + } + return "" +} diff --git a/go/argv/relationships_test.go b/go/argv/relationships_test.go new file mode 100644 index 000000000..a16564917 --- /dev/null +++ b/go/argv/relationships_test.go @@ -0,0 +1,183 @@ +package argv + +import ( + "reflect" + "testing" +) + +// Keys 1 and 2 are `--file` and `--stdin`, the pair the corpus uses. +const ( + keyFile uint64 = iota + 1 + keyStdin + keyURL +) + +func pair(fileDeclares, stdinDeclares []uint64, field string) Metadata { + m := Metadata{ + {Key: keyFile, Name: "file", Flag: true}, + {Key: keyStdin, Name: "stdin", Flag: true}, + {Key: keyURL, Name: "url", Flag: true}, + } + set := func(at int, keys []uint64) { + switch field { + case "overrides": + m[at].Overrides = keys + case "conflicts": + m[at].Conflicts = keys + case "required_unless": + m[at].RequiredUnless = keys + case "required_if": + m[at].RequiredIf = keys + } + } + set(0, fileDeclares) + set(1, stdinDeclares) + return m +} + +func TestApplyOverrides(t *testing.T) { + cases := []struct { + name string + meta Metadata + order map[uint64]int + want map[uint64]bool + }{ + { + // Declared on both sides, `--file` typed last. + "the last one given wins", + pair([]uint64{keyStdin}, []uint64{keyFile}, "overrides"), + map[uint64]int{keyStdin: 1, keyFile: 2}, + map[uint64]bool{keyStdin: true}, + }, + { + // The relationship is symmetric however it was declared: `--file` + // declares it and `--file` is the one that loses, because `--stdin` + // came last. + "the declaring side can be the loser", + pair([]uint64{keyStdin}, nil, "overrides"), + map[uint64]int{keyFile: 1, keyStdin: 2}, + map[uint64]bool{keyFile: true}, + }, + { + // The declaration is about which of two *given* flags survives. + "nothing is lost when only one was given", + pair([]uint64{keyStdin}, nil, "overrides"), + map[uint64]int{keyFile: 1}, + nil, + }, + { + "an unrelated flag is untouched", + pair([]uint64{keyStdin}, nil, "overrides"), + map[uint64]int{keyFile: 1, keyStdin: 2, keyURL: 3}, + map[uint64]bool{keyFile: true}, + }, + { + "nothing declared, nothing lost", + pair(nil, nil, "overrides"), + map[uint64]int{keyFile: 1, keyStdin: 2}, + nil, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := ApplyOverrides(c.meta, c.order) + if len(got) == 0 && len(c.want) == 0 { + return + } + if !reflect.DeepEqual(got, c.want) { + t.Errorf("want %v, got %v", c.want, got) + } + }) + } +} + +func TestConflicts(t *testing.T) { + meta := pair([]uint64{keyStdin}, nil, "conflicts") + all := []uint64{keyFile, keyStdin, keyURL} + + set := func(keys ...uint64) func(uint64) bool { + in := map[uint64]bool{} + for _, k := range keys { + in[k] = true + } + return func(k uint64) bool { return in[k] } + } + + if err := CheckRelationships(meta, all, set(keyFile, keyStdin)); err == nil { + t.Error("both given should conflict") + } else { + if err.Code != CodeConflictingFlags { + t.Errorf("want conflicting_flags, got %q", err.Code) + } + // Both names, because either alone reads as a puzzle. + if err.Name != "file" || err.Other != "stdin" { + t.Errorf("want both names, got %q and %q", err.Name, err.Other) + } + } + + // Declared on one side only, and the relationship holds either way round: + // it is between the two flags, not between a flag and the tokens after it. + if err := CheckRelationships(meta, []uint64{keyStdin, keyFile, keyURL}, + set(keyStdin, keyFile)); err == nil { + t.Error("the order the entries are visited in should not matter") + } + + for _, given := range [][]uint64{{keyFile}, {keyStdin}, {keyFile, keyURL}, {}} { + if err := CheckRelationships(meta, all, set(given...)); err != nil { + t.Errorf("%v should be allowed, got %q", given, err.Code) + } + } +} + +func TestConditionalRequirements(t *testing.T) { + set := func(keys ...uint64) func(uint64) bool { + in := map[uint64]bool{} + for _, k := range keys { + in[k] = true + } + return func(k uint64) bool { return in[k] } + } + all := []uint64{keyFile, keyStdin, keyURL} + + unless := pair([]uint64{keyStdin}, nil, "required_unless") + // With neither present, the requirement stands. + if err := CheckRelationships(unless, all, set()); err == nil { + t.Error("neither given should be missing_required_flag") + } else if err.Code != CodeMissingRequiredFlag || err.Name != "file" { + t.Errorf("want missing file, got %q %q", err.Code, err.Name) + } + // Satisfied either by the flag itself or by the one it names. + for _, given := range [][]uint64{{keyFile}, {keyStdin}, {keyFile, keyStdin}} { + if err := CheckRelationships(unless, all, set(given...)); err != nil { + t.Errorf("%v should satisfy it, got %q", given, err.Code) + } + } + + ifm := pair([]uint64{keyURL}, nil, "required_if") + if err := CheckRelationships(ifm, all, set(keyURL)); err == nil { + t.Error("the trigger being present should make it required") + } else if err.Code != CodeMissingRequiredFlag { + t.Errorf("want missing_required_flag, got %q", err.Code) + } + if err := CheckRelationships(ifm, all, set()); err != nil { + t.Errorf("without the trigger there is no requirement, got %q", err.Code) + } + if err := CheckRelationships(ifm, all, set(keyURL, keyFile)); err != nil { + t.Errorf("given, it is satisfied, got %q", err.Code) + } +} + +// An entry already marked Required has been answered by Check, and saying it +// twice in two different voices helps nobody. +func TestAnAlreadyRequiredEntryIsNotReportedTwice(t *testing.T) { + meta := Metadata{ + {Key: keyFile, Name: "file", Flag: true, Required: true, + RequiredUnless: []uint64{keyStdin}}, + {Key: keyStdin, Name: "stdin", Flag: true}, + } + none := func(uint64) bool { return false } + if err := CheckRelationships(meta, []uint64{keyFile, keyStdin}, none); err != nil { + t.Errorf("Check owns this one, got %q", err.Code) + } +} diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index d76192c39..1e8a83d0a 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -124,6 +124,13 @@ func TestCorpus(t *testing.T) { t.Logf("%d vectors: %d answered, %d not yet", len(vectors), ran, skipped) + // The corpus is answered in full today. Asserted so that it stays a + // measurement: a vector that starts failing gets skipped by nobody, and one + // that gets quietly excluded shows up here. + if skipped != 0 { + t.Errorf("%d vectors were skipped; the whole corpus is meant to be answered", skipped) + } + // Asserted, so the unsupported set cannot quietly grow: a vector added to // `notYet` without this being raised deliberately fails here instead. if skipped != len(notYet) { @@ -134,26 +141,15 @@ func TestCorpus(t *testing.T) { // Vectors this implementation does not answer yet, and why. // -// By id rather than by inspecting the spec. Every one of these is a relationship -// *between* flags, which needs a resolution step the cold table does not have -// yet: a name in `conflicts` or `overrides` has to be turned into the entry it -// refers to. Everything else the post-binding layer covers — required, choices, -// env fallback, defaults, var_min, var_max — is a property of one entry and is -// answered. +// Empty, and kept rather than deleted: the whole corpus is answered today, and +// the mechanism is what makes that a measurement instead of a claim. The count is +// asserted against this map above, so a vector added here has to be added +// deliberately, and one that stops being skipped without being removed fails. // -// Listing ids is deliberately annoying. Inferring "has a `conflicts` in the spec" -// would exempt vectors nobody meant to exempt — `overrides-loser-is-not-refilled-from-env` -// is as much an env question as an overrides one — and the count above is asserted -// so this list cannot rot into a way of hiding failures. -var notYet = map[string]string{ - "conflicts-both-given": "conflicts: a relationship between flags", - "conflicts-either-order": "conflicts: a relationship between flags", - "conflicts-one-side-from-env": "conflicts: a relationship between flags", - "conflicts-both-sides-from-env": "conflicts: a relationship between flags", - "overrides-last-wins": "overrides: a relationship between flags", - "overrides-loser-is-not-refilled-from-env": "overrides: a relationship between flags", - "required-unless-unsatisfied": "required_unless: a relationship between flags", -} +// By id, if it ever refills. Inferring "has a `conflicts` in the spec" would +// exempt vectors nobody meant to exempt: `overrides-loser-is-not-refilled-from-env` +// is as much an env question as an overrides one. +var notYet = map[string]string{} // note quotes the vector's own explanation on failure, and flags the ones where // usage-lib diverges from the grammar: those are the cases where matching the @@ -184,8 +180,15 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err values []string occurrences int negated bool + // Where this entry's last token sat, which `overrides` needs and nothing + // else does: it is the one rule decided by which of two flags came last. + at int } + seen := 0 got := map[uint64]*bound{} + // Keys an override removed. They are absent from here on, including from the + // fallbacks, which is the whole point. + lost := map[uint64]bool{} entry := func(key uint64) *bound { if got[key] == nil { got[key] = &bound{} @@ -205,15 +208,19 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err out := ev.Command path = append(path, out) case argv.KindFlag: + seen++ b := entry(ev.Flag.Key) b.occurrences++ b.negated = ev.Negated + b.at = seen if ev.HasValue { b.values = append(b.values, ev.Value) } case argv.KindArg: + seen++ b := entry(ev.Arg.Key) b.occurrences++ + b.at = seen b.values = append(b.values, ev.Value) } } @@ -241,52 +248,107 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err return v, ok } + // Overrides first, and before anything fills from `env` or `default`: a flag + // that lost is not merely unset, and refilling it afterwards would leave both + // standing and undo the last-one-wins. + order := map[uint64]int{} + for key, b := range got { + order[key] = b.at + } + for key := range argv.ApplyOverrides(meta, order) { + delete(got, key) + lost[key] = true + } + + // The fallbacks are applied to everything in scope before anything is judged, + // because the rules that compare two entries need both of their final states — + // and `conflicts` in particular asks only whether a flag has a value, not how + // it got one. + type resolved struct { + flag *argv.Flag + arg *argv.Arg + values []string + source argv.Source + occurrences int + negated bool + } + final := map[uint64]*resolved{} + var scope []uint64 + + fill := func(key uint64, takesValue bool) *resolved { + b := got[key] + var given []string + if b != nil && (len(b.values) > 0 || !takesValue) { + // A value-less flag that was given has no values, and nil would read as + // "the command line said nothing" — so the empty slice is the + // distinction. + given = b.values + if given == nil { + given = []string{} + } + } + r := &resolved{} + // A key an override removed stays absent, fallbacks included. + if !lost[key] { + r.values, r.source = argv.Fill(meta.Lookup(key), given, lookup) + } else { + r.source = argv.Unset + } + if b != nil { + r.occurrences = b.occurrences + r.negated = b.negated + } + final[key] = r + scope = append(scope, key) + return r + } + for _, cmd := range path { for _, f := range cmd.Flags { - b := got[f.Key] - m := meta.Lookup(f.Key) - - var given []string - if b != nil && (len(b.values) > 0 || !f.TakesValue) { - // A value-less flag that was given has no values, and `nil` would - // read as "the command line said nothing" — so the empty slice is - // the distinction. - given = b.values - if given == nil { - given = []string{} - } - } - values, source := argv.Fill(m, given, lookup) - occurrences := 0 - if b != nil { - occurrences = b.occurrences - } - if err := argv.Check(m, values, occurrences); err != nil { - return nil, err - } - if v, ok := renderFlag(f, m, multi, values, source, b != nil && b.negated, - occurrences); ok { - out.Flags[f.Name] = v - } + fill(f.Key, f.TakesValue).flag = f } for _, a := range cmd.Args { - b := got[a.Key] - m := meta.Lookup(a.Key) - var given []string - if b != nil { - given = b.values - } - values, _ := argv.Fill(m, given, lookup) - if err := argv.Check(m, values, 0); err != nil { - return nil, err + fill(a.Key, true).arg = a + } + } + + // What one entry ended up with, judged on its own. + for _, key := range scope { + r := final[key] + occurrences := r.occurrences + if r.arg != nil { + occurrences = 0 + } + if err := argv.Check(meta.Lookup(key), r.values, occurrences); err != nil { + return nil, err + } + } + + // Then the rules that read one entry to judge another. + isSet := func(key uint64) bool { + r := final[key] + return r != nil && r.source != argv.Unset + } + if err := argv.CheckRelationships(meta, scope, isSet); err != nil { + return nil, err + } + + for _, key := range scope { + r := final[key] + switch { + case r.flag != nil: + if v, ok := renderFlag(r.flag, multi, r.values, r.source, r.negated, + r.occurrences); ok { + out.Flags[r.flag.Name] = v } - if len(values) == 0 { + case r.arg != nil: + if len(r.values) == 0 { continue } - if a.Var { - out.Args[a.Name] = toList(values) + if r.arg.Var { + out.Args[r.arg.Name] = toList(r.values) } else { - out.Args[a.Name] = values[len(values)-1] + out.Args[r.arg.Name] = r.values[len(r.values)-1] } } } @@ -295,7 +357,7 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err // renderFlag turns what a flag ended up with into the shape the corpus records, // which depends on what the flag is rather than on where the value came from. -func renderFlag(f *argv.Flag, m *argv.Meta, multi map[string]spec.Multi, +func renderFlag(f *argv.Flag, multi map[string]spec.Multi, values []string, source argv.Source, negated bool, occurrences int) (interface{}, bool) { if !f.TakesValue { diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index 62041a080..2a3354056 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -74,7 +74,12 @@ type Flag struct { Required bool `json:"required"` Default []string `json:"default"` Env string `json:"env"` - Arg *Arg `json:"arg"` + // The four that name another flag. They arrive as written, dashes included. + Conflicts []string `json:"conflicts"` + Overrides []string `json:"overrides"` + RequiredIf []string `json:"required_if"` + RequiredUnless []string `json:"required_unless"` + Arg *Arg `json:"arg"` } // choices for a flag are declared on the value it takes, not on the flag. @@ -261,6 +266,9 @@ func (b *builder) command(c *Cmd, inherited argv.UnknownFlags) *argv.Command { for i := range c.Args { out.Args = append(out.Args, b.arg(&c.Args[i])) } + // After the flags, because a relationship names a sibling and every sibling + // needs a key before any of them can be pointed at. + b.resolveRelationships(c, out) for _, name := range sortedKeys(c.Subcommands) { sub := c.Subcommands[name] out.Subcommands = append(out.Subcommands, b.command(&sub, unknown)) @@ -268,6 +276,59 @@ func (b *builder) command(c *Cmd, inherited argv.UnknownFlags) *argv.Command { return out } +// resolveRelationships turns the names in `conflicts`, `overrides`, +// `required_if` and `required_unless` into the keys they refer to. +// +// Done here, where the whole command is visible, so that nothing downstream has +// to search by name on a path where it would be repeating the work per parse. +// +// The names arrive as they are written — `--stdin`, dashes and all — so they are +// matched against a flag's long forms, its shorts and the name the spec gives it. +// A name nothing answers to is dropped rather than guessed at; the generator is +// where that should be reported, since it is the one a spec author runs. +func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { + find := func(name string) (uint64, bool) { + bare := strings.TrimLeft(name, "-") + for _, f := range out.Flags { + if f.Name == bare { + return f.Key, true + } + for _, long := range f.Longs { + if long == bare { + return f.Key, true + } + } + if len(bare) == 1 { + for _, short := range f.Shorts { + if short == bare[0] { + return f.Key, true + } + } + } + } + return 0, false + } + resolve := func(names []string) []uint64 { + var out []uint64 + for _, name := range names { + if key, ok := find(name); ok { + out = append(out, key) + } + } + return out + } + + // `c.Flags` and `out.Flags` are built in step, so the index is the join. + for i := range c.Flags { + src := &c.Flags[i] + m := &b.meta[out.Flags[i].Key-1] + m.Conflicts = resolve(src.Conflicts) + m.Overrides = resolve(src.Overrides) + m.RequiredUnless = resolve(src.RequiredUnless) + m.RequiredIf = resolve(src.RequiredIf) + } +} + func (b *builder) flag(f *Flag) *argv.Flag { out := &argv.Flag{ Key: b.next(), From 07e0eee262c1832c90dad75b9c9f1d72882a2352 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 04:44:46 +0000 Subject: [PATCH 2/4] fix(go): resolve a relationship against the flags actually in scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a relationship could name a flag and silently resolve to nothing, both of which usage-lib resolves and enforces. An inherited global. `conflicts="--quiet"` on a subcommand's flag, where `--quiet` is a root global, was searched for only among that subcommand's own flags — so the key was dropped and the rule was never enforced, while usage-lib reports `Invalid flag --loud: conflicts with --quiet` for the same spec. The search now goes through the command's own flags first and then any ancestor's globals, which is the scope a token has and in the same order, so a subcommand redeclaring an inherited name shadows it here exactly as it does at parse time. A flag that is *not* global still resolves to nothing from below, which is the other half and is tested. A negation. `conflicts="--no-color"` names the `color` flag, and usage-lib reports the conflict whichever of the two spellings was typed — the relationship is between entries rather than tokens, which is what this key model already assumes. Checked both ways round rather than inferred from the error message, since the message quotes the declared string either way. mise's own spec has no relationship that names anything non-local, so the checked-in tables do not change. That is worth stating rather than leaving to be inferred from an empty diff: the hole was real and simply unreachable from the one large spec in the repository, which is exactly the kind of gap a fixture cannot be relied on to find. Co-Authored-By: Claude Opus 5 --- go/internal/spec/spec.go | 65 ++++++++++++++++----- go/internal/spec/spec_test.go | 105 ++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 15 deletions(-) diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index 2a3354056..a963be46a 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -224,6 +224,10 @@ type builder struct { // meta grows in step with the keys, so entry `Key` lands at `meta[Key-1]` and // a lookup is an index. meta argv.Metadata + // scope is the chain above the command being built, so a relationship can + // name an inherited global. Ancestors only: a command cannot see its own + // children's flags, and neither can a declaration. + scope []*argv.Command } // record files an entry's cold half at the position its key indexes. @@ -269,10 +273,13 @@ func (b *builder) command(c *Cmd, inherited argv.UnknownFlags) *argv.Command { // After the flags, because a relationship names a sibling and every sibling // needs a key before any of them can be pointed at. b.resolveRelationships(c, out) + // In scope for everything below, and out of scope again afterwards. + b.scope = append(b.scope, out) for _, name := range sortedKeys(c.Subcommands) { sub := c.Subcommands[name] out.Subcommands = append(out.Subcommands, b.command(&sub, unknown)) } + b.scope = b.scope[:len(b.scope)-1] return out } @@ -289,21 +296,19 @@ func (b *builder) command(c *Cmd, inherited argv.UnknownFlags) *argv.Command { func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { find := func(name string) (uint64, bool) { bare := strings.TrimLeft(name, "-") - for _, f := range out.Flags { - if f.Name == bare { - return f.Key, true - } - for _, long := range f.Longs { - if long == bare { - return f.Key, true - } - } - if len(bare) == 1 { - for _, short := range f.Shorts { - if short == bare[0] { - return f.Key, true - } - } + // This command's own flags first, then any ancestor's globals — the same + // scope a token has, and in the same order, so a subcommand redeclaring an + // inherited name shadows it here exactly as it does at parse time. + // + // Searching only locally was a silent hole: `conflicts="--quiet"` on a + // subcommand flag, where `--quiet` is a root global, resolved to nothing + // and the rule was simply never enforced. usage-lib enforces it. + if key, ok := matchFlag(out.Flags, bare, false); ok { + return key, true + } + for i := len(b.scope) - 1; i >= 0; i-- { + if key, ok := matchFlag(b.scope[i].Flags, bare, true); ok { + return key, true } } return 0, false @@ -329,6 +334,36 @@ func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { } } +// matchFlag finds a flag by any spelling a declaration may use for it. +// +// The negation counts, and resolves to the same entry: usage-lib treats +// `conflicts="--no-color"` as naming the `color` flag, and reports the conflict +// whichever of the two spellings was typed. The relationship is between entries +// rather than between tokens, which is what this key model already assumes. +func matchFlag(flags []*argv.Flag, bare string, globalsOnly bool) (uint64, bool) { + for _, f := range flags { + if globalsOnly && !f.Global { + continue + } + if f.Name == bare || (f.Negate != "" && f.Negate == bare) { + return f.Key, true + } + for _, long := range f.Longs { + if long == bare { + return f.Key, true + } + } + if len(bare) == 1 { + for _, short := range f.Shorts { + if short == bare[0] { + return f.Key, true + } + } + } + } + return 0, false +} + func (b *builder) flag(f *Flag) *argv.Flag { out := &argv.Flag{ Key: b.next(), diff --git a/go/internal/spec/spec_test.go b/go/internal/spec/spec_test.go index 110cdc60e..ba8736d52 100644 --- a/go/internal/spec/spec_test.go +++ b/go/internal/spec/spec_test.go @@ -134,3 +134,108 @@ func TestMetadataLinesUpWithTheParseTables(t *testing.T) { } walk(root) } + +// A relationship may name a flag the declaring command does not own. +// +// A global is in scope for everything beneath it, so `conflicts="--quiet"` on a +// subcommand flag names the root's `--quiet`. Searching only the local flags left +// the key unresolved and the rule silently unenforced — while usage-lib enforced +// it, which is the kind of difference a generated CLI cannot afford. +func TestARelationshipCanNameAnInheritedGlobal(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", + Flags: []Flag{ + {Name: "quiet", Long: []string{"quiet"}, Global: true}, + // Not global, so not in scope below, and naming it must resolve to + // nothing rather than to something that will never be enforced. + {Name: "local", Long: []string{"local"}}, + }, + Subcommands: map[string]Cmd{ + "run": {Name: "run", Flags: []Flag{ + {Name: "loud", Long: []string{"loud"}, Conflicts: []string{"--quiet"}}, + {Name: "solo", Long: []string{"solo"}, Conflicts: []string{"--local"}}, + }}, + }, + }, + }) + + var quiet uint64 + for _, f := range root.Flags { + if f.Name == "quiet" { + quiet = f.Key + } + } + run := root.Subcommands[0] + + for _, f := range run.Flags { + m := meta.Lookup(f.Key) + switch f.Name { + case "loud": + if len(m.Conflicts) != 1 || m.Conflicts[0] != quiet { + t.Errorf("--loud should conflict with the root's --quiet, got %v", m.Conflicts) + } + case "solo": + if len(m.Conflicts) != 0 { + t.Errorf("--local is not global, so nothing should resolve: %v", m.Conflicts) + } + } + } +} + +// A subcommand redeclaring an inherited name shadows it, here as at parse time. +func TestALocalFlagShadowsTheGlobalOfTheSameName(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", + Flags: []Flag{{Name: "quiet", Long: []string{"quiet"}, Global: true}}, + Subcommands: map[string]Cmd{ + "run": {Name: "run", Flags: []Flag{ + {Name: "quiet", Long: []string{"quiet"}}, + {Name: "loud", Long: []string{"loud"}, Conflicts: []string{"--quiet"}}, + }}, + }, + }, + }) + + run := root.Subcommands[0] + var localQuiet uint64 + for _, f := range run.Flags { + if f.Name == "quiet" { + localQuiet = f.Key + } + } + for _, f := range run.Flags { + if f.Name != "loud" { + continue + } + m := meta.Lookup(f.Key) + if len(m.Conflicts) != 1 || m.Conflicts[0] != localQuiet { + t.Errorf("should name run's own --quiet (%d), got %v", localQuiet, m.Conflicts) + } + } +} + +// usage-lib treats `conflicts="--no-color"` as naming the `color` flag, and +// reports the conflict whichever of the two spellings was typed — the +// relationship is between entries, not tokens. +func TestARelationshipCanNameANegation(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "color", Long: []string{"color"}, Negate: "--no-color"}, + {Name: "plain", Long: []string{"plain"}, Conflicts: []string{"--no-color"}}, + }}, + }) + + var color uint64 + for _, f := range root.Flags { + if f.Name == "color" { + color = f.Key + } + } + m := metaFor(t, meta, root, "plain") + if len(m.Conflicts) != 1 || m.Conflicts[0] != color { + t.Errorf("should resolve to the color flag (%d), got %v", color, m.Conflicts) + } +} From ed4acede924faf0125d67ee02ed84db0456a6202 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 05:19:23 +0000 Subject: [PATCH 3/4] fix(go): count a default as a fallback, leave an override loser alone, and match the form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, all of them cases where this enforced something usage-lib does not. Each was checked against it rather than reasoned about, and each turned out to be real. A default did not count as given. `isSet` treated any source but `Unset` as present, so a defaulted flag conflicted with every partner anyone typed. usage-lib says otherwise: with `--file` defaulted and only `--stdin` given, a declared conflict does not fire. The command line and the environment are the user saying something; a default is a fallback. `Source.Given` is that distinction, named so the next caller does not have to rediscover it. An override loser was still judged. It was cleared from the bindings but `Check` still ran on it, so a `required` loser failed as `missing_required_flag` — undoing the last-one-wins the user asked for by typing the other flag. usage-lib skips overridden flags in the requirement pass, and a loser is now out of the running entirely rather than merely absent. A relationship resolved through the wrong form. `--q` reached the short `-q` and `-color` reached the long `--color`, because the dashes were stripped before matching. usage-lib resolves neither, so this had a generated CLI enforcing a rule the reference does not — the same failure mode as the two above, from the opposite direction. The form is part of the name now: `--x` matches long forms and the negation, `-x` matches shorts, and an undashed word matches the name the spec gives. A declaration naming the wrong form is a typo, and the useful failure is the rule not existing rather than a rule nobody wrote. Also corrects the README's vector count, which said 152 while the corpus this branch runs against has 154. Co-Authored-By: Claude Opus 5 --- go/README.md | 10 ++++--- go/argv/post.go | 10 +++++++ go/argv/post_test.go | 15 ++++++++++ go/argv/relationships.go | 18 +++++++---- go/conformance/conformance_test.go | 18 ++++++----- go/internal/spec/spec.go | 48 +++++++++++++++++++++++------- go/internal/spec/spec_test.go | 45 ++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 26 deletions(-) diff --git a/go/README.md b/go/README.md index f8514c967..0ac104103 100644 --- a/go/README.md +++ b/go/README.md @@ -123,14 +123,16 @@ an implementation in any language can run it. `go/conformance` runs all of it: mise run test:go ``` -**All 152 vectors pass** — every one the corpus has, binding and post-binding +**All 154 vectors pass** — every one the corpus has, binding and post-binding alike. The suite asserts that nothing was skipped, so that stays a measurement rather than a claim. The number is worth watching rather than quoting: 101 when this module landed, 122 -once the corpus imported the argv questions clap's suite answers (which the Go -parser answered without a change), 145 once the post-binding rules arrived, and 152 -with the relationships between flags. A vector's spec is KDL, and this module deliberately has no KDL parser — +once the corpus imported the argv questions clap's suite answers, 145 once the +post-binding rules arrived, 152 with the relationships between flags, and 154 as +the corpus kept growing underneath. Every one of those increases was answered +without a change to the Go side, which is the argument for a shared corpus in one +line. A vector's spec is KDL, and this module deliberately has no KDL parser — `usage generate json` does the lowering, which is why the suite needs the CLI built. That split is the same one an adopter gets: tables are generated once at build time by a maintainer who has the usage CLI, and the shipped binary never diff --git a/go/argv/post.go b/go/argv/post.go index addafa144..baa72e9d9 100644 --- a/go/argv/post.go +++ b/go/argv/post.go @@ -109,6 +109,16 @@ const ( Unset ) +// Given reports whether a source counts as the flag having been *given*, which +// is the question the rules comparing two entries ask. +// +// A default does not count, and that is the whole reason this exists. usage-lib +// and clap both treat `env` as a value source and a default as a fallback: with +// `--file` defaulted and only `--stdin` typed, a declared conflict between them +// does not fire. Counting the default would make a defaulted flag conflict with +// every partner anyone types, which is a CLI nobody can use. +func (s Source) Given() bool { return s == FromArgv || s == FromEnv } + // Fill applies the fallbacks: command line, then environment, then default. // // `given` is what binding produced, and nil means the command line said nothing — diff --git a/go/argv/post_test.go b/go/argv/post_test.go index 09daf7ed6..314f54f74 100644 --- a/go/argv/post_test.go +++ b/go/argv/post_test.go @@ -188,3 +188,18 @@ func TestPostBindingIsOffTheHotPath(t *testing.T) { t.Errorf("want 0 allocations, got %v", n) } } + +// TestSourceGiven pins the asymmetry the relationship rules depend on. +// +// The command line and the environment count as the user having said something; +// a default does not. Counting a default would make a defaulted flag conflict +// with every partner anyone types, and usage-lib agrees: with `--file` defaulted +// and only `--stdin` given, a declared conflict between them does not fire. +func TestSourceGiven(t *testing.T) { + if !FromArgv.Given() || !FromEnv.Given() { + t.Error("argv and env are both the user supplying a value") + } + if FromDefault.Given() || Unset.Given() { + t.Error("a default is a fallback, not something the user said") + } +} diff --git a/go/argv/relationships.go b/go/argv/relationships.go index e8e69970d..f45fb6ccb 100644 --- a/go/argv/relationships.go +++ b/go/argv/relationships.go @@ -68,11 +68,19 @@ func ApplyOverrides(meta Metadata, order map[uint64]int) map[uint64]bool { // another, once every entry's final state is known. // // `entries` is every key in scope, so each declaration is visited once, and -// `isSet` reports whether an entry ended up with a value from any source. That -// last part is the whole of `conflicts`: a value from the environment counts as -// given, because the check asks whether a flag has a value rather than how it got -// one. clap, argparse and usage all agree on that, and the corpus pins both the -// one-sided and the neither-side-typed case. +// `isSet` reports whether an entry counts as *given* — see [Source.Given], which +// is what a caller should use to answer it. +// +// The command line and the environment count; a default does not. That asymmetry +// is the part worth getting right: `conflicts` asks whether a flag has a value +// rather than how it got one, so an environment variable counts on both sides +// and the corpus pins the one-sided and neither-side-typed cases. But a default +// is a fallback rather than something the user said, and counting it would make +// a defaulted flag conflict with every partner anyone types. usage-lib and clap +// both draw the line there. +// +// A key removed by [ApplyOverrides] should not appear in `entries` at all: it +// lost, so it is out of the running rather than merely absent. func CheckRelationships(meta Metadata, entries []uint64, isSet func(uint64) bool) *Error { for _, key := range entries { m := meta.Lookup(key) diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index 1e8a83d0a..10e1cdb85 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -288,12 +288,7 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err } } r := &resolved{} - // A key an override removed stays absent, fallbacks included. - if !lost[key] { - r.values, r.source = argv.Fill(meta.Lookup(key), given, lookup) - } else { - r.source = argv.Unset - } + r.values, r.source = argv.Fill(meta.Lookup(key), given, lookup) if b != nil { r.occurrences = b.occurrences r.negated = b.negated @@ -305,6 +300,15 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err for _, cmd := range path { for _, f := range cmd.Flags { + // 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. + if lost[f.Key] { + continue + } fill(f.Key, f.TakesValue).flag = f } for _, a := range cmd.Args { @@ -327,7 +331,7 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err // Then the rules that read one entry to judge another. isSet := func(key uint64) bool { r := final[key] - return r != nil && r.source != argv.Unset + return r != nil && r.source.Given() } if err := argv.CheckRelationships(meta, scope, isSet); err != nil { return nil, err diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index a963be46a..20e6c4e28 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -295,7 +295,6 @@ func (b *builder) command(c *Cmd, inherited argv.UnknownFlags) *argv.Command { // where that should be reported, since it is the one a spec author runs. func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { find := func(name string) (uint64, bool) { - bare := strings.TrimLeft(name, "-") // This command's own flags first, then any ancestor's globals — the same // scope a token has, and in the same order, so a subcommand redeclaring an // inherited name shadows it here exactly as it does at parse time. @@ -303,11 +302,11 @@ func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { // Searching only locally was a silent hole: `conflicts="--quiet"` on a // subcommand flag, where `--quiet` is a root global, resolved to nothing // and the rule was simply never enforced. usage-lib enforces it. - if key, ok := matchFlag(out.Flags, bare, false); ok { + if key, ok := matchFlag(out.Flags, name, false); ok { return key, true } for i := len(b.scope) - 1; i >= 0; i-- { - if key, ok := matchFlag(b.scope[i].Flags, bare, true); ok { + if key, ok := matchFlag(b.scope[i].Flags, name, true); ok { return key, true } } @@ -340,22 +339,51 @@ func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { // `conflicts="--no-color"` as naming the `color` flag, and reports the conflict // whichever of the two spellings was typed. The relationship is between entries // rather than between tokens, which is what this key model already assumes. -func matchFlag(flags []*argv.Flag, bare string, globalsOnly bool) (uint64, bool) { +func matchFlag(flags []*argv.Flag, name string, globalsOnly bool) (uint64, bool) { + // The form is part of the name. `--q` does not reach the short `-q`, and + // `-color` does not reach the long `--color`: usage-lib resolves neither, and + // resolving them here would have a generated CLI enforcing a rule the + // reference does not. A declaration that names a flag by the wrong form is a + // typo, and the useful failure is the rule not existing rather than a rule + // nobody wrote. + long, short, bare := "", byte(0), "" + switch { + case strings.HasPrefix(name, "--"): + long = name[2:] + case strings.HasPrefix(name, "-") && len(name) == 2: + short = name[1] + case !strings.HasPrefix(name, "-"): + // Undashed, which is the name the spec gives the flag rather than a form + // it can be typed as. + bare = name + } + if long == "" && short == 0 && bare == "" { + return 0, false + } + for _, f := range flags { if globalsOnly && !f.Global { continue } - if f.Name == bare || (f.Negate != "" && f.Negate == bare) { + if bare != "" && f.Name == bare { return f.Key, true } - for _, long := range f.Longs { - if long == bare { + if long != "" { + // The negation is a long form of the flag it belongs to, and names the + // same entry: usage-lib reports a conflict declared against + // `--no-color` whichever of the two spellings was typed. + if f.Negate == long { return f.Key, true } + for _, l := range f.Longs { + if l == long { + return f.Key, true + } + } } - if len(bare) == 1 { - for _, short := range f.Shorts { - if short == bare[0] { + if short != 0 { + for _, s := range f.Shorts { + if s == short { return f.Key, true } } diff --git a/go/internal/spec/spec_test.go b/go/internal/spec/spec_test.go index ba8736d52..c040c8009 100644 --- a/go/internal/spec/spec_test.go +++ b/go/internal/spec/spec_test.go @@ -239,3 +239,48 @@ func TestARelationshipCanNameANegation(t *testing.T) { t.Errorf("should resolve to the color flag (%d), got %v", color, m.Conflicts) } } + +// A relationship names a flag by a form that flag actually has. +// +// usage-lib resolves neither `--q` for a short `-q` nor `-color` for a long +// `--color`, so resolving them here would have a generated CLI enforcing a rule +// the reference does not. A declaration naming the wrong form is a typo, and the +// useful failure is the rule not existing. +func TestARelationshipNeedsTheRightForm(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "quiet", Long: []string{"quiet"}, Short: []string{"q"}}, + {Name: "color", Long: []string{"color"}}, + {Name: "a", Long: []string{"a"}, Conflicts: []string{"--q"}}, + {Name: "b", Long: []string{"b"}, Conflicts: []string{"-color"}}, + {Name: "c", Long: []string{"c"}, Conflicts: []string{"-q"}}, + {Name: "d", Long: []string{"d"}, Conflicts: []string{"--color"}}, + }}, + }) + + var quiet, color uint64 + for _, f := range root.Flags { + switch f.Name { + case "quiet": + quiet = f.Key + case "color": + color = f.Key + } + } + + for _, c := range []struct { + flag string + want []uint64 + }{ + {"a", nil}, // --q is not a long form of anything + {"b", nil}, // -color is not a short + {"c", []uint64{quiet}}, // -q is + {"d", []uint64{color}}, // --color is + } { + got := metaFor(t, meta, root, c.flag).Conflicts + if len(got) != len(c.want) || (len(got) == 1 && got[0] != c.want[0]) { + t.Errorf("--%s: want %v, got %v", c.flag, c.want, got) + } + } +} From b9a0fe619d0a4ecd10bc53f71ec9d3c5f23b8d65 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 06:00:40 +0000 Subject: [PATCH 4/4] fix(go): a default counts for the entry it holds, not for the flags judging it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Source.Given` fix in the commit before this one was half right, and the missing half was a regression it introduced: excluding defaults made a *defaulted entry* fall into the conditional-requirement path and be reported missing, even though its default had already filled it. Both halves checked against usage-lib, because getting this backwards is silent in either direction: --file defaulted, required_unless="--stdin", nothing typed → ok --stdin defaulted, --file required_unless="--stdin" → --file missing --stdin defaulted, --file conflicts="--stdin", --file typed → ok So a default counts for the entry being judged — it has a value, it is not missing — and does not count for the partners judging it, which are asking what the user said. `CheckRelationships` now takes the whole `Source` rather than a predicate, because a caller collapsing it into a yes or no gets one of the two wrong whichever way it chooses, and this way the choice lives in the library beside the reasoning. Also compares a negation as the spec wrote it. `negate="-no-color"` is a form nobody can type as `--no-color`, and usage-lib does not resolve a relationship naming the latter to the flag declaring the former — so the parse table's bare spelling, which the parser needs, is the wrong thing to match a declaration against. The raw form is kept alongside for that. Co-Authored-By: Claude Opus 5 --- go/argv/relationships.go | 51 ++++++++++++------- go/argv/relationships_test.go | 80 ++++++++++++++++++++++++------ go/conformance/conformance_test.go | 10 ++-- go/internal/spec/spec.go | 32 +++++++++--- go/internal/spec/spec_test.go | 29 +++++++++++ 5 files changed, 158 insertions(+), 44 deletions(-) diff --git a/go/argv/relationships.go b/go/argv/relationships.go index f45fb6ccb..58e3bd35a 100644 --- a/go/argv/relationships.go +++ b/go/argv/relationships.go @@ -68,29 +68,35 @@ func ApplyOverrides(meta Metadata, order map[uint64]int) map[uint64]bool { // another, once every entry's final state is known. // // `entries` is every key in scope, so each declaration is visited once, and -// `isSet` reports whether an entry counts as *given* — see [Source.Given], which -// is what a caller should use to answer it. +// `sourceOf` reports where each entry's value came from — the whole [Source] +// rather than a yes or no, because the rules need both readings of it. // -// The command line and the environment count; a default does not. That asymmetry -// is the part worth getting right: `conflicts` asks whether a flag has a value -// rather than how it got one, so an environment variable counts on both sides -// and the corpus pins the one-sided and neither-side-typed cases. But a default -// is a fallback rather than something the user said, and counting it would make -// a defaulted flag conflict with every partner anyone types. usage-lib and clap -// both draw the line there. +// As a *partner*, only the command line and the environment count. `conflicts` +// asks whether a flag has a value rather than how it got one, so an environment +// variable counts on both sides and the corpus pins the one-sided and +// neither-side-typed cases. A default does not count: it is a fallback rather +// than something the user said, and counting it would make a defaulted flag +// conflict with every partner anyone types. +// +// As the entry *being judged*, a default does count — it has a value, so it is +// not missing. usage-lib agrees on both halves, and a caller that collapsed +// `sourceOf` into one predicate would get one of them wrong whichever way it +// chose. // // A key removed by [ApplyOverrides] should not appear in `entries` at all: it // lost, so it is out of the running rather than merely absent. -func CheckRelationships(meta Metadata, entries []uint64, isSet func(uint64) bool) *Error { +func CheckRelationships(meta Metadata, entries []uint64, sourceOf func(uint64) Source) *Error { + given := func(key uint64) bool { return sourceOf(key).Given() } + for _, key := range entries { m := meta.Lookup(key) if m == nil { continue } - if isSet(key) { + if given(key) { for _, other := range m.Conflicts { - if isSet(other) { + if given(other) { // Both names, because either alone reads as a puzzle: which flag // is unwelcome depends entirely on what else was given. return &Error{ @@ -103,21 +109,32 @@ func CheckRelationships(meta Metadata, entries []uint64, isSet func(uint64) bool continue } - // Absent, so the two conditional requirements are the question. Both are - // skipped where the entry is already `Required`, since that has been - // answered by Check and reporting it twice helps nobody. + // Not given — but a default still fills it, and an entry that has a value + // is not missing whatever supplied it. That is the asymmetry: a default + // counts for the entry being judged and not for the partners judging it. + // usage-lib draws it in exactly the same place, which is worth spelling + // out because getting it backwards is silent either way: + // + // --file defaulted, required_unless="--stdin", nothing typed → fine + // --stdin defaulted, --file required_unless="--stdin" → --file missing + if sourceOf(key) != Unset { + continue + } + + // Both are skipped where the entry is already `Required`, since that has + // been answered by Check and reporting it twice helps nobody. if m.Required { continue } // Required unless one of these is present. With none of them present the // requirement stands. - if len(m.RequiredUnless) > 0 && !anySet(m.RequiredUnless, isSet) { + if len(m.RequiredUnless) > 0 && !anySet(m.RequiredUnless, given) { return missingRequired(m) } // Required because one of these is present. - if len(m.RequiredIf) > 0 && anySet(m.RequiredIf, isSet) { + if len(m.RequiredIf) > 0 && anySet(m.RequiredIf, given) { return missingRequired(m) } } diff --git a/go/argv/relationships_test.go b/go/argv/relationships_test.go index a16564917..cf0bdfdcd 100644 --- a/go/argv/relationships_test.go +++ b/go/argv/relationships_test.go @@ -92,18 +92,25 @@ func TestApplyOverrides(t *testing.T) { } } +// set builds a `sourceOf` where the named keys came from the command line and +// everything else is absent. +func set(keys ...uint64) func(uint64) Source { + in := map[uint64]bool{} + for _, k := range keys { + in[k] = true + } + return func(k uint64) Source { + if in[k] { + return FromArgv + } + return Unset + } +} + func TestConflicts(t *testing.T) { meta := pair([]uint64{keyStdin}, nil, "conflicts") all := []uint64{keyFile, keyStdin, keyURL} - set := func(keys ...uint64) func(uint64) bool { - in := map[uint64]bool{} - for _, k := range keys { - in[k] = true - } - return func(k uint64) bool { return in[k] } - } - if err := CheckRelationships(meta, all, set(keyFile, keyStdin)); err == nil { t.Error("both given should conflict") } else { @@ -131,13 +138,6 @@ func TestConflicts(t *testing.T) { } func TestConditionalRequirements(t *testing.T) { - set := func(keys ...uint64) func(uint64) bool { - in := map[uint64]bool{} - for _, k := range keys { - in[k] = true - } - return func(k uint64) bool { return in[k] } - } all := []uint64{keyFile, keyStdin, keyURL} unless := pair([]uint64{keyStdin}, nil, "required_unless") @@ -176,8 +176,56 @@ func TestAnAlreadyRequiredEntryIsNotReportedTwice(t *testing.T) { RequiredUnless: []uint64{keyStdin}}, {Key: keyStdin, Name: "stdin", Flag: true}, } - none := func(uint64) bool { return false } + none := func(uint64) Source { return Unset } if err := CheckRelationships(meta, []uint64{keyFile, keyStdin}, none); err != nil { t.Errorf("Check owns this one, got %q", err.Code) } } + +// A default counts for the entry being judged and not for the partners judging +// it. Getting that backwards is silent in either direction, so both halves are +// pinned — and both were checked against usage-lib rather than reasoned about. +func TestADefaultCountsOnlyForTheEntryItHolds(t *testing.T) { + unless := pair([]uint64{keyStdin}, nil, "required_unless") + all := []uint64{keyFile, keyStdin, keyURL} + + // `--file` has a value, from its default, so it is not missing. + defaulted := func(k uint64) Source { + if k == keyFile { + return FromDefault + } + return Unset + } + if err := CheckRelationships(unless, all, defaulted); err != nil { + t.Errorf("a defaulted entry has a value and is not missing, got %q", err.Code) + } + + // A defaulted *partner* does not satisfy the requirement: nobody said + // `--stdin`, so `--file` is still required and still absent. + partner := func(k uint64) Source { + if k == keyStdin { + return FromDefault + } + return Unset + } + if err := CheckRelationships(unless, all, partner); err == nil { + t.Error("a defaulted partner should not satisfy required_unless") + } else if err.Code != CodeMissingRequiredFlag || err.Name != "file" { + t.Errorf("want missing file, got %q %q", err.Code, err.Name) + } + + // Nor does a defaulted partner trigger a conflict. + conflicts := pair([]uint64{keyStdin}, nil, "conflicts") + both := func(k uint64) Source { + switch k { + case keyFile: + return FromArgv + case keyStdin: + return FromDefault + } + return Unset + } + if err := CheckRelationships(conflicts, all, both); err != nil { + t.Errorf("a defaulted partner should not conflict, got %q", err.Code) + } +} diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index 10e1cdb85..f0f4d4d1c 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -329,11 +329,13 @@ func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Err } // Then the rules that read one entry to judge another. - isSet := func(key uint64) bool { - r := final[key] - return r != nil && r.source.Given() + sourceOf := func(key uint64) argv.Source { + if r := final[key]; r != nil { + return r.source + } + return argv.Unset } - if err := argv.CheckRelationships(meta, scope, isSet); err != nil { + if err := argv.CheckRelationships(meta, scope, sourceOf); err != nil { return nil, err } diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index 20e6c4e28..2785be49f 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -228,9 +228,25 @@ type builder struct { // name an inherited global. Ancestors only: a command cannot see its own // children's flags, and neither can a declaration. scope []*argv.Command + // negation holds each flag's `negate` exactly as the spec wrote it, dashes + // included. The parse table stores it bare, because that is what the parser + // has after stripping the `--`; a relationship names a *form*, so comparing + // it needs the original. A spec declaring `negate="-no-color"` is not named + // by `--no-color`, and usage-lib does not resolve it either. + negation map[uint64]string } // record files an entry's cold half at the position its key indexes. +func (b *builder) recordNegation(key uint64, raw string) { + if raw == "" { + return + } + if b.negation == nil { + b.negation = map[uint64]string{} + } + b.negation[key] = raw +} + func (b *builder) record(key uint64, m argv.Meta) { m.Key = key for uint64(len(b.meta)) < key { @@ -302,11 +318,11 @@ func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { // Searching only locally was a silent hole: `conflicts="--quiet"` on a // subcommand flag, where `--quiet` is a root global, resolved to nothing // and the rule was simply never enforced. usage-lib enforces it. - if key, ok := matchFlag(out.Flags, name, false); ok { + if key, ok := b.matchFlag(out.Flags, name, false); ok { return key, true } for i := len(b.scope) - 1; i >= 0; i-- { - if key, ok := matchFlag(b.scope[i].Flags, name, true); ok { + if key, ok := b.matchFlag(b.scope[i].Flags, name, true); ok { return key, true } } @@ -339,7 +355,7 @@ func (b *builder) resolveRelationships(c *Cmd, out *argv.Command) { // `conflicts="--no-color"` as naming the `color` flag, and reports the conflict // whichever of the two spellings was typed. The relationship is between entries // rather than between tokens, which is what this key model already assumes. -func matchFlag(flags []*argv.Flag, name string, globalsOnly bool) (uint64, bool) { +func (b *builder) matchFlag(flags []*argv.Flag, name string, globalsOnly bool) (uint64, bool) { // The form is part of the name. `--q` does not reach the short `-q`, and // `-color` does not reach the long `--color`: usage-lib resolves neither, and // resolving them here would have a generated CLI enforcing a rule the @@ -369,10 +385,11 @@ func matchFlag(flags []*argv.Flag, name string, globalsOnly bool) (uint64, bool) return f.Key, true } if long != "" { - // The negation is a long form of the flag it belongs to, and names the - // same entry: usage-lib reports a conflict declared against - // `--no-color` whichever of the two spellings was typed. - if f.Negate == long { + // The negation is a form of the flag it belongs to and names the same + // entry: usage-lib reports a conflict declared against `--no-color` + // whichever of the two spellings was typed. Compared as written, so a + // `negate="-no-color"` is not reached by `--no-color`. + if b.negation[f.Key] == name { return f.Key, true } for _, l := range f.Longs { @@ -401,6 +418,7 @@ func (b *builder) flag(f *Flag) *argv.Flag { TakesValue: f.Arg != nil, Global: f.Global, } + b.recordNegation(out.Key, f.Negate) for _, s := range f.Short { if s != "" { // One byte: a cluster is walked a byte at a time, so a multi-byte short diff --git a/go/internal/spec/spec_test.go b/go/internal/spec/spec_test.go index c040c8009..929880de8 100644 --- a/go/internal/spec/spec_test.go +++ b/go/internal/spec/spec_test.go @@ -284,3 +284,32 @@ func TestARelationshipNeedsTheRightForm(t *testing.T) { } } } + +// A negation is compared as the spec wrote it. +// +// `negate="-no-color"` is a form nobody can type as `--no-color`, and usage-lib +// does not resolve a relationship naming the latter to the flag declaring the +// former. Resolving it here would enforce a rule the reference does not. +func TestANegationIsMatchedAsWritten(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "color", Long: []string{"color"}, Negate: "--no-color"}, + {Name: "tint", Long: []string{"tint"}, Negate: "-no-tint"}, + {Name: "a", Long: []string{"a"}, Conflicts: []string{"--no-color"}}, + {Name: "b", Long: []string{"b"}, Conflicts: []string{"--no-tint"}}, + }}, + }) + var color uint64 + for _, f := range root.Flags { + if f.Name == "color" { + color = f.Key + } + } + if got := metaFor(t, meta, root, "a").Conflicts; len(got) != 1 || got[0] != color { + t.Errorf("--no-color should reach the color flag, got %v", got) + } + if got := metaFor(t, meta, root, "b").Conflicts; len(got) != 0 { + t.Errorf("--no-tint is not the form `-no-tint`, so nothing: got %v", got) + } +}