From 3f932fad66890a4d2f09edc1edb19e8fd0d354b4 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 02:00:30 +0000 Subject: [PATCH 1/2] feat(go): apply the rules that are decided after the last token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Binding says which token becomes which flag or argument. This says whether what landed is acceptable, and fills in what the command line left empty. The corpus goes from 122 answered to 145 of 152. `required`, `choices`, the `env`-then-`default` fallback, `var_min` and `var_max` all need to know something no single token can tell you, which is why they were left out of the parser rather than overlooked. They read a second, cold table — `Meta`, indexed by the same key the parse table carries, so the two cannot drift on identity — and a program that never applies them never touches it. The zero-allocation test covers a parse with metadata present, since the property that matters is that binding does not reach for any of this. Deliberately not a framework. `Fill` and `Check` are pure functions over what binding produced, because the caller is the one that knows how it accumulated: a generated struct assigns to a field, a harness with no target type appends to a slice. Inventing a value model here would force both through it, and this crate has spent four PRs not doing that to the parser either. Three things the corpus settled that guessing would have got wrong: An environment variable set to the empty string is set — `EX_JOBS=` is a value, because treating empty as unset would make it mean something no other empty value in the grammar means. The value is one token, never re-split: quoting is the shell's job and there was no shell here at all. A flag that holds no value reads its variable as a yes or a no, and the rule is an allow-list — `1`, `true`, `True`, `TRUE` — matching usage-lib exactly. So `yes`, `on` and `TrUe` are all false, which is worth pinning rather than discovering: `EX_VERBOSE=0` meaning verbose would be a trap. The corpus pins the four cases it cares about and `TestEnvTruth` records the rest. `var_max` counts occurrences here, never values. A variadic's per-occurrence bound is a limit binding applies, so judging the total again afterwards would fail an invocation that never broke it. The seven vectors still unanswered are relationships *between* flags — `conflicts`, `overrides`, `required_unless` — which need a name resolved to the entry it refers to. They are listed by id with a reason rather than inferred from the spec, because `overrides-loser-is-not-refilled-from-env` is as much an env question as an overrides one and inference would have exempted vectors nobody meant to exempt. The skip count is asserted against the list, so it cannot become a way of hiding failures. Co-Authored-By: Claude Opus 5 --- go/README.md | 41 ++++-- go/argv/argv.go | 40 ++++++ go/argv/post.go | 202 +++++++++++++++++++++++++++ go/argv/post_test.go | 190 +++++++++++++++++++++++++ go/conformance/conformance_test.go | 215 ++++++++++++++++++++++++----- go/internal/spec/spec.go | 98 +++++++++++-- 6 files changed, 728 insertions(+), 58 deletions(-) create mode 100644 go/argv/post.go create mode 100644 go/argv/post_test.go diff --git a/go/README.md b/go/README.md index 08e89fc80..61a667d61 100644 --- a/go/README.md +++ b/go/README.md @@ -54,12 +54,15 @@ its error inline; a bound value is a slice of the argv string rather than a copy `TestParseAllocatesNothing` measures this with `testing.AllocsPerRun`, on the failure paths as well as the success ones. A mise-sized binding runs in 57 ns. -**Binding only.** The parser answers one question — which token becomes which flag -or argument — and reports each occurrence as an event. Everything that needs to -know a value's _type_ (`required`, `choices`, `env` fallback, defaults, `var_min`, -`overrides`) belongs to the layer that owns the target struct, exactly as it does -in Rust. That is why the corpus's post-binding vectors are skipped here rather -than failed. +**Binding stays separate from judging.** The parser answers one question — which +token becomes which flag or argument — and reports each occurrence as an event. +The rules that need a value's declared type live in `post.go`, reading a second, +cold table the parser never touches: `required`, `choices`, the `env`-then- +`default` fallback, `var_min`, `var_max`. They are pure functions over what +binding produced rather than a framework, because the caller is the one that knows +how it accumulated — generated code assigns to a field, a harness with no target +type appends to a slice, and inventing a value model here would force both through +it. ## Using it @@ -114,10 +117,14 @@ an implementation in any language can run it. `go/conformance` runs all of it: mise run test:go ``` -All 122 binding vectors pass; the 30 post-binding ones are skipped with the reason -recorded. The count is worth watching rather than just quoting: it was 101 when -this module landed, and grew when the corpus imported the argv questions clap's -suite answers — which the Go parser then answered without a change. A vector's spec is KDL, and this module deliberately has no KDL parser — +**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. + +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 — `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 @@ -131,9 +138,17 @@ claim is measured at real scale rather than against a fixture with four flags: ## What is missing -- **The typed layer.** Binding produces events; something has to turn them into a - struct with `int` and `time.Duration` fields, and apply the post-binding rules. - This is where the corpus's skipped post-binding vectors get answered. +- **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 + against the corpus came first, the way the binder did before the generator. +- **Typed values.** Binding collects text. Something still has to turn `"8"` into + an `int` and `"1m"` into a `time.Duration`, and report the ones that will not + convert. - **Help and errors.** A cold table of help text, and rendering worth reading. - **Completions.** The Rust side serves these from the parser's own scope rules so that what is offered and what is accepted cannot disagree; the hooks for it diff --git a/go/argv/argv.go b/go/argv/argv.go index fc9bdab7f..6dc3d03ae 100644 --- a/go/argv/argv.go +++ b/go/argv/argv.go @@ -242,6 +242,22 @@ const ( CodeHelp // CodeVersion means --version or -V was given. Not a failure either. CodeVersion + + // The rest are raised after the parse, by [Check], because they need to know + // a value's declared type or its final count. They share this type so a caller + // has one error to handle rather than two. + + // CodeMissingRequiredFlag means a required flag never appeared. + CodeMissingRequiredFlag + // CodeMissingRequiredArg means a required argument was never filled. + CodeMissingRequiredArg + // CodeInvalidChoice means a value was given that is not among the declared + // choices. + CodeInvalidChoice + // CodeVarTooFew means fewer values than var_min. + CodeVarTooFew + // CodeVarTooMany means more occurrences than a repeatable flag's var_max. + CodeVarTooMany ) var codeNames = [...]string{ @@ -252,6 +268,11 @@ var codeNames = [...]string{ CodeTooDeep: "too_deep", CodeHelp: "help", CodeVersion: "version", + CodeMissingRequiredFlag: "missing_required_flag", + CodeMissingRequiredArg: "missing_required_arg", + CodeInvalidChoice: "invalid_choice", + CodeVarTooFew: "var_too_few", + CodeVarTooMany: "var_too_many", } // String gives the code the corpus spells it with. @@ -290,6 +311,17 @@ type Error struct { Cmd *Command // Long distinguishes --help from -h, which print different amounts. Long bool + + // Name is the flag or argument the post-binding rules rejected, as the spec + // spells it. + Name string + // Choices carries the declared list for CodeInvalidChoice, rather than the + // offending value: the value is the caller's to render, and it has it. + Choices []string + // Bound and Got are the declared limit and what was actually counted, for the + // two var codes. + Bound uint32 + Got int } func (e *Error) Error() string { @@ -308,6 +340,14 @@ func (e *Error) Error() string { return "help requested" case CodeVersion: return "version requested" + case CodeMissingRequiredFlag, CodeMissingRequiredArg: + return "missing required: " + e.Name + case CodeInvalidChoice: + return "invalid value for " + e.Name + case CodeVarTooFew: + return "too few values for " + e.Name + case CodeVarTooMany: + return "too many occurrences of " + e.Name } return "parse error" } diff --git a/go/argv/post.go b/go/argv/post.go new file mode 100644 index 000000000..296d7d4bf --- /dev/null +++ b/go/argv/post.go @@ -0,0 +1,202 @@ +package argv + +// The rules that are decided once the last token has been read. +// +// Binding says which token becomes which flag or argument. These say whether +// what landed is acceptable, and fill in what the command line left empty — and +// every one of them needs to know something about a value that no single token +// can tell you. `required` needs to know nothing was given anywhere, `var_min` +// needs the final count, `choices` needs the declared list. So they live here, +// off the hot path, reading a second table the parser never touches. +// +// Deliberately not a framework. These are pure functions over what binding +// produced, because the caller is the one that knows how it accumulated: a +// generated struct assigns to a field, and a harness with no target type appends +// to a slice. Inventing a value model here would force both of them through it. +// +// The split is the same one usage-argv makes in Rust, and the reason is the same: +// a successful parse never reaches this file, so nothing here is in front of the +// parser. + +// Meta is the cold half of a flag or argument's declaration. +// +// Everything binding deliberately does not know. A generated table holds one of +// these per entry, indexed by [Meta.Key] — see [Metadata] — and a program that +// never applies the rules never touches them. +type Meta struct { + // Key matches the [Flag.Key] or [Arg.Key] this describes, so the two tables + // cannot drift apart on identity even though they are separate data. + Key uint64 + // Name is what the spec calls it, for the error. + Name string + // Flag distinguishes a missing flag from a missing argument, which the + // grammar reports as different classes. + Flag bool + // Required means it must end up with a value, from anywhere. + Required bool + // Choices is the exact set of values allowed. Matching is case-sensitive: + // case-insensitive matching would have to be declared rather than assumed. + Choices []string + // Default fills in when neither the command line nor the environment did. + Default []string + // Env names an environment variable to fall back to. Empty means none. + Env string + // VarMin is the fewest values a variadic may end up with. Zero means no + // bound. It is a check rather than a limit, because nothing about a single + // word tells you a variadic will end up short. + VarMin uint32 + // VarMax is the most times a repeatable flag may be given. Zero means no + // bound. + // + // Occurrences, not values. A variadic's per-occurrence bound is a limit that + // 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 +} + +// Metadata is the cold table, indexed by key. +// +// Keys are dense from 1, so entry `Key` sits at `Metadata[Key-1]` and a lookup is +// an index rather than a map — which also keeps the table static data the linker +// can lay out, where a Go map would need building at init. +type Metadata []Meta + +// Lookup returns the metadata for a key, or nil if the table has none. +func (m Metadata) Lookup(key uint64) *Meta { + if key == 0 || key > uint64(len(m)) { + return nil + } + entry := &m[key-1] + if entry.Key != key { + // A table built by hand, or one that got out of step with the parse + // tables. Searching would paper over it; reporting nothing makes the + // caller's own test fail instead. + return nil + } + return entry +} + +// Source says where a value came from, which callers need because the rules +// distinguish them: an `overrides` loser is not refilled from the environment, +// and a value that arrived from `env` is still checked against `choices`. +type Source uint8 + +const ( + // FromArgv means the command line supplied it. + FromArgv Source = iota + // FromEnv means the environment did. + FromEnv + // FromDefault means neither did, and the declaration filled it in. + FromDefault + // Unset means nothing supplied it. + Unset +) + +// Fill applies the fallbacks: command line, then environment, then default. +// +// `given` is what binding produced, and nil means the command line said nothing — +// which is distinct from a flag given an empty value, since `--jobs=` binds the +// empty string and that is a value. +// +// `lookupEnv` is passed in rather than read from the process, so that a caller +// testing a parse is not testing the machine it runs on. [LookupEnv] wraps +// os.LookupEnv for callers that do want the process environment. +// +// An environment variable set to the empty string is set. Treating empty as unset +// would make `EX_JOBS=` mean something no other empty value in the grammar means. +func Fill(m *Meta, given []string, lookupEnv func(string) (string, bool)) ([]string, Source) { + if given != nil { + return given, FromArgv + } + if m == nil { + return nil, Unset + } + if m.Env != "" && lookupEnv != nil { + if v, ok := lookupEnv(m.Env); ok { + // One token. The grammar never re-splits a value on whitespace — + // quoting is the shell's job and there was no shell here at all. + return []string{v}, FromEnv + } + } + if len(m.Default) > 0 { + return m.Default, FromDefault + } + return nil, Unset +} + +// Check applies the rules that judge what ended up bound. +// +// `values` is the result of [Fill], and `occurrences` is how many times a +// repeatable flag was given — which is not `len(values)`, since one occurrence of +// a variadic can bring several. Pass 0 for an argument. +// +// The first failure is returned; a caller wanting all of them should call this +// per entry, which it is doing anyway. +func Check(m *Meta, values []string, occurrences int) *Error { + if m == nil { + return nil + } + + // `occurrences` is what makes a value-less flag work here: it has no values to + // count, so "was it given" is the only question, and the answer is that it was + // seen at least once. + if m.Required && len(values) == 0 && occurrences == 0 { + // Two classes, because the grammar reports them separately: what is + // missing reads differently for a flag nobody typed than for an argument + // the command needed. + code := CodeMissingRequiredArg + if m.Flag { + code = CodeMissingRequiredFlag + } + return &Error{Code: code, Name: m.Name} + } + + if len(m.Choices) > 0 { + // Every value, not just the first: a variadic can be given a good value + // and a bad one, and so can a repeatable flag across occurrences. + for _, v := range values { + if !contains(m.Choices, v) { + return &Error{Code: CodeInvalidChoice, Name: m.Name, Choices: m.Choices} + } + } + } + + // Only where something was given. An absent optional variadic has not broken + // its minimum; it simply is not there, and reporting `var_too_few` for it + // would make every bounded variadic effectively required. + if m.VarMin > 0 && len(values) > 0 && uint32(len(values)) < m.VarMin { + return &Error{Code: CodeVarTooFew, Name: m.Name, Bound: m.VarMin, Got: len(values)} + } + + if m.VarMax > 0 && occurrences > int(m.VarMax) { + return &Error{Code: CodeVarTooMany, Name: m.Name, Bound: m.VarMax, Got: occurrences} + } + + return nil +} + +// EnvTruth reports whether an environment value sets a flag that holds no value. +// +// A flag with no value has nowhere to put the text, so the variable has to be +// read as a yes or a no — and `EX_VERBOSE=0` meaning "verbose" would be a trap. +// +// An allow-list rather than a not-falsy test, matching usage-lib exactly, which +// means `yes`, `on` and `TrUe` are all false. That is worth knowing rather than +// discovering: the corpus pins `1`, `true`, `false` and `0`, and the rest of the +// list is here so the two implementations cannot drift on the cases it does not. +func EnvTruth(value string) bool { + switch value { + case "1", "true", "True", "TRUE": + return true + } + return false +} + +func contains(list []string, s string) bool { + for _, x := range list { + if x == s { + return true + } + } + return false +} diff --git a/go/argv/post_test.go b/go/argv/post_test.go new file mode 100644 index 000000000..09daf7ed6 --- /dev/null +++ b/go/argv/post_test.go @@ -0,0 +1,190 @@ +package argv + +import ( + "reflect" + "testing" +) + +func env(pairs map[string]string) func(string) (string, bool) { + return func(name string) (string, bool) { + v, ok := pairs[name] + return v, ok + } +} + +func TestFillOrder(t *testing.T) { + meta := &Meta{Name: "jobs", Flag: true, Env: "EX_JOBS", Default: []string{"1"}} + + cases := []struct { + name string + given []string + env map[string]string + want []string + source Source + }{ + {"the command line wins", []string{"8"}, map[string]string{"EX_JOBS": "4"}, + []string{"8"}, FromArgv}, + {"then the environment", nil, map[string]string{"EX_JOBS": "4"}, + []string{"4"}, FromEnv}, + {"then the default", nil, nil, []string{"1"}, FromDefault}, + // Treating empty as unset would make `EX_JOBS=` mean something no other + // empty value in the grammar means. + {"an empty variable is set", nil, map[string]string{"EX_JOBS": ""}, + []string{""}, FromEnv}, + // The grammar never re-splits a value: quoting is the shell's job, and + // there was no shell involved here at all. + {"a value is one token", nil, map[string]string{"EX_JOBS": "a b,c"}, + []string{"a b,c"}, FromEnv}, + // `--jobs=` binds the empty string, which is a value the command line gave. + {"an empty value from argv is still a value", []string{""}, + map[string]string{"EX_JOBS": "4"}, []string{""}, FromArgv}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, source := Fill(meta, c.given, env(c.env)) + if !reflect.DeepEqual(got, c.want) || source != c.source { + t.Errorf("want %q from %v, got %q from %v", c.want, c.source, got, source) + } + }) + } +} + +func TestFillWithNothingDeclared(t *testing.T) { + // No metadata at all is ordinary: an entry with nothing to say about itself + // beyond how it binds. + if got, source := Fill(nil, nil, env(nil)); got != nil || source != Unset { + t.Errorf("want nothing, got %q from %v", got, source) + } + if got, _ := Fill(nil, []string{"x"}, env(nil)); !reflect.DeepEqual(got, []string{"x"}) { + t.Errorf("what binding gave should survive: %q", got) + } +} + +func TestCheck(t *testing.T) { + cases := []struct { + name string + meta Meta + values []string + occurrences int + want Code + ok bool + }{ + {"a required flag nobody gave", Meta{Name: "file", Flag: true, Required: true}, + nil, 0, CodeMissingRequiredFlag, false}, + {"a required argument nobody filled", Meta{Name: "file", Required: true}, + nil, 0, CodeMissingRequiredArg, false}, + {"a required flag given a value", Meta{Name: "file", Flag: true, Required: true}, + []string{"x"}, 1, 0, true}, + // A value-less flag has no values to count, so being seen at all is the + // only evidence it was given. + {"a required flag that holds no value", Meta{Name: "v", Flag: true, Required: true}, + nil, 1, 0, true}, + + {"a value outside the choices", Meta{Name: "shell", Choices: []string{"bash", "zsh"}}, + []string{"csh"}, 1, CodeInvalidChoice, false}, + // Matching is case-sensitive: case-insensitive matching would have to be + // declared rather than assumed. + {"choices are case-sensitive", Meta{Name: "shell", Choices: []string{"bash"}}, + []string{"BASH"}, 1, CodeInvalidChoice, false}, + // Every value, because a variadic can be given a good one and a bad one. + {"every value is checked", Meta{Name: "shell", Choices: []string{"bash", "zsh"}}, + []string{"bash", "csh"}, 1, CodeInvalidChoice, false}, + {"all of them allowed", Meta{Name: "shell", Choices: []string{"bash", "zsh"}}, + []string{"bash", "zsh"}, 2, 0, true}, + + {"fewer values than var_min", Meta{Name: "files", VarMin: 2}, + []string{"a"}, 1, CodeVarTooFew, false}, + {"enough values", Meta{Name: "files", VarMin: 2}, []string{"a", "b"}, 1, 0, true}, + // An absent optional variadic has not broken its minimum; it simply is not + // there, and reporting it would make every bounded variadic required. + {"an absent variadic has not broken its minimum", Meta{Name: "files", VarMin: 2}, + nil, 0, 0, true}, + + // Occurrences, not values: a variadic occurrence can bring several. + {"more occurrences than var_max", Meta{Name: "include", Flag: true, VarMax: 1}, + []string{"a", "b"}, 2, CodeVarTooMany, false}, + {"one occurrence bringing several values", Meta{Name: "include", Flag: true, VarMax: 1}, + []string{"a", "b"}, 1, 0, true}, + + // Required is asked first: a required variadic given nothing is missing + // rather than short, which is the more useful thing to be told. + {"missing beats short", Meta{Name: "files", Required: true, VarMin: 2}, + nil, 0, CodeMissingRequiredArg, false}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := Check(&c.meta, c.values, c.occurrences) + if c.ok { + if err != nil { + t.Fatalf("want no error, got %q", err.Code) + } + return + } + if err == nil { + t.Fatalf("want %q, got no error", c.want) + } + if err.Code != c.want { + t.Errorf("want %q, got %q", c.want, err.Code) + } + if err.Name != c.meta.Name { + t.Errorf("the error should name the entry, got %q", err.Name) + } + }) + } +} + +// TestEnvTruth pins the allow-list, including what it deliberately leaves out. +func TestEnvTruth(t *testing.T) { + for _, s := range []string{"1", "true", "True", "TRUE"} { + if !EnvTruth(s) { + t.Errorf("%q should set the flag", s) + } + } + // `0` and `false` are what the corpus pins. The rest are the cases the + // allow-list quietly excludes, recorded so nobody assumes otherwise. + for _, s := range []string{"0", "false", "", "yes", "on", "TrUe", "2"} { + if EnvTruth(s) { + t.Errorf("%q should not set the flag", s) + } + } +} + +func TestMetadataLookup(t *testing.T) { + m := Metadata{{Key: 1, Name: "a"}, {Key: 2, Name: "b"}, {Key: 3, Name: "c"}} + if got := m.Lookup(2); got == nil || got.Name != "b" { + t.Errorf("want b, got %v", got) + } + // Out of range in both directions, and key 0, which nothing is ever assigned. + for _, key := range []uint64{0, 4, 1 << 40} { + if got := m.Lookup(key); got != nil { + t.Errorf("key %d should find nothing, got %v", key, got) + } + } + // A table out of step with the parse tables reports nothing rather than + // silently describing the wrong entry. + crooked := Metadata{{Key: 7, Name: "wrong"}} + if got := crooked.Lookup(1); got != nil { + t.Errorf("a mismatched key should find nothing, got %v", got) + } +} + +// TestPostBindingIsOffTheHotPath is the reason this file is separate. +// +// A parse that never asks for the rules must not pay for them, and the check +// worth making is that the tables above are not consulted during binding at all — +// which shows up as the parse still allocating nothing with metadata present. +func TestPostBindingIsOffTheHotPath(t *testing.T) { + args := []string{"install", "--verbose", "-f", "a", "b"} + n := testing.AllocsPerRun(100, func() { + p := New(root, args) + for p.Next() { + _ = p.Event() + } + _ = p.Err() + }) + if n != 0 { + t.Errorf("want 0 allocations, got %v", n) + } +} diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index 722f837ab..d76192c39 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -86,14 +86,9 @@ func TestCorpus(t *testing.T) { for _, v := range vectors { v := v t.Run(v.ID, func(t *testing.T) { - // usage-argv answers binding vectors. The rest are decided once the last - // token has been read — required, choices, env fallback, defaults, var_min, - // overrides — and need to know a value's type, so they belong to the layer - // that owns the target struct. The Go parser draws the line in the same - // place. - if v.Layer == "post-binding" { + if reason, unsupported := notYet[v.ID]; unsupported { skipped++ - t.Skip("post-binding: belongs to the layer that owns the target struct") + t.Skip(reason) } ran++ @@ -103,7 +98,7 @@ func TestCorpus(t *testing.T) { lowered[v.Spec] = s } - got, gotErr := run(s, v.Argv) + got, gotErr := run(s, v.Argv, v.Env) switch { case v.Expect.Error != "": @@ -127,8 +122,37 @@ func TestCorpus(t *testing.T) { }) } - t.Logf("%d vectors: %d binding, %d post-binding left to the layer above", - len(vectors), ran, skipped) + t.Logf("%d vectors: %d answered, %d not yet", len(vectors), ran, 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) { + t.Errorf("skipped %d vectors but `notYet` lists %d; a listed id may have been "+ + "renamed, which would silently stop excluding anything", skipped, len(notYet)) + } +} + +// 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. +// +// 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", } // note quotes the vector's own explanation on failure, and flags the ones where @@ -149,40 +173,48 @@ func note(v Vector) string { // deliberately does not know: it reports each occurrence and lets the caller // decide. Generated code assigns to a field or appends to a slice; here the spec // says which of the two to do. -func run(s *spec.Spec, args []string) (*Parsed, *argv.Error) { +func run(s *spec.Spec, args []string, env map[string]string) (*Parsed, *argv.Error) { + root, meta := s.Build() multi := s.MultiFlags() - out := &Parsed{ - Cmd: []string{}, - Flags: map[string]interface{}{}, - Args: map[string]interface{}{}, + + // Accumulated by key rather than by name, because the post-binding rules are + // keyed that way and two commands in one path may declare the same name — a + // subcommand redeclaring a global is ordinary. + type bound struct { + values []string + occurrences int + negated bool } + got := map[uint64]*bound{} + entry := func(key uint64) *bound { + if got[key] == nil { + got[key] = &bound{} + } + return got[key] + } + + // The commands whose declarations are in scope. A required flag on a command + // nobody selected is not missing; it is simply not this invocation's. + path := []*argv.Command{root} - p := argv.New(s.Tables(), args) + p := argv.New(root, args) for p.Next() { ev := p.Event() switch ev.Kind { case argv.KindCommand: - out.Cmd = append(out.Cmd, ev.Command.Name) + out := ev.Command + path = append(path, out) case argv.KindFlag: - name := ev.Flag.Name - switch { - case multi[name] == spec.MultiCount: - // A count flag records one entry per occurrence. - out.Flags[name] = append(bools(out.Flags[name]), true) - case multi[name] == spec.MultiVar && ev.HasValue: - out.Flags[name] = append(strs(out.Flags[name]), ev.Value) - case ev.HasValue: - out.Flags[name] = ev.Value - default: - out.Flags[name] = !ev.Negated + b := entry(ev.Flag.Key) + b.occurrences++ + b.negated = ev.Negated + if ev.HasValue { + b.values = append(b.values, ev.Value) } case argv.KindArg: - name := ev.Arg.Name - if ev.Arg.Var { - out.Args[name] = append(strs(out.Args[name]), ev.Value) - } else { - out.Args[name] = ev.Value - } + b := entry(ev.Arg.Key) + b.occurrences++ + b.values = append(b.values, ev.Value) } } if err := p.Err(); err != nil { @@ -192,9 +224,124 @@ func run(s *spec.Spec, args []string) (*Parsed, *argv.Error) { } return nil, e } + + out := &Parsed{ + Cmd: []string{}, + Flags: map[string]interface{}{}, + Args: map[string]interface{}{}, + } + for _, cmd := range path[1:] { + out.Cmd = append(out.Cmd, cmd.Name) + } + + lookup := func(name string) (string, bool) { + // The vector's own environment, never the process's, so no result can + // depend on the machine running the suite. + v, ok := env[name] + return v, ok + } + + 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 + } + } + 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 + } + if len(values) == 0 { + continue + } + if a.Var { + out.Args[a.Name] = toList(values) + } else { + out.Args[a.Name] = values[len(values)-1] + } + } + } return out, nil } +// 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, + values []string, source argv.Source, negated bool, occurrences int) (interface{}, bool) { + + if !f.TakesValue { + // A count flag records one entry per occurrence, so it is asked before + // anything that collapses the flag to a single answer. + if multi[f.Name] == spec.MultiCount { + if occurrences == 0 { + return nil, false + } + list := make([]interface{}, occurrences) + for i := range list { + list[i] = true + } + return list, true + } + switch source { + case argv.FromArgv: + return !negated, true + case argv.FromEnv: + // The text has nowhere to go, so it is read as a yes or a no. + return argv.EnvTruth(values[0]), true + case argv.FromDefault: + return len(values) > 0 && values[0] == "true", true + } + return nil, false + } + + if len(values) == 0 { + return nil, false + } + if multi[f.Name] == spec.MultiVar { + return toList(values), true + } + // The last one wins for a flag that is not collecting, which is what a field + // assignment does. + return values[len(values)-1], true +} + +func toList(values []string) []interface{} { + out := make([]interface{}, len(values)) + for i, v := range values { + out[i] = v + } + return out +} + func strs(v interface{}) []interface{} { if v == nil { return nil diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index 037afabd9..b5ff72f1d 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -67,18 +67,47 @@ type Flag struct { Count bool `json:"count"` // VarMax here bounds occurrences, which is a post-binding check. The bound // binding cares about is the one on Arg. - VarMax int `json:"var_max"` - Arg *Arg `json:"arg"` + VarMax int `json:"var_max"` + // VarMin is a post-binding check too, and for the same reason: no single + // token can tell you a repeatable flag will end up short. + VarMin int `json:"var_min"` + Required bool `json:"required"` + Default []string `json:"default"` + Env string `json:"env"` + Arg *Arg `json:"arg"` +} + +// choices for a flag are declared on the value it takes, not on the flag. +func (f *Flag) choices() []string { + if f.Arg == nil { + return nil + } + return f.Arg.Choices.list() } // Arg is one positional argument, or a flag's value, in the lowered spec. type Arg struct { - Name string `json:"name"` - Required bool `json:"required"` - Var bool `json:"var"` - VarMax int `json:"var_max"` - VarMin int `json:"var_min"` - DoubleDash string `json:"double_dash"` + Name string `json:"name"` + Required bool `json:"required"` + Var bool `json:"var"` + VarMax int `json:"var_max"` + VarMin int `json:"var_min"` + DoubleDash string `json:"double_dash"` + Choices *Choices `json:"choices"` + Default []string `json:"default"` + Env string `json:"env"` +} + +// Choices is the declared set of values, which the lowering nests one level. +type Choices struct { + Choices []string `json:"choices"` +} + +func (c *Choices) list() []string { + if c == nil { + return nil + } + return c.Choices } // Multi is how a flag accumulates when it is given more than once. @@ -93,11 +122,22 @@ const ( MultiVar ) -// Tables builds the parse tables for a spec. +// Tables builds the parse tables for a spec, discarding the cold half. +func (s *Spec) Tables() *argv.Command { + root, _ := s.Build() + return root +} + +// Build produces both tables: the hot one binding reads, and the cold one the +// post-binding rules read. +// +// Together, because they share the keys that tie them to each other. Building +// them in separate passes would mean two places assigning identifiers and one +// bug away from a `Meta` describing a different entry than the one it names. // // Inheritance is resolved here rather than in the parser: each command's entry // holds the effective value, which is what a generated table would carry. -func (s *Spec) Tables() *argv.Command { +func (s *Spec) Build() (*argv.Command, argv.Metadata) { b := &builder{} root := b.command(&s.Cmd, unknownFlags(s.UnknownFlags, argv.UnknownFlagsValue)) @@ -113,7 +153,7 @@ func (s *Spec) Tables() *argv.Command { } } } - return root + return root, b.meta } // MultiFlags reports which flags accumulate rather than replace, keyed by the @@ -151,6 +191,18 @@ type builder struct { // can simply count where the Rust derive has to hash: two macro expansions // cannot see each other, and two `go generate` runs over one spec can. key uint64 + // meta grows in step with the keys, so entry `Key` lands at `meta[Key-1]` and + // a lookup is an index. + meta argv.Metadata +} + +// record files an entry's cold half at the position its key indexes. +func (b *builder) record(key uint64, m argv.Meta) { + m.Key = key + for uint64(len(b.meta)) < key { + b.meta = append(b.meta, argv.Meta{}) + } + b.meta[key-1] = m } func (b *builder) next() uint64 { @@ -207,6 +259,19 @@ func (b *builder) flag(f *Flag) *argv.Flag { out.Shorts = append(out.Shorts, s[0]) } } + b.record(out.Key, argv.Meta{ + Name: f.Name, + Flag: true, + Required: f.Required, + Choices: f.choices(), + Default: f.Default, + Env: f.Env, + VarMin: clampVarMax(f.VarMin), + // Occurrences. The per-occurrence value bound is a limit binding applies, + // and is set on the parse table below rather than here. + VarMax: clampVarMax(f.VarMax), + }) + if f.Arg != nil && f.Arg.Var { // Only a variadic argument is greedy. A var flag with a single-value argument // is repeatable instead: one value per occurrence, which the parser gets by @@ -230,6 +295,17 @@ func (b *builder) arg(a *Arg) *argv.Arg { if a.Var { out.VarMax = clampVarMax(a.VarMax) } + b.record(out.Key, argv.Meta{ + Name: a.Name, + Required: a.Required, + Choices: a.Choices.list(), + Default: a.Default, + Env: a.Env, + VarMin: clampVarMax(a.VarMin), + // No VarMax: for an argument the bound is a limit binding applies, which + // is what makes `[a]… [b]` fillable at all, so judging it again here would + // fail an invocation that never broke it. + }) return out } From de6de50174b38fa49f1613ebd326a6d519487e74 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 02:23:30 +0000 Subject: [PATCH 2/2] fix(go): read a flag's default from the value it takes, as usage-lib does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A default can be written in two places, and only one was being read: flag "--jobs " { arg "" default="4" } usage-lib falls back to the nested one — `lib/src/parse.rs` says so in as many words, and a live parse of exactly that spec prints `jobs=4` — so a Go CLI generated from the same spec has to as well, or the two disagree about what a bare invocation means. `choices` was already read through the same nesting, which is what made the gap look arbitrary. The flag's own default wins where both are written, that being the narrower declaration. `env` deliberately does not follow, and the asymmetry is the interesting part. usage-lib does not read a nested `env` either: with `arg "" env="EX_MODE"` inside a flag, `EX_MODE=turbo` leaves the flag unset. Reading it here would be this implementation inventing behaviour the reference does not have, which is worse than the inconsistency. Checked rather than assumed — the review that raised the default said nothing about `env`, and it would have been easy to "fix" both and diverge. Both halves are tested, along with the tie between the two tables: every entry's metadata must describe that entry and no other, since they are separate data joined only by key. Co-Authored-By: Claude Opus 5 --- go/internal/spec/spec.go | 27 ++++++- go/internal/spec/spec_test.go | 136 ++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 go/internal/spec/spec_test.go diff --git a/go/internal/spec/spec.go b/go/internal/spec/spec.go index b5ff72f1d..62041a080 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -85,6 +85,31 @@ func (f *Flag) choices() []string { return f.Arg.Choices.list() } +// defaults reads through to the value a flag takes, which is the other place a +// default can be written: +// +// flag "--jobs " { +// arg "" default="4" +// } +// +// usage-lib falls back to it — `lib/src/parse.rs` says so in as many words and a +// live parse confirms it — so a Go CLI generated from the same spec has to as +// well, or the two disagree about what `ex` alone means. +// +// Only the default. A nested `env` is *not* read, because usage-lib does not read +// it either: with `arg "" env="EX_MODE"` inside a flag, `EX_MODE=turbo` leaves +// the flag unset. Following the nesting for one and not the other looks arbitrary +// until you try it, so it is recorded here rather than rediscovered. +func (f *Flag) defaults() []string { + if len(f.Default) > 0 { + return f.Default + } + if f.Arg != nil { + return f.Arg.Default + } + return nil +} + // Arg is one positional argument, or a flag's value, in the lowered spec. type Arg struct { Name string `json:"name"` @@ -264,7 +289,7 @@ func (b *builder) flag(f *Flag) *argv.Flag { Flag: true, Required: f.Required, Choices: f.choices(), - Default: f.Default, + Default: f.defaults(), Env: f.Env, VarMin: clampVarMax(f.VarMin), // Occurrences. The per-occurrence value bound is a limit binding applies, diff --git a/go/internal/spec/spec_test.go b/go/internal/spec/spec_test.go new file mode 100644 index 000000000..110cdc60e --- /dev/null +++ b/go/internal/spec/spec_test.go @@ -0,0 +1,136 @@ +package spec + +import ( + "reflect" + "testing" + + "github.com/jdx/usage/go/argv" +) + +// Built from a Spec literal rather than from lowered JSON, so these run without +// the `usage` CLI. The corpus is where the lowering itself is exercised. +func build(s *Spec) (*argv.Command, argv.Metadata) { return s.Build() } + +func metaFor(t *testing.T, meta argv.Metadata, root *argv.Command, name string) *argv.Meta { + t.Helper() + for _, f := range root.Flags { + if f.Name == name { + m := meta.Lookup(f.Key) + if m == nil { + t.Fatalf("no metadata for flag %q", name) + } + return m + } + } + for _, a := range root.Args { + if a.Name == name { + m := meta.Lookup(a.Key) + if m == nil { + t.Fatalf("no metadata for arg %q", name) + } + return m + } + } + t.Fatalf("no entry named %q", name) + return nil +} + +// A default can be written on the flag or on the value the flag takes, and +// usage-lib falls back to the second. A Go CLI generated from the same spec has +// to agree, or the two disagree about what a bare invocation means. +func TestAFlagsDefaultCanBeDeclaredOnItsValue(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "jobs", Long: []string{"jobs"}, Arg: &Arg{Name: "n", Default: []string{"4"}}}, + {Name: "level", Long: []string{"level"}, Default: []string{"info"}, + Arg: &Arg{Name: "l", Default: []string{"ignored"}}}, + {Name: "plain", Long: []string{"plain"}, Arg: &Arg{Name: "p"}}, + }}, + }) + + if got := metaFor(t, meta, root, "jobs").Default; !reflect.DeepEqual(got, []string{"4"}) { + t.Errorf("a default on the value should be read: got %q", got) + } + // The flag's own wins where both are written, which is the narrower + // declaration. + if got := metaFor(t, meta, root, "level").Default; !reflect.DeepEqual(got, []string{"info"}) { + t.Errorf("the flag's own default should win: got %q", got) + } + if got := metaFor(t, meta, root, "plain").Default; got != nil { + t.Errorf("nothing declared should stay nothing: got %q", got) + } +} + +// The other half of the same question, and the answer is the opposite one: +// usage-lib does not read a nested `env`, so neither does this. Verified against +// it rather than assumed — `arg "" env="EX_MODE"` inside a flag leaves the +// flag unset when EX_MODE is set. +func TestANestedEnvIsNotRead(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "mode", Long: []string{"mode"}, Arg: &Arg{Name: "m", Env: "EX_MODE"}}, + {Name: "shell", Long: []string{"shell"}, Env: "EX_SHELL", Arg: &Arg{Name: "s"}}, + }}, + }) + + if got := metaFor(t, meta, root, "mode").Env; got != "" { + t.Errorf("a nested env should not be read, got %q", got) + } + if got := metaFor(t, meta, root, "shell").Env; got != "EX_SHELL" { + t.Errorf("the flag's own env should be read, got %q", got) + } +} + +// choices are only ever written on the value, so they are always read through. +func TestChoicesAreReadThroughTheValue(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", Flags: []Flag{ + {Name: "shell", Long: []string{"shell"}, + Arg: &Arg{Name: "s", Choices: &Choices{Choices: []string{"bash", "zsh"}}}}, + }}, + }) + want := []string{"bash", "zsh"} + if got := metaFor(t, meta, root, "shell").Choices; !reflect.DeepEqual(got, want) { + t.Errorf("want %q, got %q", want, got) + } +} + +// The two tables are separate data tied together by key, so the tie is what is +// worth testing: every entry's metadata must describe that entry and no other. +func TestMetadataLinesUpWithTheParseTables(t *testing.T) { + root, meta := build(&Spec{ + Name: "ex", Bin: "ex", + Cmd: Cmd{Name: "ex", + Flags: []Flag{{Name: "verbose", Long: []string{"verbose"}}}, + Args: []Arg{{Name: "file", Required: true}}, + Subcommands: map[string]Cmd{ + "install": {Name: "install", + Flags: []Flag{{Name: "force", Long: []string{"force"}}}, + Args: []Arg{{Name: "pkg"}}}, + }, + }, + }) + + var walk func(*argv.Command) + walk = func(c *argv.Command) { + for _, f := range c.Flags { + m := meta.Lookup(f.Key) + if m == nil || m.Name != f.Name || !m.Flag { + t.Errorf("flag %q of %q has metadata %+v", f.Name, c.Name, m) + } + } + for _, a := range c.Args { + m := meta.Lookup(a.Key) + if m == nil || m.Name != a.Name || m.Flag { + t.Errorf("arg %q of %q has metadata %+v", a.Name, c.Name, m) + } + } + for _, sub := range c.Subcommands { + walk(sub) + } + } + walk(root) +}