fix(argv): inherit unknown_flags, which reached one command out of a tree - #939
Conversation
|
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 |
Greptile SummaryThe PR moves
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(argv): inherit `unknown_flags`, whic..." | Re-trigger Greptile |
Instruction counts
No instruction-count regression above 1%. 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. Shadow comparisonParsing
|
86026ea to
0c6a382
Compare
0c6a382 to
3313d52
Compare
…a tree usage-lib resolves this by walking outward from the command that ran and falling back to the spec's own setting — `effective_unknown_flags` in `lib/src/parse.rs`. usage-argv held the effective value per command instead, on the theory that whoever built the tables would resolve it. A derive cannot. It expands one struct at a time and cannot see the command above, so `#[usage(unknown_flags = "error")]` reached the root alone — and on an `Args` it was parsed and then ignored, a declaration that compiled and did nothing. A CLI could not make a subcommand strict, or a single command lenient, whatever it wrote. So `Command::unknown_flags` is an `Option` now: `None` means inherit, and the parser carries the effective value down as it descends rather than reading it off the command in scope. That is one branch on descent and a field read instead of a pointer chase on the two sites that ask. The corpus's table builder stops resolving inheritance on the way in, which is the part worth noticing: it had its own correct implementation of the rule, and that is exactly why no vector ever caught the parser not having one. One implementation now, in the parser, where the reference has it. Costs **160 instructions per parse — 72,272 against 72,112** at mise's scale, measured twice against the parent branch on one machine. Found by converting usage-cli itself to the derive, where every command wants the strict reading and the five that hand a command line to somebody else's script want the lenient one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3313d52 to
2ff6307
Compare
## Summary - accept `alias` and `alias_hidden(...)` on derived `Args` command structs - merge struct aliases with any enum-variant aliases for backwards compatibility - reject duplicate command names and aliases at compile time - document the migration-friendly placement and cover it with conformance tests This lets Clap-shaped command structs keep their command metadata when moving to `usage_derive`, so callers no longer need to relocate aliases into the parent enum. ## Stack Targets jdx#939 (`claude/derive-unknown-flags-inherit`). jdx#936 should be retargeted onto the final PR in this stack. ## Checks - `cargo test -p usage-conformance --test subcommands` - `cargo test -p usage-derive` - `cargo test -p usage-argv` - full stacked `cargo test --all --all-features` - full stacked Clippy with warnings denied _This PR was generated by Codex._ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes derive codegen and static command tables used by every CLI; mistakes could break routing, help, or specs, though conformance tests cover the new alias paths. > > **Overview** > **Command aliases can live on derived `Args` structs** (clap-style migration), not only on `Subcommands` variants. Visible and hidden aliases on the struct and on the mounting variant are **joined at compile time** via new `concat_aliases`, and emitted into both parse tables and metadata. > > **Root `Cli` still cannot declare aliases** — those attributes are rejected there as misplaced. **Duplicate aliases on one command** fail at derive time; **`assert_unique_subcommand_names`** panics at compile time if any two subcommands share a name or alias after merging struct + variant tables. > > Conformance tests move install/remove aliases onto the `Args` types and assert spec + parsing behavior (including hidden vs visible aliases). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6a4511b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…#965) Stacked on #963, the `usage-rs` facade. `usage` is now its own first adopter. Its command structs, root, and command enums use the `usage-rs` facade instead of Clap, and `--usage-spec` prints `Cli::to_kdl()` from the same tables that parsed the command line. ## What goes away - `clap`, `clap_usage`, and the `clap-sort` dev dependency - `tests/clap_sort.rs`; declaration order is held by the spec - the duplicated command-effect tables; effects now live on the commands they describe - the empty `Sponsors` struct; unit subcommands are supported directly - runtime checks for relationships the spec can express ## Migration shape The declarations remain close to their former Clap layout: - inferred shorts use `#[usage(short, long)]` - command aliases live on their command structs, e.g. `#[usage(alias = "c", alias_hidden("complete", "completions"))]` - positive dependencies use `requires`, including `--cache-key` → `--usage-cmd` and `--out-dir` → `--multi` - aliases written on enum variants still merge with struct aliases for compatibility - deliberately formatted docs keep `#[usage(verbatim_doc_comment)]` - path hints keep `value_hint = usage_rs::ValueHint::FilePath` or `DirPath` from usage’s own runtime type - subcommand payloads stay unboxed as they were under Clap; `Box<T>` remains an optional size optimization The four shell commands are separate derived structs flattening a shared group. A single destination struct cannot identify which enum variant selected it, so a macro keeps their common declaration in one place. ## Parity gaps closed by the stack - required subcommands survive KDL generation (#937) - strict unknown-flag behavior is inherited by subcommands (#939) - command-owned visible and hidden aliases are supported (#946) - duplicate non-repeatable flags are rejected (#945) - a missing required subcommand prints the available command choices (#947) - verbatim doc-comment layout is preserved on commands, fields, and variants (#949) - file and directory value hints reach native and emitted completions (#951) The emitted spec still intentionally gains metadata the Clap bridge dropped, including `JDX_USAGE_BIN` on `--usage-bin`, preserved multiline long help, the manpage description, and the correct binary name `usage`. ## Verification - `mise run render` - `cargo test --all --all-features` - `cargo clippy --all --all-features --all-targets -- -D warnings` - direct binary checks for no-argument help, duplicate flags, command aliases, and positive requirements _This PR was generated by Codex._ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how the primary `usage` CLI is parsed and documented (including `--completions` UX), but behavior is covered by updated integration tests and is an intentional migration to the shipped parser. > > **Overview** > The **`usage` binary dogfoods `usage-rs`**: command structs use `#[derive(usage_rs::Cli)]` / `#[usage(...)]` instead of Clap, and **`--usage-spec` emits `Cli::to_kdl()`** from the same parse tables that handle argv. **Clap, `clap_usage`, and `clap-sort` are removed**, along with the post-hoc **`command_effects` patch table**—`effect`, `requires`, `overrides`, and similar metadata now live on the declarations. > > **Root CLI shape is corrected in spec and docs**: **`--completions <SHELL>` is a long flag** (not a positional), with an **explicit multi-line `usage` synopsis** for `usage <COMMAND>` / `--completions` / `--usage-spec`. Shell runner commands (`bash`, `fish`, `zsh`, `powershell`, `exec`) use **`unknown_flags = "value"`** so script arguments can include unknown flags. > > **Help and manpages** gain support for an optional root **`usage` string** on `Spec` (derive + KDL emission); root help/man synopsis prefer that over generated lines. Generated assets (`usage.usage.kdl`, `usage.1`, Fig spec, CLI reference markdown, snapshots) are refreshed to match. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3aba1aa. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `--completions <SHELL>` for selecting a completion shell. * Improved command validation, aliases, defaults, environment settings, and file-output completion hints. * Shell commands now forward unrecognized arguments as script values. * Added clearer help text and descriptions for generation commands. * **Documentation** * Updated CLI references, manpages, usage specifications, and shell documentation to reflect revised options and behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Replaces #936 after flipping the facade and CLI layers. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Stacked on #937 (and so on #923) — review those first; this PR's diff is the last commit. The second of the two gaps #936 found by converting usage-cli itself to the derive.
usage-lib resolves this by walking outward from the command that ran and falling back to the spec's own setting —
effective_unknown_flagsinlib/src/parse.rs. usage-argv held the effective value per command instead, on the theory that whoever built the tables would resolve it.A derive cannot. It expands one struct at a time and cannot see the command above, so
#[usage(unknown_flags = "error")]reached the root alone — and on anArgsit was parsed and then ignored, a declaration that compiled and did nothing. A CLI could not make a subcommand strict, or a single command lenient, whatever it wrote.Without it,
usage lint --nope f.kdloffers--nopeto the positional, so the file becomes--nopeand the real file is the unexpected word — where clap named the flag.Shape
Command::unknown_flagsis anOptionnow:Nonemeans inherit. The parser carries the effective value down as it descends rather than reading it off the command in scope — one branch on descent, and a field read instead of a pointer chase on the two sites that ask.The KDL writer follows: a command that states nothing writes nothing, one that restates what it inherited writes nothing, and one that differs says so. Same output as before for every spec that already round-tripped.
The corpus's table builder stops resolving inheritance on the way in, which is the part worth noticing:
conformance/src/argv.rshad its own correct implementation of the rule, and that is exactly why no vector ever caught the parser not having one. One implementation now, in the parser, where the reference has it.Cost
160 instructions per parse — 72,272 against 72,112 at mise's scale, measured twice against the parent branch on one machine. cachegrind is deterministic; both figures repeated.
Verification
conformance/tests/unknown_flags_inherit.rs— five tests: a subcommand inherits the root's answer, it reaches two levels down, a command that declares its own keeps it, the emitted spec says what the parser does, and usage-lib reads the emitted spec the same way, which is the claim that matters.a_subcommand_writes_unknown_flags_only_where_it_differsgains a command that declares nothing at all, which is the case theOptionadds.This PR was generated by Claude Code.
Note
Medium Risk
Changes core argv binding for unrecognized flags across the whole CLI tree; behavior fixes are intentional but affect every parse path that relied on the old pre-resolved tables.
Overview
unknown_flagsnow inherits down the command tree instead of being baked into each command table.Command::unknown_flagsisOption<UnknownFlags>(None= inherit); the parser keeps an effective mode on descent and only updates it when a subcommand sets one.The derive emits
Nonefor commands that do not declare the attribute (fixing root-onlyerrorand ignored#[usage(unknown_flags)]onArgs). KDL emission and the conformance table builder stop flattening inheritance on build—only the parser applies the rule, matching usage-lib’s outward walk.New
conformance/tests/unknown_flags_inherit.rscovers inheritance depth, per-commandvalue, spec round-trip, and usage-lib parity on emitted KDL.Reviewed by Cursor Bugbot for commit 2ff6307. Bugbot is set up for automated code reviews on this repo. Configure here.