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..62041a080 100644 --- a/go/internal/spec/spec.go +++ b/go/internal/spec/spec.go @@ -67,18 +67,72 @@ 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() +} + +// 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"` - 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 +147,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 +178,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 +216,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 +284,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.defaults(), + 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 +320,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 } 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) +}