feat(derive): compile a struct into parse tables and a spec - #803
Conversation
📝 WalkthroughWalkthroughThe PR adds the ChangesDerive CLI implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant derive_cli
participant Cli_from_input
participant emit
participant GeneratedCli
participant usage_argv
derive_cli->>Cli_from_input: Parse and validate CLI declaration
Cli_from_input->>emit: Provide validated model
emit->>GeneratedCli: Generate static tables and parser APIs
GeneratedCli->>usage_argv: Parse argv and receive events
usage_argv-->>GeneratedCli: Return fields or errors
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Instruction countsNothing was compared, and so nothing was gated. No series appears on both sides: either the base has no measurements recorded, or the two were measured on different runner classes, which are deliberately not comparable — counts shift between machine types by more than a real regression does. New, nothing to compare against: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
Greptile SummaryThe PR introduces the
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (8): Last reviewed commit: "fix(derive): stop generated code from co..." | Re-trigger Greptile |
|
All five findings fixed. Every one was real, and the first is the interesting one.
The first one deserves a note: that is the same repeatable-vs-greedy conflation #799 fixed in the conformance harness, recurring in the derive's attribute vocabulary a day later. Twice in one week is a design smell, not bad luck — the spec spells greedy as a variadic argument ( Worth knowing that my original test missed it by always writing 17 tests, and the three new compile errors were checked by hand for message and span. AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
|
Two more findings from the latest review, both fixed.
Some shorts can never be delivered. Both are the same shape of bug as the earlier ones: the derive accepted a declaration the grammar cannot honor. Worth noting that is the failure mode to look for in the rest of this crate — the grammar is written down, so the derive's job is to refuse anything it does not permit, and every finding so far has been a place where it did not. 18 tests. On the check status: Greptile reports AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
cc568bc to
8fed243
Compare
|
Three more findings fixed, and two of them are the same distinction biting a third time.
The middle one is worth dwelling on. That is now three places the repeatable-vs-variadic distinction has gone wrong: the conformance harness (#799), the derive's attribute vocabulary, and now its spec emission. Each time the symptom differed — a stolen positional, then a flag that claims both Two tests now assert the emitted spec for both shapes side by side ( The bare-short fix needed a test where the field name and the renamed name start with different letters — 21 tests. Also rebased on the updated #802 and #801. AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
|
One new finding fixed; the other three in that batch are re-posts of what the previous two commits already fixed — the bots re-review the cumulative diff, so those threads read worse than the code is. Verified in the current tree: New: a short of That is the fifth character class a short cannot be, and they now sit together in one place: non-ASCII, digits, AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
8fed243 to
da412c7
Compare
usage-derive reads a Rust type and emits the two trees the previous PRs defined: static parse tables for usage-argv, static metadata for the spec, and a parse function that is a match on table keys assigning straight into the struct's fields. Nothing is built at run time — `command()` hands back a static — and a successful parse touches only the tables. One declaration is now enough to get a parser, a spec, markdown, a manpage, and grouped help. A field with `long` or `short` is a flag and anything else is positional; help comes from the doc comment, first paragraph short and the whole thing long. Scope is one command per struct. Subcommands need an enum, cross-type table references, and a nested path through the parse function, which is its own PR. Values are held as the text they arrive as, because converting them is also where `env`, required-ness, and choices get enforced and that layer does not exist yet — so any other field type is a compile error rather than a surprise, and PLAN.md carries both as boxes. Twelve tests parse a deliberately awkward CLI and check the same declaration emits a spec usage-lib accepts, renders as docs, and groups by heading. The error messages get their own attention: a wrong `short = "j"` says to write `short = 'j'`, a duplicate flag points at both declarations, and an argument after a variadic one explains why it could never be filled. Not published yet: a CLI framework that cannot express subcommands is not one to depend on by accident. The version tracks the workspace so it is ready when it can. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five review findings, all real. The first is the distinction #799 fixed in the conformance harness, recurring here: `var` was setting the parser's greedy collection, so `--include a b` gave the flag both values and stole the positional that `b` should have filled. The two are now spelled the way a spec spells them — `var` for a flag that may be repeated, `variadic` for one occurrence that keeps taking values — and a `Vec` flag is repeatable without having to say so. The original test missed this by always writing `--include` twice. A negation is another long form, so it now collides like one; two flags could previously answer to the same token with only the first reachable. Generic parameters were silently dropped from the generated impl, and are now refused with a reason. A non-ASCII `short` was truncated to one byte and could never be matched, so it is refused too. And a `u8` count given 256 occurrences panicked in debug and wrapped to zero in release; it saturates, with the accumulator typed as the field rather than inferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two more review findings. An explicit `long = "--no-color"` was stored with its dashes, while a token has its `--` taken off before it is matched — so the flag could never be reached, and the duplicate check could not see that it collided with a plain `no-color`. Explicit long names are stripped the way negations already were. Some ASCII shorts can never be delivered because the grammar spends the character on something else first: `-5` reads as a negative number so that `--offset -1` works, `--` ends flag parsing, and `=` separates a short from its value. Each is refused with the reason rather than compiled into a flag nobody can type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`#[usage(name = "--color", long)]` derived its long form from the raw name, so the flag answered to a token nobody could send — a token has its dashes taken off before it is matched. The spec name is stripped where it is read, which fixes the derived long form and keeps a dashed spelling out of the spec's flag name at the same time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…late Three more findings, and two of them are the same distinction biting again. A `variadic` flag on a non-`Vec` field was accepted, and the generated code assigned each value in turn, so only the last survived. It now has to be a `Vec`, since that is the only field that can hold what the flag collects. Every `Vec` flag was also marked repeatable, including one declared only `variadic` — which emitted `var=#true` alongside a `...` argument, claiming both ways of collecting at once. `variadic` no longer implies `var`. And a bare `short` took the field's first letter at the moment it was read, so `short` before `name = "quiet"` gave `-v` for a field called `verbose`. It is resolved after all attributes are read, as a bare `long` already was. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A spec spells a flag's forms as one space-delimited string, so a short of `' '` or `'\t'` was written straight into it and produced a declaration nothing could read back. Refused, with the reason. That is the fifth character class a short cannot be, so they are now all checked in one place: non-ASCII, digits, `-`, `=`, and whitespace or control characters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Held back on the grounds that a CLI framework without subcommands is not one to depend on by accident. Publishing it instead: it is easier to argue with something that exists, the limits are stated plainly in the crate docs, and the version is shared so it tracks the rest of the workspace. The release script publishes it in dependency order, includes derive/ when computing the version bump and changelog, and stages its bumped manifest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0b820df. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
derive/src/codegen.rs (1)
136-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe helper functions trigger
dead_codewarnings in consumer crates.
textandvalue_textare nested functions insideparse_from. If a CLI declares onlyboolorcountflags and no positionals, neither helper is called, and the consumer crate getsdead_codewarnings from code it did not write. The#[allow(...)]at Line 67 applies to the hidden module only.♻️ Proposed change
+ #[allow(dead_code)] fn text(value: &[u8]) -> ::std::string::String { ::std::string::String::from_utf8_lossy(value).into_owned() } + #[allow(dead_code)] fn value_text(value: ::std::option::Option<&[u8]>) -> ::std::string::String { value.map(text).unwrap_or_default() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@derive/src/codegen.rs` around lines 136 - 141, Update the nested helper functions text and value_text inside parse_from so they do not emit dead_code warnings when no positional arguments use them. Apply an appropriate local allow attribute to these helpers, preserving their existing behavior and keeping the suppression scoped to the generated functions.derive/src/lib.rs (1)
43-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the required
usage-argvdependency and itsspecfeature.Generated code always imports
::usage_argv::spec::{ArgMeta, CommandMeta, FlagMeta, Spec}(derive/src/codegen.rs Line 69). A consumer crate that depends onusage-argvwithout thespecfeature gets an unresolved-import error inside the expansion, which is hard to trace back to the derive. Add one sentence to the crate docs that states the dependency and the feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@derive/src/lib.rs` around lines 43 - 65, Add a sentence to the crate-level documentation in derive/src/lib.rs stating that generated code requires the usage-argv dependency with its spec feature enabled. Place it near the existing declaration and usage documentation without changing the documented option behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@derive/src/codegen.rs`:
- Around line 136-173: Prefix all generated accumulator locals and parser
internals in the emitted parser code with a reserved __usage_ prefix to avoid
collisions with user field names; update field initialization, flag_arm,
arg_arm, helper functions, parser loop bindings, and event-pattern bindings
consistently. In the final Self struct literal, map each field explicitly from
its renamed accumulator local rather than relying on shorthand initialization.
- Around line 313-343: Update field_init for Shape::Many and Shape::Bool so
parsing applies defaults exactly as recorded by flag_meta and arg_meta:
initialize repeated fields from field.default rather than always using an empty
Vec, and use the model’s supported boolean-default semantics instead of only
recognizing the exact lowercase "true". If those defaults are unsupported,
reject them during model validation rather than emitting a spec that disagrees
with parsing.
In `@derive/src/model.rs`:
- Line 115: The default command name in the model generation path currently
passes the struct identifier directly through `to_kebab`, which does not convert
CamelCase. Add or reuse a helper that converts identifiers such as `MyTool` and
`Cli` to lowercase kebab-case, and use it for the `name` derived from
`input.ident`; keep `to_kebab` unchanged for snake_case field names.
- Around line 383-409: Add validation alongside the existing !is_flag checks in
the model’s field-kind validation to reject positional bool fields lacking both
long and short forms. Detect the bool field shape/type before code generation
and return a syn::Error anchored at span, so derive/src/codegen.rs::arg_arm is
never asked to assign text to a bool.
---
Nitpick comments:
In `@derive/src/codegen.rs`:
- Around line 136-141: Update the nested helper functions text and value_text
inside parse_from so they do not emit dead_code warnings when no positional
arguments use them. Apply an appropriate local allow attribute to these helpers,
preserving their existing behavior and keeping the suppression scoped to the
generated functions.
In `@derive/src/lib.rs`:
- Around line 43-65: Add a sentence to the crate-level documentation in
derive/src/lib.rs stating that generated code requires the usage-argv dependency
with its spec feature enabled. Place it near the existing declaration and usage
documentation without changing the documented option behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e8636906-488a-46f1-8676-e38bc12ef92c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlPLAN.mdconformance/Cargo.tomlconformance/tests/derive.rsderive/Cargo.tomlderive/src/codegen.rsderive/src/lib.rsderive/src/model.rstasks/release-plz
| fn field_init(field: &Field) -> TokenStream { | ||
| let ident = &field.ident; | ||
| match field.shape { | ||
| Shape::Bool => { | ||
| // A negatable flag's default is whatever `default` says, since | ||
| // `--no-x` has to be able to turn something off. | ||
| let start = field.default.as_deref() == Some("true"); | ||
| quote!(let mut #ident = #start;) | ||
| } | ||
| Shape::Count => { | ||
| // Typed, so `saturating_add` below resolves to the field's own integer. | ||
| let ty = &field.ty; | ||
| quote!(let mut #ident: #ty = 0;) | ||
| } | ||
| Shape::Optional => { | ||
| let default = match field.default.as_deref() { | ||
| Some(d) => quote!(::std::option::Option::Some(#d.to_string())), | ||
| None => quote!(::std::option::Option::None), | ||
| }; | ||
| quote!(let mut #ident = #default;) | ||
| } | ||
| Shape::Required => { | ||
| let default = match field.default.as_deref() { | ||
| Some(d) => quote!(#d.to_string()), | ||
| None => quote!(::std::string::String::new()), | ||
| }; | ||
| quote!(let mut #ident = #default;) | ||
| } | ||
| Shape::Many => quote!(let mut #ident = ::std::vec::Vec::new();), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The spec records defaults that the parse does not apply.
Two cases diverge from flag_meta/arg_meta, which always write field.default into the spec:
- Line 341:
Shape::Manystarts as an emptyVecand ignoresfield.default. AVec<String>field withdefault = "x"documentsxbut parses to[]. - Line 319:
Shape::Boolcompares against the exact string"true".default = "TRUE"ordefault = "1"silently yieldsfalse.
Either apply the default in both shapes, or reject the unsupported combinations in model.rs so the spec and the parse agree.
♻️ One option for `Shape::Many`
- Shape::Many => quote!(let mut `#ident` = ::std::vec::Vec::new();),
+ Shape::Many => match field.default.as_deref() {
+ Some(d) => quote!(let mut `#ident` = ::std::vec![`#d.to_string`()];),
+ None => quote!(let mut `#ident` = ::std::vec::Vec::new();),
+ },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@derive/src/codegen.rs` around lines 313 - 343, Update field_init for
Shape::Many and Shape::Bool so parsing applies defaults exactly as recorded by
flag_meta and arg_meta: initialize repeated fields from field.default rather
than always using an empty Vec, and use the model’s supported boolean-default
semantics instead of only recognizing the exact lowercase "true". If those
defaults are unsupported, reject them during model validation rather than
emitting a spec that disagrees with parsing.
Seven findings, and the first is the one that would have bitten a real adopter. Generated locals were named after the fields, and the parse function also defines helpers called `text`, `value_text`, `parser`, and `argv` — so a CLI with a field of any of those names failed to compile, or worse, read the wrong binding. Every generated name now carries a `__usage_` prefix and the struct is built with explicit `field: local` pairs, so a field can be called anything. A test declares six fields named after the internals. The rest are declarations that recorded something the generated code would not honor, which is this crate's recurring failure mode: - `var` together with `variadic` claimed both ways of collecting at once - a `bool` positional had nowhere to put the word it was given - a `bool` default other than true/false, a `count` default, and a default on a collecting field were written into the spec and never applied - a `long` of only dashes stripped to nothing and could never be matched - a bare `long` alongside an explicit one kept the pre-rename name And a `CamelCase` struct kept its spelling as the command name, so `MyLittleCli` is now `my-little-cli` — which is what it would be typed as. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Stacked on #802 (which is stacked on #801) — review those first; this diff includes them until they merge.
The piece the last three PRs were building toward: one Rust type in, a parser and a spec out.
That gives you
Cli::parse_from(argv),Cli::command(),Cli::spec(), andCli::to_kdl()— so the same declaration feedsusage g markdown|manpage, the completion generators, and grouped help output from #802.What's generated
Three things, and the split is the design:
staticparse tables — all a successful parse reads.staticmetadata — what spec emission and help need, which a parse never touches.matchon table keys assigning straight into the struct's fields. No map to build and read back, nothing allocated that does not end up in the result.command()returns a&'static, so there is no command tree to construct before parsing starts. A test asserts the pointer is the same every call, which is the property the whole project exists for.A field with
longorshortis a flag; anything else is positional. Help comes from the doc comment — first paragraph short, whole comment long.Scope, stated plainly
One command per struct. Subcommands need an enum of variants, cross-type table references, and a nested path through the parse function; that is its own PR and a box in
PLAN.md.Values are text —
bool,String,Option<String>,Vec<String>, or an unsigned integer withcount. Converting to other types is also whereenv, required-ness, andchoicesget enforced, and that layer does not exist yet. SoOption<u32>is a compile error explaining exactly that, rather than something that silently half-works.The error messages got real attention
They are the surface an author actually interacts with, so:
short = "j"→ a short flag is a character: writeshort = 'j'--flag→ points at both declarations, second firstcounton aString→ says it has to be an unsigned integerTests
Twelve, over a deliberately awkward CLI: attached and bundled shorts,
--flag=value, repeated flags, a negation turning off a default, a hidden flag that still parses,--passthrough, and a typo reported rather than bound. Then the same declaration is checked to emit a spec usage-lib accepts field by field, render as markdown and a manpage, and group by heading. Two more CLIs cover the empty cases — no flags, no positionals — which is where generated code tends to break on an unused variable or an emptymatch.Not published: a CLI framework that cannot express subcommands is not one to depend on by accident. The version tracks the workspace so it is ready the moment it can.
AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Medium Risk
Large new proc-macro surface that defines CLI parsing behavior for adopters; mitigated by extensive conformance tests but still pre-v1 (no subcommands, limited types, env not enforced at parse time).
Overview
Adds the
usage-deriveworkspace crate and#[derive(Cli)], so a single struct with#[usage(...)]attributes becomes ausage-argvparser, static spec metadata, and KDL for docs/completions.Generated code exposes
parse_from/parse,command()andspec()as&'statictables, andto_kdl(). Parsing is a directmatchon flag/arg keys into prefixed locals (avoids field-name clashes). The model layer rejects invalid declarations at compile time (duplicate flags,varvsvariadic, unsupported types, dashed long/name normalization, etc.) with targeted errors.v0 scope: one command per struct; text-ish field types only (
bool,String,Option<String>,Vec<String>, counting integers). Subcommands and typed value conversion are explicitly deferred in PLAN.md.Conformance gains
conformance/tests/derive.rsend-to-end tests (parsing, spec round-trip, markdown/manpage/help). Release tooling includesusage-derivein publish and git-cliff paths.Reviewed by Cursor Bugbot for commit 973a60a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
usage-derive, enabling CLI definitions through a#[derive(Cli)]macro.Documentation
Tests