diff --git a/go/README.md b/go/README.md index 61a667d61..0ac104103 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,16 @@ 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 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), and 145 once the post-binding rules arrived. 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 @@ -138,10 +146,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..baa72e9d9 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. @@ -92,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 new file mode 100644 index 000000000..58e3bd35a --- /dev/null +++ b/go/argv/relationships.go @@ -0,0 +1,169 @@ +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 +// `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. +// +// 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, 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 given(key) { + for _, other := range m.Conflicts { + 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{ + Code: CodeConflictingFlags, + Name: m.Name, + Other: nameOf(meta, other), + } + } + } + continue + } + + // 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, given) { + return missingRequired(m) + } + + // Required because one of these is present. + if len(m.RequiredIf) > 0 && anySet(m.RequiredIf, given) { + 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..cf0bdfdcd --- /dev/null +++ b/go/argv/relationships_test.go @@ -0,0 +1,231 @@ +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) + } + }) + } +} + +// 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} + + 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) { + 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) 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 d76192c39..f0f4d4d1c 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,113 @@ 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{} + r.values, r.source = argv.Fill(meta.Lookup(key), given, lookup) + 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 + // 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 { - 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. + sourceOf := func(key uint64) argv.Source { + if r := final[key]; r != nil { + return r.source + } + return argv.Unset + } + if err := argv.CheckRelationships(meta, scope, sourceOf); 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 +363,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..2785be49f 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. @@ -219,9 +224,29 @@ 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 + // 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 { @@ -261,13 +286,129 @@ 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) + // 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 } +// 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) { + // 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 := b.matchFlag(out.Flags, name, false); ok { + return key, true + } + for i := len(b.scope) - 1; i >= 0; i-- { + if key, ok := b.matchFlag(b.scope[i].Flags, name, true); ok { + return 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) + } +} + +// 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 (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 + // 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 bare != "" && f.Name == bare { + return f.Key, true + } + if 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 { + if l == long { + return f.Key, true + } + } + } + if short != 0 { + for _, s := range f.Shorts { + if s == short { + return f.Key, true + } + } + } + } + return 0, false +} + func (b *builder) flag(f *Flag) *argv.Flag { out := &argv.Flag{ Key: b.next(), @@ -277,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 110cdc60e..929880de8 100644 --- a/go/internal/spec/spec_test.go +++ b/go/internal/spec/spec_test.go @@ -134,3 +134,182 @@ 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) + } +} + +// 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) + } + } +} + +// 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) + } +}