feat(derive): nest commands to any depth - #818
Conversation
mise reaches four levels — `mise bootstrap macos launchd-agents apply` — so one was never going to be enough. A command inside a command is now not a special case: an `Args` struct carries a `subcommand` field exactly as the root does, and generates the same code for it. What made that possible was pulling the wiring out of the root's emitter into one place both use — the tables to splice, the state to carry, how an event is routed, how the field is built — so the root differs from a nested command only in how it is entered. Two things came out of that. `build` and `select` are fallible now, because a command can require a subcommand of its own and "none was given" is only knowable where the value has to exist. Every generated reference to the user's own types now sits at one scope. The root's post-binding checks used to be emitted beside the parse rather than inside the generated module, which was invisible until a nested command's check referred to the user's enum from there and `super::` escaped the crate root. Both emitters put the checks in the module and call them, so there is one answer to "where is this code" rather than two. Also drops the `Default` bound on `CommandArgs::Partial`, which `start` supersedes: nested state cannot be set up by a derived `Default`. Eight tests on a three-level CLI: routing to the deepest command, each level keeping its own flags, a global reaching any depth from either side, a middle command requiring one of its own, a deep command's requirements being its own rather than its parent's, and the nested spec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds recursive nested subcommand support to generated parsers. It centralizes routing and metadata, uses declaration fingerprints for command keys, adds post-binding validation, makes construction fallible, and adds conformance tests. ChangesNested subcommand parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant GeneratedApply
participant SubcommandParts
participant CommandArgsBuild
Parser->>GeneratedApply: route each parse event
GeneratedApply->>SubcommandParts: apply nested command routing
Parser->>SubcommandParts: run post-binding validation
SubcommandParts->>CommandArgsBuild: build selected nested command
CommandArgsBuild-->>Parser: return command or Error
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 arbitrarily deep derived subcommands and centralizes nested-command routing, validation, and construction. It also changes collision handling to position-based command selection and pointer-checked field dispatch.
Confidence Score: 4/5The PR is not yet safe to merge because token-identical argument structs still produce duplicate keys that make debug spec generation panic. The parser’s new pointer checks prevent colliding declarations from selecting the wrong partial state, but declaration-only fingerprints preserve duplicate keys and Files Needing Attention: derive/src/model.rs, derive/src/codegen.rs, argv/src/spec.rs Important Files Changed
Reviews (4): Last reviewed commit: "fix(derive): select a command by positio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
derive/src/codegen.rs (1)
818-825: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
unknown_flagsto nestedCommandtables.Parser::descendchecks only the descended command, butemit_argsleaves itsunknown_flagsatCommand::EMPTY(UnknownFlags::Value). A root configured withunknown_flags = "error"therefore accepts unknown flags after a subcommand. Preserve explicit nested settings and inherit the effective ancestor setting when unset.🤖 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 818 - 825, Update the generated nested Command tables in the codegen path around Parser::descend and emit_args so unknown_flags is propagated from the effective ancestor configuration instead of remaining Command::EMPTY’s UnknownFlags::Value. Preserve explicitly configured nested unknown_flags values, and ensure descendants inherit the root or parent setting when unset so Parser::descend rejects unknown flags consistently.
🧹 Nitpick comments (3)
derive/src/codegen.rs (1)
1232-1240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a non-path type.
in_modulereturns the type unchanged when it is not aType::Path(Line 424-426). No test covers that branch. One extra assertion locks the early return in place.💚 Proposed test addition
assert_eq!(rendered("self::Commands"), "super::Commands"); assert_eq!(rendered("super::Commands"), "super::super::Commands"); + assert_eq!(rendered("super::cmds::Commands"), "super::super::cmds::Commands"); + // Not a path, so it is left alone. + assert_eq!(rendered("(A,B)"), "(A,B)"); }🤖 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 1232 - 1240, Add a test assertion in a_path_is_qualified_for_the_generated_module covering a non-Type::Path type, and verify rendered returns that type unchanged through the in_module early-return branch.conformance/tests/nesting.rs (1)
18-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a nested
Option<T>subcommand to the fixtures.Every
subcommandfield here is a bareT. The optional branch of the generated build (derive/src/codegen.rsLines 678-679) therefore never runs in this suite, and the new nested path is exactly where "no subcommand given" must produceNoneinstead ofError::MissingSubcommand. Give one nested command anOptionsubcommand field and assert both the absent and present cases.💚 Proposed fixture and test
/// Show every value #[derive(Args)] struct SettingsLs { /// As JSON #[usage(long)] json: bool, + /// Narrow the listing + #[usage(subcommand)] + command: Option<SettingsLsCommands>, } + +#[derive(Subcommands)] +enum SettingsLsCommands { + /// Only the changed ones + Changed(SettingsLsChanged), +} + +/// Only the changed ones +#[derive(Args)] +struct SettingsLsChanged {}#[test] fn a_nested_optional_subcommand_may_be_left_out() { let a = argv(["settings", "ls"]); let ex = Ex::parse_from(&a).expect("should parse"); let Commands::Settings(settings) = ex.command else { panic!("expected settings"); }; let SettingsCommands::Ls(ls) = settings.command else { panic!("expected settings ls"); }; assert!(ls.command.is_none(), "an Option subcommand may be absent"); let a = argv(["settings", "ls", "changed"]); let ex = Ex::parse_from(&a).expect("should parse"); let Commands::Settings(settings) = ex.command else { panic!("expected settings"); }; let SettingsCommands::Ls(ls) = settings.command else { panic!("expected settings ls"); }; assert!(ls.command.is_some(), "and present when the word is given"); }🤖 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/nesting.rs` around lines 18 - 79, Extend the nested command fixtures by adding an Option<T> subcommand field to SettingsLs, then add a test covering both ["settings", "ls"] producing None and ["settings", "ls", "changed"] producing Some. Reuse the existing Ex, Commands, SettingsCommands, and SettingsLs parsing paths and assert the expected nested variants.argv/src/spec.rs (1)
750-754: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the starting-state rule for
Subcommands::Partialtoo.
CommandArgs::Partialnow dropsDefaultbecause a fresh partial must come fromstart().Subcommands::Partialat Line 804 keeps theDefaultbound, and the generatedDefaultimpl calls each variant'sCommandArgs::start(). A hand-writtenSubcommandsimplementation that derivesDefaultinstead would silently drop every declared default of every variant. State that requirement on the associated type.📝 Proposed doc change
pub trait Subcommands: Sized { /// Values collected for whichever variant is being filled. + /// + /// `Default` must produce each variant's [`CommandArgs::start`] state, not a + /// derived zero value: a variant's declared defaults have to be in place before + /// parsing, since nothing afterwards distinguishes them from what was typed. type Partial: 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 `@argv/src/spec.rs` around lines 750 - 754, Update the documentation for the Subcommands::Partial associated type near its Default bound to state that fresh partial state must be produced through the appropriate start() method, preserving all variant-declared defaults. Explain that deriving or manually using Default can discard those defaults, and keep the existing bound unchanged.
🤖 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 153-164: Update both generated check bodies in
derive/src/codegen.rs: the root check at lines 153-164 and the nested check
emitted by emit_args at lines 863-868. Add a harmless reference to partial as
the first statement before `#post` in each body, preserving the existing
post-check logic and preventing unused-variable warnings when `#post` is empty.
- Around line 527-536: Update the comments immediately above the Partial struct
generation in the quote! block to reflect that partial_defaults always
constructs Partial with an explicit struct literal. Remove the outdated claims
about using or conditionally omitting Default, while preserving the explanation
of the partial’s initial values and subcommand handling.
---
Outside diff comments:
In `@derive/src/codegen.rs`:
- Around line 818-825: Update the generated nested Command tables in the codegen
path around Parser::descend and emit_args so unknown_flags is propagated from
the effective ancestor configuration instead of remaining Command::EMPTY’s
UnknownFlags::Value. Preserve explicitly configured nested unknown_flags values,
and ensure descendants inherit the root or parent setting when unset so
Parser::descend rejects unknown flags consistently.
---
Nitpick comments:
In `@argv/src/spec.rs`:
- Around line 750-754: Update the documentation for the Subcommands::Partial
associated type near its Default bound to state that fresh partial state must be
produced through the appropriate start() method, preserving all variant-declared
defaults. Explain that deriving or manually using Default can discard those
defaults, and keep the existing bound unchanged.
In `@conformance/tests/nesting.rs`:
- Around line 18-79: Extend the nested command fixtures by adding an Option<T>
subcommand field to SettingsLs, then add a test covering both ["settings", "ls"]
producing None and ["settings", "ls", "changed"] producing Some. Reuse the
existing Ex, Commands, SettingsCommands, and SettingsLs parsing paths and assert
the expected nested variants.
In `@derive/src/codegen.rs`:
- Around line 1232-1240: Add a test assertion in
a_path_is_qualified_for_the_generated_module covering a non-Type::Path type, and
verify rendered returns that type unchanged through the in_module early-return
branch.
🪄 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: 35275801-9a73-44ab-a972-52421e86b80a
⛔ Files ignored due to path filters (1)
conformance/tests/snapshots/nesting__the_emitted_spec_reads_like_a_handwritten_one.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
argv/src/spec.rsconformance/tests/nesting.rsderive/src/codegen.rsderive/src/lib.rs
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.
|
Three review findings, and writing the test for the first one exposed a second bug of my own. Keys hashed the type's *name*, so two same-named structs in different modules got the same key and the wrong command was selected. They now hash the whole declaration: a macro cannot see a module path, but it can see the item, and two types have to be identical to collide. The duplicate-key assertion remains the backstop. Writing that test turned up the second one: the check refusing two variants that wrap the same struct compared rendered type *names*, and `type_name` renders only the last segment — so `add::Op` and `remove::Op` looked identical and two perfectly good commands were refused. It compares whole paths now. Also: `check` reads its parameter unconditionally, since a command that declares nothing to check would otherwise leave it unused in the user's crate where nobody can silence it; and a comment about deriving `Default` on the partial no longer describes what the code does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Three findings fixed, and writing the test for the first exposed a second bug of mine — which is becoming the pattern worth noting. Keys hashed the type's name, so two same-named structs in different modules got the same key and the wrong command was selected. They now hash the whole declaration: a macro cannot see a module path, but it can see the item, so two types have to be identical to collide. The duplicate-key assertion in Then the test for that refused to compile — and the message was mine:
Two smaller ones: 9 tests on this file. The new one asserts both commands route correctly and that AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/model.rs`:
- Around line 14-20: Update the fingerprint generation used by the derive
expansion to include the declaration-site module path and type name, rather than
serializing only the annotated item; ensure distinct same-shaped command types
in different modules produce different command keys while preserving duplicate
detection for truly identical identities. Add a regression test defining
identically declared command structs in two modules and verify their emitted
commands route to distinct keys without triggering Spec::to_kdl duplicate-key
handling.
🪄 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: ae008973-d177-45a7-8fd4-4fb6cf7a242a
📒 Files selected for processing (3)
conformance/tests/nesting.rsderive/src/codegen.rsderive/src/model.rs
| /// What this type's keys are derived from. | ||
| /// | ||
| /// The whole item rather than its name: two same-named structs in different | ||
| /// modules would otherwise hash alike, and a macro cannot see a module path. Two | ||
| /// types now have to be *identical* to collide, which the duplicate-key assertion | ||
| /// still catches. | ||
| pub fingerprint: String, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not use an unqualified declaration as command identity.
Line 144 serializes only the annotated item. It does not include the enclosing module path. Therefore, identically declared add::Op and remove::Op types receive the same fingerprint. Lines 998-1018 permit those distinct wrapper paths, but emit and emit_args derive the same command key from this value. Routing can select the wrong command, and Spec::to_kdl can hit its duplicate-key assertion.
Generate the key identity from the declaration-site module path and type name in the expansion. Add a regression test with identically declared command structs in two modules.
Also applies to: 144-144
🤖 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/model.rs` around lines 14 - 20, Update the fingerprint generation
used by the derive expansion to include the declaration-site module path and
type name, rather than serializing only the annotated item; ensure distinct
same-shaped command types in different modules produce different command keys
while preserving duplicate detection for truly identical identities. Add a
regression test defining identically declared command structs in two modules and
verify their emitted commands route to distinct keys without triggering
Spec::to_kdl duplicate-key handling.
Hashing the whole declaration narrowed collisions to two byte-identical structs in different modules, but narrowing is not the same as fixing, and the failure mode was still "one command's flag fills another's field". Each arm now checks that the event came from its own table. `static` items have distinct addresses, so the check is exact, and it costs one comparison on the arm that was already selected: the key still gets there in one jump. A collision now means an event goes unclaimed rather than misbound, and `to_kdl`'s duplicate-key assertion still turns it into a failed test rather than a puzzle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good push on this one — hashing the whole declaration narrowed the collision to two byte-identical structs in different modules, but narrowing is not fixing, and the failure mode was still "one command's flag fills another's field". There is a better answer than hashing harder: #key if ::core::ptr::eq(*flag, &FLAG_0) => { … }The key still gets to the right arm in one jump; the check costs one comparison on the arm that was already selected. A collision now means an event goes unclaimed rather than misbound — and (The other threads in this batch are re-posts from before the previous push: the unread AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib.rs`:
- Around line 88-92: Remove the duplicated trailing “Spec::to_kdl asserts...”
fragment in the documentation near the key-dispatch explanation, leaving one
complete sentence stating that Spec::to_kdl asserts the tree contains no
duplicate keys.
🪄 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: 6d8a75fd-39a9-481c-919c-62efecdfc14c
📒 Files selected for processing (2)
derive/src/codegen.rsderive/src/lib.rs
![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 5827931. Configure here.
The identity guard covered flags and arguments but not commands: which subcommand was reached, which one to check, and which variant to build all compared keys, so two commands whose keys collided could still be confused for one another. Guarding half of the dispatch is not guarding it. A command is now identified by its position in `COMMANDS`, found by matching the table's own address. That removes key comparison from command selection entirely, and turns `check` and `select` into a jump on a small integer rather than a chain of comparisons. Also drops a sentence my last commit had duplicated in the crate docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Right on both counts, and the second one mattered. The identity guard covered half the dispatch. Flags and arguments verified which table an event came from; commands still compared keys — so which subcommand was reached, which one to check, and which variant to build could all still be confused by a collision. Guarding half of it is not guarding it. A command is now identified by its position in COMMANDS.iter().position(|c| ptr::eq(*c, *cmd))Key comparison is gone from command selection entirely, and The duplicated AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
| let (about, long_about) = doc_comment(&input.attrs)?; | ||
| let mut cli = Cli { | ||
| ident: input.ident.clone(), | ||
| fingerprint: quote::ToTokens::to_token_stream(input).to_string(), |
There was a problem hiding this comment.
When separate modules contain token-identical Args structs, this declaration-only fingerprint assigns them identical command and field keys. The pointer guards prevent variant misbinding, but Spec::to_kdl still detects the duplicate keys and panics in debug builds, preventing an otherwise valid CLI from emitting its specification.
1 participant
Third of the stack. mise reaches four levels —
mise bootstrap macos launchd-agents apply— so #816's one-level limit had to go.A nested command is not a special case
An
Argsstruct carries asubcommandfield exactly as the root does:What made that cheap was pulling the wiring out of the root's emitter into one place both use — the tables to splice, the state to carry, how an event is routed, how the field is built. The root now differs from a nested command only in how it is entered.
Two consequences worth flagging
buildandselectare fallible. A command can require a subcommand of its own, and "none was given" is only knowable where the value has to exist.Every generated reference to a user type now sits at one scope. The root's post-binding checks were emitted beside the parse rather than inside the generated module — harmless until a nested command's check referred to the user's enum from there and
super::escaped the crate root:That was worth more than the fix: two emitters had drifted into putting the same code at different scopes, and the bug only appeared once the two met. Both now put the checks in the module and call them, so there is one answer to "where does this code live". A unit test pins
in_module's behaviour for plain,crate::,::absolute,self::, andsuper::paths, since I had reasoned about it twice and been wrong once.Verified
Eight tests on a three-level CLI: routing to the deepest command, each level keeping its own flags, a global reaching any depth and working after the deepest command, a middle command requiring one of its own, a deep command's requirements being its own rather than its parent's (
settings set jobs→MissingRequired { name: "value" }whilesettings lsis fine), and the spec:What is left before a mise-shaped CLI is expressible
flatten, and theconflicts/requires/overridesfamily — which need the order flags arrived in, so they want a small ordering record. Then the bench harness and the gate.AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Medium Risk
Touches core derive codegen and trait APIs (
CommandArgs/Subcommands), including falliblebuild/selectand selection-by-position instead of key. Well covered by new nesting conformance tests, but hand-written trait impls would break.Overview
Enables arbitrarily nested subcommands — an
Argsstruct can carry asubcommandfield exactly as the root does, to any depth (needed for mise's four-level trees).Root and nested commands now share one
subcommand_partswiring path for tables, routing, checks, and builds.CommandArgs::buildandSubcommands::selectbecome fallible so a middle command can require its own subcommand. Selection uses table position (via pointer identity) instead of command keys, so key collisions cannot pick the wrong variant.Also hardens key assignment: fingerprints hash the whole declaration (not just the type name), and flag/arg match arms verify table identity so same-named structs in different modules cannot misbind. Post-binding checks move into the generated module so root and nested code share one scope.
Reviewed by Cursor Bugbot for commit 7822d16. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation