feat(derive): let a variant hold its command in a Box - #828
Conversation
📝 WalkthroughWalkthroughThe derive layer now supports boxed subcommand payloads. Generated ChangesBoxed subcommand support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VariantParser
participant DeriveCodegen
participant ShadowGenerator
participant ConformanceTests
VariantParser->>DeriveCodegen: provide payload type and boxed state
DeriveCodegen->>ShadowGenerator: generate Box-wrapped enum variants
ShadowGenerator->>ConformanceTests: expose boxed command definitions
ConformanceTests->>VariantParser: parse commands and compare specifications
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThe PR adds support for boxed subcommand payloads while keeping generated parser tables and emitted CLI specifications based on the inner command type.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (6): Last reviewed commit: "feat(derive): let a variant hold its com..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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. Shadow comparisonParsing
|
aaffa13 to
d05c43d
Compare
|
Correct, and it was a bad shortcut on my part: I read the box out of a rendered type string, and Taken apart syntactically now — the last path segment's angle-bracketed argument — which keeps the path and reads There's a test with the command declared in another module and referred to as AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
d05c43d to
06cbca6
Compare
06cbca6 to
90f6edf
Compare
90f6edf to
3c17ee6
Compare
|
The Worth knowing what the force-push was for, though: this branch had silently lost four of #826's fixes, because I restacked it by replaying a content patch captured against the older base, which reverts newer changes touching the same lines. Rebuilt on the corrected parent, with the fixes verified on the pushed ref. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
3c17ee6 to
fbba60b
Compare
An enum is as large as its biggest variant, so one subcommand with thirty flags makes every invocation of a mise-sized CLI move that much stack. mise answers that by boxing its largest commands — and, per jdx, by boxing them to stay out of trouble with clap at that size. `Install(Box<Install>)` now works, and the box is taken apart syntactically rather than by rendering the type and parsing it back: `type_name` keeps only the last segment, so `Box<cmds::Deep>` would have come out as `Deep` and named a type that is not in scope. `std::boxed::Box<T>` is read too. The box is an artifact of how the variant holds the struct: the tables, the partial and `build` all speak to the struct itself, and the box goes back on at the one point a value is made. Nothing about the spec changes, and a test checks the emitted KDL cannot tell. The shadow generator boxes every variant, which is what let the generated crates rejoin the workspace: `clippy::large_enum_variant` had been the reason they sat outside it, and answering the lint rather than silencing it means 12k lines of generated code are linted like everything else. It is also faster, which was not the point: moving a `Commands` as large as its biggest variant costs more than a malloc and a pointer.
fbba60b to
0d9f107
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
conformance/tests/subcommands.rs (1)
406-443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding negative coverage for
Box<T, A>.The PR states that a
Boxwith an allocator parameter is left unwrapped and fails using the user-provided type. No test records that behavior, so a later change to the unwrapping rule would go unnoticed. If the repository has a compile-fail harness such astrybuild, add a case forInstall(Box<Install, MyAlloc>).#!/bin/bash # Description: Check for an existing compile-fail test harness. set -euo pipefail rg -n 'trybuild|compile_fail|compiletest' --glob '!target' . fd -t d 'ui' derive conformance🤖 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 `@conformance/tests/subcommands.rs` around lines 406 - 443, Add negative compile-fail coverage for the qualified boxed-command handling around QualifiedCommands, using a Box<T, A> declaration such as Install(Box<Install, MyAlloc>) and the repository’s existing compile-fail harness if available. Assert that the allocator-parameter form is not unwrapped and therefore fails with the user-provided type, preserving the current behavior.xtask/src/shadow.rs (1)
401-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider qualifying
Boxfor the Usage dialect.This file qualifies prelude types in generated Usage-dialect code. Line 340 emits
::std::option::Option<...>andDialect::vecemits::std::vec::Vec<::std::string::String>, while the Clap dialect uses the bare names. Line 405 emits a bareBox<...>for both dialects, which breaks that pattern. The generated code compiles today, so this is a consistency concern only.♻️ Proposed change to follow the existing dialect convention
- out.push_str(&format!(" {variant}(Box<{}>),\n", sub_ty.args)); + let boxed = match dialect { + Dialect::Usage => format!("::std::boxed::Box<{}>", sub_ty.args), + Dialect::Clap => format!("Box<{}>", sub_ty.args), + }; + out.push_str(&format!(" {variant}({boxed}),\n"));Note: the derive must accept
::std::boxed::Box<T>for this change. The PR description states that qualified paths are supported.🤖 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 `@xtask/src/shadow.rs` around lines 401 - 405, Update the generated type in the shadow output around the variant formatting to qualify Box for the Usage dialect as ::std::boxed::Box<...>, while preserving the Clap dialect’s existing bare Box form. Ensure the derive handling accepts the qualified path and keep the existing sub_ty.args generic payload unchanged.Cargo.toml (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine
workspace.default-membersfor faster contributor builds.The virtual workspace currently selects all members for bare root commands. Exclude
benches/gateand both generated shadow crates fromdefault-members; run them explicitly with--workspacein benchmark and lint jobs.🤖 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 `@Cargo.toml` around lines 12 - 18, Define the workspace.default-members list in Cargo.toml to include the regular workspace crates while excluding benches/gate, benches/shadows/mise, and benches/shadows/mise-clap. Keep those excluded crates available as workspace members so benchmark and lint jobs can still target them explicitly with --workspace.
🤖 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 `@PLAN.md`:
- Around line 191-197: Update the usage-derive v1 checklist entry to remove
“boxed subcommand variants” from the unsupported features list, keeping the
newly documented boxed generated command enum behavior consistent.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 12-18: Define the workspace.default-members list in Cargo.toml to
include the regular workspace crates while excluding benches/gate,
benches/shadows/mise, and benches/shadows/mise-clap. Keep those excluded crates
available as workspace members so benchmark and lint jobs can still target them
explicitly with --workspace.
In `@conformance/tests/subcommands.rs`:
- Around line 406-443: Add negative compile-fail coverage for the qualified
boxed-command handling around QualifiedCommands, using a Box<T, A> declaration
such as Install(Box<Install, MyAlloc>) and the repository’s existing
compile-fail harness if available. Assert that the allocator-parameter form is
not unwrapped and therefore fails with the user-provided type, preserving the
current behavior.
In `@xtask/src/shadow.rs`:
- Around line 401-405: Update the generated type in the shadow output around the
variant formatting to qualify Box for the Usage dialect as
::std::boxed::Box<...>, while preserving the Clap dialect’s existing bare Box
form. Ensure the derive handling accepts the qualified path and keep the
existing sub_ty.args generic payload unchanged.
🪄 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: 2b46f158-b0ff-401a-bd72-60b492536290
📒 Files selected for processing (9)
Cargo.tomlPLAN.mdbenches/shadows/mise-clap/src/lib.rsbenches/shadows/mise/src/lib.rsconformance/tests/subcommands.rsderive/src/codegen.rsderive/src/lib.rsderive/src/model.rsxtask/src/shadow.rs
| counts: 13 secondary flag aliases, 3 `double_dash="automatic"`, 2 mounts, 2 restart | ||
| tokens, 1 `default_subcommand`, 1 default on a collecting flag. The | ||
| `default_subcommand` is the one that changes the _root's_ grammar, since | ||
| `mise build` routes through `run` in mise and answers at the root in the shadow. | ||
| The generated crates are ordinary workspace members: their command enums are boxed, | ||
| as the real mise boxes its own, so `large_enum_variant` has nothing to say and no | ||
| lint is silenced anywhere. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the derive v1 checklist entry for boxed variants.
This paragraph now states that the generated command enums are boxed. The usage-derive v1 item earlier in the same file still lists "boxed subcommand variants" among the features that are not yet supported. This PR adds that support, so the two statements conflict. Remove that phrase from the v1 list.
🤖 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 `@PLAN.md` around lines 191 - 197, Update the usage-derive v1 checklist entry
to remove “boxed subcommand variants” from the unsupported features list,
keeping the newly documented boxed generated command enum behavior consistent.
…mmands nobody ran (#829) Stacked on #828. The last number the gate owed, and it turned up a real inefficiency on the way. ## The result | invocation | usage | clap | | --- | --- | --- | | bare `mise` | **0** | — | | `mise use -g node@20` | **3** | **6,560** | | `mise settings set experimental true` | **4** | — | At mise's scale — 211 commands, 711 flags — a parse with nothing to bind never reaches the allocator, and binding three or four words costs one allocation per value. clap builds its tree on the way to parsing and the tree is the CLI, every time. About **2,000× fewer**. ## The inefficiency it found Declared defaults were applied in `start()`, which builds the partial for **every** command in the CLI rather than the selected one. So a bare `mise` allocated **60 `String`s** for defaults it would never read — the CLI's size leaking into the invocation, which is the one thing this design exists to avoid. They run in `check()` now, which runs for the selected command only, guarded on `__given_*` so a negation that turned a defaulted `bool` off during the parse is not undone afterwards. That is what takes the bare parse from 60 allocations to 0. ## Two corrections to the instrument, which are the interesting part - **The counter was wrong.** Armed per thread but counting into a *global*, so tests running in parallel counted each other's allocations: a 4-allocation parse read as 24, and a 0-allocation one as 3, intermittently, depending on which tests overlapped. Both halves are thread-local now. **usage-argv's own zero-alloc counter had the same latent flaw** — harmless with one test, wrong the moment a second is added — so it counts per thread too. - **First-use costs are not the parser's.** The first parse on a fresh thread pays for standard-library setup; that alone read as 60 allocations rather than 3. The measurement warms up and takes the least of several runs, and says so. I also dropped a property I had written and could not defend: comparing the mise shadow against a hand-written one-command CLI and asserting the counts were *equal*. They differ for reasons that have nothing to do with size, so the assertion was wrong even though the property it was reaching for is real. `bare mise == 0` states that property exactly, on the CLI that matters. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes when defaults are applied in generated parse code (behavior should match for real invocations but affects every derived CLI); allocation tests rely on global allocator hooks and warmed measurements. > > **Overview** > Closes the gate’s **heap allocation** target with new `benches/gate/tests/allocations.rs` tests: bare `mise` at full shadow scale must allocate **0**, typical invocations only a handful (one per bound value), and clap stays **>1,000** on the same argv. **PLAN.md** records the results (0 / 3–4 vs clap’s 6,560) as all three runtime gates met. > > **usage-derive** no longer applies field **defaults in `start()`** (which ran for every command’s partial). Defaults run in **`check()`** for the selected command only, guarded on `__given_*` so negated booleans stay correct—fixing ~60 spurious `String` allocations on an empty argv. > > Allocation instrumentation in **`argv/tests/no_alloc.rs`** and the new gate tests uses **thread-local** armed flags and counters (not a global atomic), avoiding parallel-test flakiness; measurements **warm up** and take the minimum run to exclude first-use stdlib/allocator noise. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2bda275. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
The largest thing standing between the derive and mise. Independent of #827–#829, so it can go in either order. mise names `PathBuf` **227 times** and its tool-version type **83 times** in `src/cli/`. A derive that only holds `String` cannot hold mise's commands, however faithfully it expresses mise's spec. ```rust #[usage(short = 'j', long)] jobs: Option<usize>, #[usage(long)] out: Option<PathBuf>, #[usage(long)] port: u16, // bare: the type says it is required #[usage(long, var)] tool: Vec<Tool>, // any FromStr of your own #[usage(long, var)] search: Option<Vec<PathBuf>>, ``` `Option<Vec<T>>` earns its place: "never given" and "given nothing" are different answers, and mise's root asks for that distinction three times. ## What keeps it cheap The layering does not move. **Binding still collects text** — a word's meaning is not a question about where it lands — and conversion happens once, where the struct is built. So defaults, `env`, `choices` and the variadic bounds all keep working on text, untouched. **Measured on the same fixture as main: 40,129 instructions against 39,963.** +166, or 0.4%, nearly all of it bookkeeping the identity conversion cannot avoid. That comparison is worth a note. On #827/#828 this parse measures ~50.9k, and I nearly reported this branch's 40k as a 20% win — it is not, it is a *smaller shadow*, because main has neither those branches' 91 aliases nor their boxed variants. Same trap the informative-benchmark change was made for; the honest number needs the same fixture on both sides. ## Errors say what the type says ```rust Err(Error::InvalidValue(bad)) => { bad.name; // "jobs" bad.value; // "lots" bad.reason; // "invalid digit found in string" } ``` Better than anything this crate could invent about someone else's type: `u16` explains "number too large than can fit", and a tool argument explains itself. `Error` gained one variant and **lost `Copy`**, since that variant owns two strings — but the payload is boxed, so `Error` is still **40 bytes** and the `Result` it rides in on the hot path did not grow. I checked that rather than assuming it. A type no word could become is a compile error naming *that* type: ``` error[E0277]: the trait bound `HashMap<String, String>: FromStr` is not satisfied ``` ## One gap, deliberate and documented A word reaches a field through `from_utf8_lossy`, so a `PathBuf` holding a path that is not UTF-8 gets replacement characters rather than bytes. Rare, and wrong when it happens. The fix is for the partial to hold `OsString` and let `build` decide — exact for `PathBuf` and `OsString`, an error for `String` rather than a silent mangling — which is the next PR rather than a bigger one now. Also in here, from jdx's note that mise carries hacks purely to work around clap: PLAN.md now lists what adoption should let mise **delete**, checked against mise rather than assumed. Two of the five do not survive contact — `command_effects.rs` argues its own case for staying one central list, and `mise-extra.usage.kdl` turns out to be a docs link template rather than a workaround. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Public `usage_argv::Error` API changes (`Copy` removed, new variant) and derive codegen affects all parsers, but behavior is covered by new conformance tests and conversion stays off the successful parse hot path. > > **Overview** > **Typed CLI fields** move beyond string-only binding: a field can be any `T: FromStr`, plus `Option<T>`, `Vec<T>`, and `Option<Vec<T>>` (so “flag never given” vs “given but empty” stays distinct). The argv layer still binds text; **`usage-derive` converts when building the struct**, with a fast path that moves `std::string::String` without re-parsing. > > Failed conversions surface as **`Error::InvalidValue`** (boxed **`InvalidValue`** with field name, raw text, and the type’s `Display` message). **`Error` no longer implements `Copy`**; the new variant is boxed so the enum stays **40 bytes**. > > **Subcommand `Args` builds** use the same **`field_final`** logic as the root CLI (fixing typed fields on nested commands). > > **`PLAN.md`** marks typed values done, notes the next step (non–UTF-8 via `OsString` in partials), and adds a mise adoption section on what clap workarounds can be removed vs what should stay. > > **`conformance/tests/typed.rs`** covers numeric/path/custom types, `Option<Vec<_>>`, spec/KDL unchanged by Rust types, and subcommand conversion errors. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0501bb5. 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 type-driven conversion for CLI values, supporting numeric, path, custom `FromStr`, optional, and repeated fields. * Added support for optional collections, distinguishing absent values from empty collections. * Conversion failures now report the field, rejected value, and reason. * **Documentation** * Updated documentation for supported typed-value patterns and current non-UTF-8 handling limitations. * **Tests** * Added comprehensive coverage for typed fields, collections, conversion errors, and numeric validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Stacked on #827. This is the one you pointed at: "the purpose behind the boxing was to avoid clap issues when commands get too big."
An enum is as large as its biggest variant, so one thirty-flag subcommand makes every invocation of a mise-sized CLI move that much stack — and at that size clap itself starts to struggle, which is why mise boxes its largest commands.
The box is read from the written form, as
Option<T>is elsewhere. It is an artifact of how the variant holds the struct: the tables, the partial andbuildall speak to the struct itself, and the box goes back on at the single point a value is made. A test checks the emitted spec cannot tell the difference — boxing is not something a CLI has.What it unlocks
The generated shadows are ordinary workspace members again.
clippy::large_enum_variantwas the reason they sat outside the workspace, and boxing answers the lint instead of silencing it — so 12k lines of generated code are now linted like everything else, and there is still noallowanywhere in the repo.It is also slightly faster
Not the point, but worth recording: 51.1k instructions against 51.3k, because moving a
Commandsas large as its biggest variant costs more than a malloc and a pointer move. Wall clock 2.05µs, ratio still 117×.One note on process
My first version shadowed
name— the command's name — with the held type's name, so every command was suddenly called after its struct and no subcommand matched. The existing subcommand tests failed immediately, which is exactly the argument for having them; it took one diff to find.AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Medium Risk
Changes core derive codegen and regenerates very large benchmark shadows; parsing/spec behavior is covered by new conformance tests, but any derive bug affects all adopters.
Overview
usage-derivenow acceptsInstall(Box<Install>)(and mixed boxed/unboxed enums). The macro strips theBoxfor tables, partials, andbuild, then re-wraps withBox::newwhen constructing the enum variant. Types likeBox<cmds::Deep>are parsed syntactically so generated code keeps the full path. Emitted KDL/spec is unchanged.Shadow crates are first-class workspace members again.
xtask gen-shadowemits boxed subcommand variants (matching mise/clap practice), solarge_enum_variantno longer forcesexclude = ["benches/shadows"]. Regeneratedmiseandmise-clapshadows are the bulk of the diff.Conformance adds tests that boxed variants parse like unboxed ones and that boxing does not leak into the spec.
Reviewed by Cursor Bugbot for commit 0d9f107. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests