feat(derive): check what a parse cannot decide on its own - #817
Conversation
The parser binds tokens and stops there, which left required-ness, choices, variadic bounds, and `env` fallback unimplemented — the 18 corpus vectors the harness reports as post-binding, and the reason a subcommand field had to be `Option<T>`. Generated code now checks them after the last token, in an order that matters: the environment fills what argv left out, then required-ness (so a flag with `env` set is not missing), then choices and bounds, which judge a value however it arrived. Only the command that actually ran is judged — a flag that `install` requires says nothing about an invocation of `run` — which is why the check hangs off the `CommandArgs` trait rather than running for every variant. A bare `subcommand` field now means one is required, and says so when it is absent. Two decisions worth recording. A field remembers whether a token supplied it, rather than inferring that from an empty value: `--out=` is a value somebody typed, and treating "" as unset would make it unsayable. And bounds constrain the values a field was *given*, so an unused optional flag is absent rather than a violation — otherwise `var_min` would be a second spelling of required-ness, with no way left to say "at least two, if you use it at all". Contradictions are refused at compile time: `choices` on a `bool`, `var_min` above `var_max`, a bound on something that is not a `Vec`, a default outside its own choices. The declarations reach the spec too, which a test now pins — a choice list that the parser enforces but the spec omits would be invisible to docs and completions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe parser now performs post-binding validation for required values, choices, variadic bounds, environment fallback, and required subcommands. Public traits expose validation hooks, derive metadata records constraints, generated code validates selected commands, and conformance tests cover the behavior. ChangesPost-Binding Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GeneratedParser
participant Environment
participant SelectedCommand
participant ParsedValue
CLI->>GeneratedParser: supply arguments
GeneratedParser->>Environment: resolve unset fields
GeneratedParser->>SelectedCommand: validate selected command
SelectedCommand-->>GeneratedParser: return success or Error
GeneratedParser-->>ParsedValue: build validated value
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 |
6872b35 to
d642010
Compare
Greptile SummaryThe PR adds derive-generated post-binding validation after argv parsing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(derive): stop claiming a count came ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 d642010. Configure here.
![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
🧹 Nitpick comments (4)
conformance/tests/post_binding.rs (2)
74-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises
VarTooFew.
includedeclaresvar_min = 1, but the minimum can never be violated on this fixture.bound_checksruns only when__given_includeis true, and every--includeoccurrence pushes exactly one value. The count is therefore at least 1 whenever the check runs.The
minbranch ofbound_checksinderive/src/codegen.rsis currently untested. Add a field withvar_min = 2and give it one value.💚 Proposed test addition
/// Files to pair #[usage(long, var = true, var_min = 2)] pair: Vec<String>,#[test] fn too_few_values_is_refused() { let a = argv(["--pair", "a", "x"]); assert!(matches!( Ex::parse_from(&a), Err(Error::VarTooFew { name: "pair", min: 2, got: 1 }) )); }🤖 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/post_binding.rs` around lines 74 - 98, Extend the test fixture with a repeatable pair field configured with var_min = 2, then add a too_few_values_is_refused test passing one value and asserting Error::VarTooFew with name "pair", min 2, and got 1. Keep the existing include tests unchanged.
162-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an optional subcommand field.
These tests cover the required subcommand path only.
derive/src/codegen.rsat lines 126-139 branches onoptional, and theOption<T>branch has no test here. That branch must produceNonewhen no command is given, and it must still runcheckon a command that was given.💚 Proposed test addition
/// A CLI whose subcommand may be left out #[derive(Cli)] #[usage(bin = "opt")] struct Opt { /// What to do, if anything #[usage(subcommand)] command: Option<Commands>, } #[test] fn an_optional_subcommand_may_be_left_out() { let a = argv([]); assert!(Opt::parse_from(&a).expect("should parse").command.is_none()); // A command that was given is still judged. let a = argv(["install"]); assert!(matches!( Opt::parse_from(&a), Err(Error::MissingRequired { name: "tool" }) )); }
CommandswrapsInstallandNoop, and the model rejects two variants sharing one struct. ReuseCommandsrather than declaring a second enum over the same structs.🤖 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/post_binding.rs` around lines 162 - 205, Add a test near the existing subcommand tests defining an optional-subcommand CLI type, such as Opt with command: Option<Commands>. Verify parsing no arguments succeeds with command set to None, and parsing ["install"] still returns Error::MissingRequired for tool, confirming validation runs for provided commands while omission is allowed. Reuse the existing Commands enum.derive/src/codegen.rs (2)
1000-1009: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA count field with
envset is marked as given without receiving a value.For
Shape::Countthe assignment islet _ = value;, but the surrounding block still runspartial.#given = true;. The field keeps its zero value while__given_*claims a value arrived.Nothing reads that marker for a count field today:
required_checksonly coversShape::Required, and the model rejects choices and bounds on counting fields. The state is still contradictory, and a later check that consults__given_*would read it wrong.Consider skipping the field entirely instead of emitting a no-op assignment.
♻️ Proposed change
// A count from the environment would be a number, not an occurrence // count; nothing needs it yet. - Shape::Count => quote!(let _ = value;), + Shape::Count => return None, };🤖 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 1000 - 1009, Update the environment-variable assignment logic in the Shape::Count branch so count fields are skipped entirely when env is configured, rather than executing the shared partial.#given update. Preserve the existing behavior for other shapes, and ensure count fields do not emit a no-op assignment or mark their given-state flag.
769-780: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
unused_lifetimesto the generated module's allow list.
checkdeclares'tand'v, but the body uses neither, and only'tappears in the return type. The generated module inherits the deriving crate's lint configuration. A crate with#![deny(unused_lifetimes)]would then fail to compile on a derive it did not write.The other generated-module allow lists in this file already cover
non_snake_caseandunused_importsfor the same reason.♻️ Proposed change
#[allow( non_upper_case_globals, non_snake_case, unused_imports, + unused_lifetimes, // Fires when a metadata struct happens to be fully specified. `..EMPTY` // is kept on purpose: it is what lets usage-argv gain a metadata field // without breaking every crate that derives. clippy::needless_update )]🤖 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 769 - 780, Add unused_lifetimes to the generated module’s lint allow list so the generated check method’s unused generic lifetimes do not fail downstream crates configured with deny(unused_lifetimes). Update the existing generated-module allow attributes near the check method generation, preserving the current non_snake_case and unused_imports allowances.
🤖 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 `@argv/src/lib.rs`:
- Around line 288-319: Preserve 5.x compatibility across
argv/src/lib.rs:288-319, argv/src/spec.rs:775-781, and argv/src/spec.rs:804-808,
or explicitly release these changes as 6.0.0. For compatibility, avoid adding
breaking Error variants such as MissingRequired, InvalidChoice, VarTooFew,
VarTooMany, and MissingSubcommand, and give CommandArgs::check and
Subcommands::check default Ok(()) implementations so existing manual
implementations remain valid; generated implementations should continue
providing both checks.
In `@conformance/tests/post_binding.rs`:
- Around line 100-125: Move the_environment_fills_what_argv_left_out into a
separate integration test file containing no other tests that parse a struct
with env bindings, so its set_var/remove_var calls cannot race with sibling
tests. Keep the existing environment assertions and cleanup intact, and add the
required // SAFETY: comments for each unsafe environment mutation.
---
Nitpick comments:
In `@conformance/tests/post_binding.rs`:
- Around line 74-98: Extend the test fixture with a repeatable pair field
configured with var_min = 2, then add a too_few_values_is_refused test passing
one value and asserting Error::VarTooFew with name "pair", min 2, and got 1.
Keep the existing include tests unchanged.
- Around line 162-205: Add a test near the existing subcommand tests defining an
optional-subcommand CLI type, such as Opt with command: Option<Commands>. Verify
parsing no arguments succeeds with command set to None, and parsing ["install"]
still returns Error::MissingRequired for tool, confirming validation runs for
provided commands while omission is allowed. Reuse the existing Commands enum.
In `@derive/src/codegen.rs`:
- Around line 1000-1009: Update the environment-variable assignment logic in the
Shape::Count branch so count fields are skipped entirely when env is configured,
rather than executing the shared partial.#given update. Preserve the existing
behavior for other shapes, and ensure count fields do not emit a no-op
assignment or mark their given-state flag.
- Around line 769-780: Add unused_lifetimes to the generated module’s lint allow
list so the generated check method’s unused generic lifetimes do not fail
downstream crates configured with deny(unused_lifetimes). Update the existing
generated-module allow attributes near the check method generation, preserving
the current non_snake_case and unused_imports allowances.
🪄 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: 75aabba8-962e-4526-a20d-b5b70ad4f1e6
📒 Files selected for processing (7)
argv/src/lib.rsargv/src/spec.rsconformance/src/argv.rsconformance/tests/post_binding.rsderive/src/codegen.rsderive/src/lib.rsderive/src/model.rs
| // The rest are raised *after* the parse, by whoever owns the target type: they | ||
| // need to know a value's declared type, which the parser deliberately does not. | ||
| // They share this enum so that a caller has one error to handle rather than two. | ||
| /// Something the command requires was never given. | ||
| MissingRequired { | ||
| /// The flag or argument's name, as the spec calls it. | ||
| name: &'t str, | ||
| }, | ||
| /// A value was given that is not among the declared choices. | ||
| /// | ||
| /// Carries the choices rather than the offending value: rendering the value means | ||
| /// owning it, and an error that allocates on a path this crate promises not to | ||
| /// allocate on would be a poor trade for a better message. Diagnostics are a | ||
| /// separate layer. | ||
| InvalidChoice { | ||
| name: &'t str, | ||
| choices: &'t [&'t str], | ||
| }, | ||
| /// Fewer values than `var_min`. | ||
| VarTooFew { | ||
| name: &'t str, | ||
| min: usize, | ||
| got: usize, | ||
| }, | ||
| /// More values than `var_max`. | ||
| VarTooMany { | ||
| name: &'t str, | ||
| max: usize, | ||
| got: usize, | ||
| }, | ||
| /// A subcommand was required, and none was given. | ||
| MissingSubcommand, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect declared package versions and local manual trait implementations.
fd -a 'Cargo.toml' -x sh -c '
printf "\n== %s ==\n" "$1"
rg -n "^(name|version)\\s*=" "$1"
' sh {}
ast-grep outline . --items all --type impl --match 'CommandArgs|Subcommands'
rg -n -C 3 --glob '*.rs' \
'\bimpl(?:<[^>]*>)?\s+(?:[^ \t]+::)?(?:CommandArgs|Subcommands)\b' .Repository: jdx/usage
Length of output: 803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== workspace and package metadata =='
sed -n '1,120p' Cargo.toml
sed -n '1,100p' argv/Cargo.toml
printf '%s\n' '== relevant trait definitions and implementations =='
rg -n -C 12 \
'trait (CommandArgs|Subcommands)|impl[^\\n]*(CommandArgs|Subcommands)|fn check<' \
argv/src '*.rs' 2>/dev/null || true
printf '%s\n' '== public Error definition =='
rg -n -C 8 'pub enum Error|MissingRequired|InvalidChoice|VarTooFew|VarTooMany|MissingSubcommand' argv/src
printf '%s\n' '== repository-wide manual implementations =='
rg -n -C 8 --glob '*.rs' \
'impl(?:<[^>]*>)?\\s+(?:[^\\s]+::)?(?:CommandArgs|Subcommands)\\b' . || true
printf '%s\n' '== API and compatibility documentation =='
rg -n -C 3 --glob '*.md' --glob '*.toml' \
'breaking|major release|semver|compatib|CommandArgs|Subcommands' . || trueRepository: jdx/usage
Length of output: 25343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== spec module boundaries and check call sites =='
sed -n '1,80p' argv/src/spec.rs
rg -n -C 10 '::check\\(|\\.check\\(|CommandArgs::check|Subcommands::check|check\\(' \
argv/src derive/src lib/src cli/src 2>/dev/null || true
printf '%s\n' '== derive implementation generation =='
fd -a -e rs . derive -x sh -c '
if rg -q "CommandArgs|Subcommands|fn check|MissingRequired|InvalidChoice|VarTooFew|VarTooMany|MissingSubcommand" "$1"; then
printf "\n== %s ==\n" "$1"
rg -n -C 10 "CommandArgs|Subcommands|fn check|MissingRequired|InvalidChoice|VarTooFew|VarTooMany|MissingSubcommand" "$1"
fi
' sh {}
printf '%s\n' '== read-only source invariant verifier =='
python3 - <<'PY'
from pathlib import Path
import re
lib = Path("argv/src/lib.rs").read_text()
spec = Path("argv/src/spec.rs").read_text()
derive = "\n".join(p.read_text() for p in Path("derive/src").rglob("*.rs"))
error = re.search(r"pub enum Error<'t, 'v>\s*\{(.*?)\n\}", lib, re.S)
assert error, "public Error enum not found"
body = error.group(1)
print("Error enum is public:", True)
print("Error enum is non_exhaustive:", bool(re.search(r"#\[non_exhaustive\]", lib[:error.start()])))
for variant in ("MissingRequired", "InvalidChoice", "VarTooFew", "VarTooMany", "MissingSubcommand"):
print(f"Error::{variant} present:", bool(re.search(rf"\b{variant}\b", body)))
for trait_name, method in (("CommandArgs", "check"), ("Subcommands", "check")):
m = re.search(rf"pub trait {trait_name}\s*:\s*Sized\s*\{{(.*?)\n\}}", spec, re.S)
assert m, f"{trait_name} trait not found"
trait_body = m.group(1)
method_match = re.search(rf"\bfn {method}\b[^;{{]*(;|\{{)", trait_body)
assert method_match, f"{trait_name}::{method} not found"
print(f"{trait_name}::{method} has default body:",
method_match.group(1) == "{")
print("derive source contains CommandArgs implementation:", "CommandArgs" in derive)
print("derive source contains Subcommands implementation:", "Subcommands" in derive)
PYRepository: jdx/usage
Length of output: 28079
Preserve 5.x compatibility or release these APIs as 6.0.0.
- New
Errorvariants break exhaustive downstream matches. - Required
CommandArgs::checkandSubcommands::checkmethods break manual implementations. Add defaultOk(())bodies if 5.x compatibility is required. - Generated implementations already provide both checks.
📍 Affects 2 files
argv/src/lib.rs#L288-L319(this comment)argv/src/spec.rs#L775-L781argv/src/spec.rs#L804-L808
🤖 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/lib.rs` around lines 288 - 319, Preserve 5.x compatibility across
argv/src/lib.rs:288-319, argv/src/spec.rs:775-781, and argv/src/spec.rs:804-808,
or explicitly release these changes as 6.0.0. For compatibility, avoid adding
breaking Error variants such as MissingRequired, InvalidChoice, VarTooFew,
VarTooMany, and MissingSubcommand, and give CommandArgs::check and
Subcommands::check default Ok(()) implementations so existing manual
implementations remain valid; generated implementations should continue
providing both checks.
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.
|
Four review findings. A counting field with `env` discarded the value and marked the field filled anyway. It now reads a number — `EX_VERBOSE=3` is how you say `-vvv`, since the environment cannot repeat a flag — and something unparseable leaves the field alone rather than counting as given. A required *flag* said so at run time but not in the spec, so docs and completions described a different CLI from the one that runs. The same rule an argument already followed. Two about not breaking things. `Error` is `non_exhaustive`, because an error enum grows and recognizing a new failure should never be a breaking change — which is the last time adding a variant breaks anyone. Both `check` methods now default to `Ok(())`, so a hand-written implementation with nothing to check is not forced to say so. And the environment tests move to their own test binary. Environment variables are process-wide, so setting one races every test in the same binary that reads it — including one that only reads it implicitly, by parsing a CLI with an `env` field. Consolidating the cases into a single `#[test]` fixed the race between those cases and not the race with their neighbours; a separate file is a separate process, which does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4dcf923 to
5bc52c7
Compare
|
Four findings fixed, and two of them are about not breaking people. A counting field with A required flag said so at run time but not in the spec, so docs and completions described a different CLI from the one that runs. Same rule an argument already followed. That is the second time this PR has caught a declaration enforced in one place and missing from the other, which is why the test asserting "the declarations reach the spec" earns its keep. On semver, you were right to raise it. The environment test race is the most interesting one. My comment claimed consolidating the cases into one The fix is a separate test binary, since each integration test file is its own process. The file says so at the top, because the next person will be tempted to merge it back. AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
1 participant
Second of the stack (#816 merged while this was in flight, so it targets
main). The parser binds tokens and stops there, which left required-ness,choices, variadic bounds, andenvfallback unimplemented — the 18 corpus vectors the harness reports as post-binding, and the reason asubcommandfield had to beOption<T>.Order matters, so it is stated
Stringhas nowhere to put "absent". After the environment, so a flag withenvset is not reported missing.choicesand bounds, which judge a value however it arrived, including from the environment or a default.Only the command that actually ran is judged. A flag that
installrequires says nothing about an invocation ofrun— which is why the check hangs off theCommandArgstrait and the enum forwards it to the selected variant only. A baresubcommandfield now means one is required and says so when absent, removing #816's stated limit.Two decisions worth arguing with
A field remembers whether a token supplied it, rather than inferring that from an empty value.
--out=is a value somebody typed, and treating""as unset would make it unsayable — the same distinction #810 fixed in the parser, carried through to the checks.Bounds constrain the values a field was given. An unused optional flag is absent, not a violation. Otherwise
var_min = 1becomes a second spelling of required-ness, and there is no way left to say "at least two, if you use it at all". My own test caught this: I putvar_min = 1on an optional flag and every unrelated test in the file started failing withVarTooFew.Contradictions are compile errors
Consistent with the rest of this crate: the derive's job is refusing what cannot round-trip or cannot mean anything.
The declarations reach the spec
A test pins that, because a choice list the parser enforces but the spec omits would be invisible to docs and completions — exactly the kind of half-implemented feature this project keeps turning up.
10 tests: each check, the empty-value case, per-command judging, and a required subcommand.
Still missing, and now stated in the crate docs
overridesandrequired_unless(both need the order flags arrived in, which nothing records yet), nesting past one level, and typed values.AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Medium Risk
Touches public
Errorand trait APIs and adds substantial generated validation/env logic that changes how CLIs accept or reject input. Well-tested, but incorrect checks would mis-parse real command lines.Overview
Implements the post-binding layer the parser deliberately left out: after tokens are bound, generated code now checks required-ness,
choices,var_min/var_max, and fills fromenvwhen argv left a field unset.Validation order is fixed: env fallback → required checks → choices/bounds. Only the selected subcommand is judged. A bare
subcommand: Tfield is now allowed and reportsMissingSubcommandwhen absent (previously had to beOption<T>).Extends
ErrorwithMissingRequired,InvalidChoice,VarTooFew,VarTooMany, andMissingSubcommand, and marks itnon_exhaustive. AddschecktoCommandArgs/Subcommands. Declarations also flow into the emitted spec so docs and completions see the same constraints.Contradictions (
choiceson bool,var_min > var_max, default outside choices) are compile errors. New conformance tests cover each check, empty values counting as given, and env precedence.Reviewed by Cursor Bugbot for commit 5bc52c7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation