go/post binding - #957
Conversation
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 <noreply@anthropic.com>
…does
A default can be written in two places, and only one was being read:
flag "--jobs <n>" {
arg "<n>" 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 "<m>" 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 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit de6de50. Configure here.
| // 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 |
There was a problem hiding this comment.
Default truthiness mismatches env
Medium Severity
Boolean flags filled from a default only treat the literal true as set, while the env path uses EnvTruth, which also accepts 1, True, and TRUE. usage-lib applies that same allow-list to defaults, so a default of 1 or TRUE would disagree between the two implementations.
Reviewed by Cursor Bugbot for commit de6de50. Configure here.
Greptile SummaryThe PR adds Go post-binding metadata, fallback handling, and validation for required values, choices, and variadic bounds, enabling most post-binding conformance vectors.
Confidence Score: 4/5The shadowing defect should be fixed before merging because valid subcommand invocations can receive false required errors or values from inaccessible global declarations. The parser correctly resolves a shadowing subcommand flag, but the new post-binding traversal independently processes the shadowed ancestor declaration and can validate or render it anyway. Files Needing Attention: go/conformance/conformance_test.go Important Files Changed
Reviews (1): Last reviewed commit: "fix(go): read a flag's default from the ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for _, cmd := range path { | ||
| for _, f := range cmd.Flags { |
There was a problem hiding this comment.
When a subcommand redeclares an ancestor global flag, parsing selects the subcommand declaration but this loop still fills and checks both declarations, causing a shadowed required global to produce a false missing-required error or a shadowed fallback to populate the subcommand flag's output name.
Knowledge Base Used: Compiled argv parsing and derives
1 participant
Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Medium Risk
Changes how argv values are validated and defaulted after binding, with broad conformance coverage; mistakes could alter CLI error behavior and env/default semantics, though binding itself stays unchanged and well unit-tested.
Overview
Adds a post-binding layer in
post.gothat stays off the parse hot path: a coldMeta/Metadatatable keyed like flags and args, plusFill(argv → env → default) andCheck(required, choices,var_min,var_max). Parse-timeargv.Errorgains matching codes and fields.Spec lowering now builds parse tables and metadata together via
Spec.Build(), including defaults on nested flag args (but not nestedenv, aligned with usage-lib).Conformance runs post-binding vectors with env-aware
Fill/Checkinstead of skipping the whole layer; 145/152 corpus vectors pass. Seven ids stay in an explicitnotYetmap forconflicts,overrides, andrequired_unless, with a test that skipped count matches that list.Reviewed by Cursor Bugbot for commit de6de50. Bugbot is set up for automated code reviews on this repo. Configure here.