From 2ff6307581d653a99c61838fa97c01256a59ad98 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:25:29 +0000 Subject: [PATCH] fix(argv): inherit `unknown_flags`, which reached one command out of a tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- PLAN.md | 10 ++ argv/src/lib.rs | 45 ++++-- argv/src/spec.rs | 63 +++++--- conformance/src/argv.rs | 24 +-- conformance/tests/unknown_flags_inherit.rs | 173 +++++++++++++++++++++ derive/src/codegen.rs | 30 +++- 6 files changed, 298 insertions(+), 47 deletions(-) create mode 100644 conformance/tests/unknown_flags_inherit.rs diff --git a/PLAN.md b/PLAN.md index 433164961..326d6470b 100644 --- a/PLAN.md +++ b/PLAN.md @@ -268,6 +268,16 @@ tasks --usage"`, so task names are meant to come from running that. usage-argv d flatten leaves behind while the group's `build` still demands one — and is a compile error now, asserted during const evaluation in the parent's expansion, where the group is only a type. +- [x] **`unknown_flags`, which reached one command out of a tree** — usage-lib resolves it by + walking outward from the command that ran, so a root declaring `error` makes the whole + CLI strict. usage-argv held the effective value per command instead, on the theory that + whoever built the tables would resolve it — which a derive cannot, since it expands one + struct at a time and cannot see the command above. So the attribute reached the root + alone, and on an `Args` it parsed and was then ignored: a declaration that compiled and + did nothing. Now `None` means inherit, the parser carries the effective value down as it + descends, and the corpus's own table builder stops resolving it — one implementation of + the rule instead of two, and it was the second one that hid the parser not having it. + Costs **160 instructions per parse, 72,272 against 72,112** at mise's scale. - [ ] **`subcommand_required` on the root command** — the same restriction as the root mount, and found beside it: the spec accepts the property only inside a `cmd` block, so a CLI whose _root_ cannot be run alone has no way to say so. The clap bridge could diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 9cd31c1ec..ef1549e35 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -153,9 +153,19 @@ pub struct Command<'a> { /// Resolve it with [`find_subcommand`], which turns a name that no subcommand answers to /// into a compile error. pub default_subcommand: ::core::option::Option<&'a Command<'a>>, - /// What an unrecognized flag-like token means here. Already resolved — see - /// [`UnknownFlags`]. - pub unknown_flags: UnknownFlags, + /// What an unrecognized flag-like token means here, or `None` to keep whatever the + /// enclosing command said. See [`UnknownFlags`]. + /// + /// Inherited rather than resolved per command, which is what usage-lib does — its + /// `effective_unknown_flags` walks outward from the command that ran and falls back to + /// the spec's. Resolving it in the tables instead was possible only for a builder that + /// can see the whole tree: a derive expands one struct at a time and cannot see its + /// parent, so `#[usage(unknown_flags = "error")]` on the root reached the root alone and + /// a subcommand had no way to say it at all. + /// + /// The parser carries the effective value down as it descends, so a command that states + /// nothing costs nothing. + pub unknown_flags: ::core::option::Option, /// Whether this command answers to `--version` and `-V`. /// /// Set on the root, and only when the CLI declares a version: clap adds the flag exactly @@ -183,7 +193,7 @@ impl Command<'_> { args: &[], subcommands: &[], default_subcommand: ::core::option::Option::None, - unknown_flags: UnknownFlags::Value, + unknown_flags: ::core::option::Option::None, version: false, key: 0, }; @@ -817,6 +827,12 @@ pub struct Parser<'t, 'v> { pos: usize, /// The command currently in scope. cmd: &'t Command<'t>, + /// What an unrecognized flag-like token means in the command currently in scope. + /// + /// Carried rather than looked up, because it is inherited: a command that states + /// nothing keeps what the enclosing one said, and walking back up the ancestors on + /// every unrecognized token would pay for the inheritance at the wrong moment. + unknown_flags: UnknownFlags, /// The chain above `cmd`, used to find inherited global flags. Fixed size so /// that nothing is allocated. ancestors: [Option<&'t Command<'t>>; MAX_DEPTH], @@ -878,6 +894,11 @@ impl<'t, 'v> Parser<'t, 'v> { argv, pos: 0, cmd: root, + unknown_flags: match root.unknown_flags { + ::core::option::Option::Some(mode) => mode, + // Nothing above the root to inherit from, so the default stands. + ::core::option::Option::None => UnknownFlags::Value, + }, ancestors: [None; MAX_DEPTH], depth: 0, bundle: &[], @@ -1082,7 +1103,7 @@ impl<'t, 'v> Parser<'t, 'v> { match self.check_bundle(token) { Ok(()) => {} // Unrecognized, so it is a word unless this command wants it refused. - Err(e) if self.cmd.unknown_flags == UnknownFlags::Error => { + Err(e) if self.unknown_flags == UnknownFlags::Error => { return Some(Err(e)); } Err(_) => return Some(self.word(token)), @@ -1149,7 +1170,7 @@ impl<'t, 'v> Parser<'t, 'v> { }); } - if self.cmd.unknown_flags == UnknownFlags::Error { + if self.unknown_flags == UnknownFlags::Error { return Err(Error::UnknownFlag { token }); } // Not a flag here, so it is a word like any other. @@ -1330,6 +1351,10 @@ impl<'t, 'v> Parser<'t, 'v> { self.starts[self.depth] = self.cmd_start; self.depth += 1; self.cmd = sub; + // Only a command that says something changes it, which is what inheriting means. + if let ::core::option::Option::Some(mode) = sub.unknown_flags { + self.unknown_flags = mode; + } // Where this command's own words start, which is what lets a completion hand a callback // the half-parsed struct of the command it was declared on rather than of the root. self.cmd_start = self.pos; @@ -1523,14 +1548,12 @@ mod tests { key: 100, ..Command::EMPTY }; - /// Same shape as ROOT, but a CLI that owns all of its flags. The subcommand - /// carries the setting too: the tables hold it already resolved, because - /// inheritance is the table builder's job rather than the parser's. + /// Same shape as ROOT, but a CLI that owns all of its flags. The subcommand says + /// nothing and inherits it, which is the point: only the root declares the mode. static STRICT_INSTALL: Command = Command { name: "install", aliases: &["i"], flags: &[&FORCE], - unknown_flags: UnknownFlags::Error, key: 100, ..Command::EMPTY }; @@ -1539,7 +1562,7 @@ mod tests { flags: &[&FORCE, &JOBS, &COLOR, &VERBOSE], args: &[&FILE, &REST], subcommands: &[&STRICT_INSTALL], - unknown_flags: UnknownFlags::Error, + unknown_flags: Some(UnknownFlags::Error), ..Command::EMPTY }; static ROOT: Command = Command { diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 93ac3bced..5d6ad7fa6 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -642,7 +642,7 @@ impl Spec<'_> { } // Written only when it is not the default, so an ordinary spec stays quiet // about it. - if self.root.cmd.unknown_flags == UnknownFlags::Error { + if self.root.cmd.unknown_flags == Some(UnknownFlags::Error) { prop(out, "unknown_flags", "error")?; } if let Some(default_subcommand) = self.default_subcommand { @@ -695,7 +695,14 @@ impl Spec<'_> { for example in self.root.examples { write_example(out, example, 0)?; } - write_body(out, self.root, 0, self.bin.unwrap_or(self.name)) + // Nothing above the root, so what it does not state is the default. + write_body( + out, + self.root, + 0, + UnknownFlags::Value, + self.bin.unwrap_or(self.name), + ) } } @@ -707,9 +714,12 @@ fn write_body( out: &mut String, meta: &CommandMeta<'_>, depth: usize, + inherited_unknown_flags: UnknownFlags, bin: &str, ) -> core::fmt::Result { - let enclosing_unknown_flags = meta.cmd.unknown_flags; + // The effective setting for everything inside, which is this command's if it stated one + // and otherwise whatever it inherited. + let enclosing_unknown_flags = meta.cmd.unknown_flags.unwrap_or(inherited_unknown_flags); // Indexing by metadata position below cannot see a table entry with no // metadata, which would be silently unwritten. Check the lengths first. debug_assert_eq!( @@ -775,15 +785,16 @@ fn write_command( if let Some(effect) = meta.effect { write!(out, " effect={}", quoted(effect.as_str()))?; } - // Written only where it changes, since the spec inherits it. The tables hold the - // effective value per command, so repeating the enclosing command's answer would - // say nothing — but a command that differs has to say so, or the setting is lost - // on the way out. - if meta.cmd.unknown_flags != inherited_unknown_flags { + // Written only where it changes, since the spec inherits it as the tables do: a command + // that states nothing has nothing to write, and one that restates what it inherited would + // be saying the same thing twice. A command that differs has to say so, or the setting is + // lost on the way out. + let effective_unknown_flags = meta.cmd.unknown_flags.unwrap_or(inherited_unknown_flags); + if effective_unknown_flags != inherited_unknown_flags { write!( out, " unknown_flags={}", - quoted(match meta.cmd.unknown_flags { + quoted(match effective_unknown_flags { UnknownFlags::Value => "value", UnknownFlags::Error => "error", }) @@ -832,7 +843,7 @@ fn write_command( for example in meta.examples { write_example(out, example, inner)?; } - write_body(out, meta, inner, bin)?; + write_body(out, meta, inner, effective_unknown_flags, bin)?; indent(out, depth)?; out.push_str("}\n"); @@ -1511,41 +1522,49 @@ mod tests { #[test] fn a_subcommand_writes_unknown_flags_only_where_it_differs() { - // The tables hold the effective value per command, so repeating the enclosing - // command's answer says nothing — but a command that differs has to say so, or - // the setting never reaches the spec. + // A command that restates what it inherited says nothing — and one that says + // nothing at all has nothing to write either — but a command that differs has to + // say so, or the setting never reaches the spec. static STRICT_SUB: Command = Command { name: "build", - unknown_flags: UnknownFlags::Error, + unknown_flags: Some(UnknownFlags::Error), + ..Command::EMPTY + }; + static SILENT_SUB: Command = Command { + name: "test", ..Command::EMPTY }; static LENIENT_SUB: Command = Command { name: "exec", - unknown_flags: UnknownFlags::Value, + unknown_flags: Some(UnknownFlags::Value), ..Command::EMPTY }; static ROOT: Command = Command { name: "ex", - subcommands: &[&STRICT_SUB, &LENIENT_SUB], - unknown_flags: UnknownFlags::Error, + subcommands: &[&STRICT_SUB, &SILENT_SUB, &LENIENT_SUB], + unknown_flags: Some(UnknownFlags::Error), ..Command::EMPTY }; static STRICT_META: CommandMeta = CommandMeta { cmd: &STRICT_SUB, ..CommandMeta::EMPTY }; + static SILENT_META: CommandMeta = CommandMeta { + cmd: &SILENT_SUB, + ..CommandMeta::EMPTY + }; static LENIENT_META: CommandMeta = CommandMeta { cmd: &LENIENT_SUB, ..CommandMeta::EMPTY }; static ROOT_META: CommandMeta = CommandMeta { cmd: &ROOT, - subcommands: &[&STRICT_META, &LENIENT_META], + subcommands: &[&STRICT_META, &SILENT_META, &LENIENT_META], ..CommandMeta::EMPTY }; let mut out = String::new(); - write_body(&mut out, &ROOT_META, 0, "ex").unwrap(); + write_body(&mut out, &ROOT_META, 0, UnknownFlags::Value, "ex").unwrap(); // Counted rather than checked with `contains`, which is how a duplicated // write survived review: `unknown_flags="value" unknown_flags="value"` contains @@ -1565,6 +1584,12 @@ mod tests { "a subcommand matching the enclosing command should not repeat it: {build}" ); + let test = line("test"); + assert!( + !test.contains("unknown_flags"), + "a subcommand that declares nothing inherits, and writes nothing: {test}" + ); + let exec = line("exec"); assert_eq!( exec.matches(r#"unknown_flags="value""#).count(), diff --git a/conformance/src/argv.rs b/conformance/src/argv.rs index 3dacde5f7..5c03dd20f 100644 --- a/conformance/src/argv.rs +++ b/conformance/src/argv.rs @@ -60,12 +60,11 @@ pub fn run(vector: &Vector) -> Outcome { return Outcome::OutOfScope(reason); } - // Inheritance is resolved here, not in the parser: usage-argv's tables hold the - // effective value per command, which is what a derive would emit. - let root = build( - &spec.cmd, - convert_unknown_flags(spec.unknown_flags.unwrap_or_default()), - ); + // The spec's own setting belongs to the root command, which is where usage-argv's tables + // hold it. Everything below inherits it, which the parser now does itself rather than + // this flattening it on the way in — a second implementation of the same rule, and the + // one that hid the parser not having it. + let root = build(&spec.cmd, spec.unknown_flags.map(convert_unknown_flags)); // `default_subcommand` is a property of the spec rather than of a command, so it is // resolved once, here, against the root's own subcommands. A name that answers to // nothing is left as None: the spec is what it is, and a vector that expects routing @@ -225,16 +224,17 @@ fn out_of_scope(vector: &Vector) -> Option<&'static str> { /// Build leaked tables mirroring a spec command. /// -/// `inherited_unknown_flags` is the effective setting from above, which a command -/// that states nothing keeps and passes down. +/// `unknown_flags` is carried through as the spec states it — `None` where a command says +/// nothing — because the parser inherits it. The root takes the spec-level setting, since +/// that is the command a spec's own property describes. fn build( cmd: &SpecCommand, - inherited_unknown_flags: ArgvUnknownFlags, + root_unknown_flags: Option, ) -> &'static Command<'static> { let unknown_flags = cmd .unknown_flags .map(convert_unknown_flags) - .unwrap_or(inherited_unknown_flags); + .or(root_unknown_flags); let flags: Vec<&'static Flag<'static>> = cmd .flags .iter() @@ -295,7 +295,9 @@ fn build( let subcommands: Vec<&'static Command<'static>> = cmd .subcommands .values() - .map(|sub| build(sub, unknown_flags)) + // A subcommand states its own or says nothing; there is no spec-level setting to + // hand it, since the root has already taken that. + .map(|sub| build(sub, None)) .collect(); let aliases: Vec<&'static str> = cmd diff --git a/conformance/tests/unknown_flags_inherit.rs b/conformance/tests/unknown_flags_inherit.rs new file mode 100644 index 000000000..32ec9cc6e --- /dev/null +++ b/conformance/tests/unknown_flags_inherit.rs @@ -0,0 +1,173 @@ +//! What an unrecognized flag means, and how far a command's answer reaches. +//! +//! 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 could +//! 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 accepted and then ignored — a declaration that compiled and did nothing. +//! +//! Found by converting usage-cli itself to the derive, where every command wants the strict +//! reading and the five that forward a command line to somebody else's script want the +//! lenient one. + +use std::ffi::OsStr; + +use usage::Spec as LibSpec; +use usage_argv::Error; +use usage_derive::{Args, Cli, Subcommands}; + +/// A command that says nothing, and so is as strict as the root. +#[derive(Args)] +struct Build { + /// Say more + #[usage(long)] + verbose: bool, + /// What to build + target: Option, +} + +/// A command that forwards what it does not recognise, as `usage bash` does. +#[derive(Args)] +#[usage(unknown_flags = "value")] +struct Exec { + /// The script and its own options + args: Vec, +} + +/// A command inside a command, to check the answer reaches past one level. +#[derive(Args)] +struct Inner { + /// Say more + #[usage(long)] + verbose: bool, +} + +#[derive(Subcommands)] +enum Nested { + /// Two levels down, and still strict + Inner(Box), +} + +#[derive(Args)] +struct Outer { + #[usage(subcommand)] + command: Option, +} + +#[derive(Subcommands)] +enum Commands { + /// Build something + Build(Box), + /// Run something else + Exec(Box), + /// A group + Outer(Box), +} + +/// A CLI that owns all of its flags. +#[derive(Cli)] +#[usage(bin = "ex", unknown_flags = "error")] +struct Ex { + #[usage(subcommand)] + command: Option, +} + +fn argv(tokens: [&str; N]) -> [&OsStr; N] { + tokens.map(OsStr::new) +} + +#[test] +fn a_subcommand_inherits_the_roots_answer() { + // The bug this is here for: `--nope` used to be offered to `build`'s positional, so the + // *target* became `--nope` and the real target was the unexpected word — where clap, and + // the root of this very CLI, name the flag. + let a = argv(["build", "--nope"]); + let Err(err) = Ex::parse_from(&a) else { + panic!("the root said unknown flags are errors") + }; + assert!( + matches!(err, Error::UnknownFlag { .. }), + "the flag is named, not swallowed by a positional: {err:?}" + ); + + // And the flag it *does* declare still binds, so this is strictness rather than a + // command that stopped working. + let a = argv(["build", "--verbose", "release"]); + let Some(Commands::Build(build)) = Ex::parse_from(&a).expect("should parse").command else { + panic!("expected `build`") + }; + assert!(build.verbose); + assert_eq!(build.target.as_deref(), Some("release")); +} + +#[test] +fn it_reaches_past_one_level() { + let a = argv(["outer", "inner", "--nope"]); + let Err(err) = Ex::parse_from(&a) else { + panic!("inherited two levels down") + }; + assert!(matches!(err, Error::UnknownFlag { .. }), "{err:?}"); + + let a = argv(["outer", "inner", "--verbose"]); + let Some(Commands::Outer(outer)) = Ex::parse_from(&a).expect("should parse").command else { + panic!("expected `outer`") + }; + let Some(Nested::Inner(inner)) = outer.command else { + panic!("expected `inner`") + }; + assert!(inner.verbose); +} + +#[test] +fn a_command_that_declares_its_own_keeps_it() { + // And this is why inheritance is not enough on its own: a command that forwards a command + // line to somebody else's program has to be able to say so. + let a = argv(["exec", "./script.sh", "--its-own-flag"]); + let ex = Ex::parse_from(&a).expect("a forwarding command takes it as a value"); + let Some(Commands::Exec(exec)) = ex.command else { + panic!("expected `exec`") + }; + assert_eq!(exec.args, ["./script.sh", "--its-own-flag"]); +} + +#[test] +fn a_declaration_that_does_nothing_is_not_a_declaration() { + // The emitted spec says the same thing the parser does, which is the whole claim: `exec` + // differs from what it inherited and says so, and `build` inherits and stays quiet. + let kdl = Ex::to_kdl(); + let spec: LibSpec = kdl.parse().expect("valid spec"); + assert_eq!( + spec.unknown_flags, + Some(usage::UnknownFlags::Error), + "the root's own setting: {kdl}" + ); + let cmd = |name: &str| spec.cmd.subcommands.get(name).expect("declared"); + assert_eq!(cmd("exec").unknown_flags, Some(usage::UnknownFlags::Value)); + assert_eq!( + cmd("build").unknown_flags, + None, + "a command that says nothing writes nothing, and inherits when read back: {kdl}" + ); +} + +#[test] +fn the_reference_reads_it_the_same_way() { + // The oracle, on the spec this CLI emits: usage-lib walks outward from the command that + // ran, so `build` is strict and `exec` is not. A disagreement here is the divergence this + // change is about, in the direction that would matter. + let spec: LibSpec = Ex::to_kdl().parse().expect("valid spec"); + + let strict = usage::parse::parse(&spec, &words(["ex", "build", "--nope"])); + assert!(strict.is_err(), "usage-lib inherits the root's `error` too"); + + let forwarding = usage::parse::parse(&spec, &words(["ex", "exec", "--its-own-flag"])); + assert!( + forwarding.is_ok(), + "and honours a command's own answer: {forwarding:?}" + ); +} + +fn words(tokens: [&str; N]) -> Vec { + tokens.iter().map(|t| t.to_string()).collect() +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 6fd2be312..509f3ecf4 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -31,12 +31,11 @@ pub fn emit(cli: &Cli) -> TokenStream { .filter(|f| matches!(f.kind, Kind::Arg { .. })) .collect(); - // Resolved here rather than at parse time: the tables hold the effective value, - // and with one command per struct there is nothing above it to inherit from yet. - let unknown_flags = match cli.unknown_flags.as_deref() { - Some("error") => quote!(::usage_argv::UnknownFlags::Error), - _ => quote!(::usage_argv::UnknownFlags::Value), - }; + // Left unset when the struct says nothing, which is what lets the parser inherit it: one + // command per struct, and a macro expansion cannot see the command above it. An earlier + // version resolved it here and wrote `Value` for every silent command, which made the + // root's declaration reach the root alone. + let unknown_flags = unknown_flags_tokens(cli); let default_subcommand = option_str(cli.default_subcommand.as_deref()); let restart_token = option_str(cli.restart_token.as_deref()); @@ -488,6 +487,23 @@ fn flatten_checks(cli: &Cli) -> TokenStream { quote!(#(#checks)*) } +/// A command's `unknown_flags`, as the table's `Option`. +/// +/// `None` is not a default so much as a deferral: the parser carries the enclosing command's +/// answer down, so a struct that declares nothing keeps whatever it was given. Only a struct +/// that says something writes anything. +fn unknown_flags_tokens(cli: &Cli) -> TokenStream { + match cli.unknown_flags.as_deref() { + Some("error") => quote!(::core::option::Option::Some( + ::usage_argv::UnknownFlags::Error + )), + Some(_) => quote!(::core::option::Option::Some( + ::usage_argv::UnknownFlags::Value + )), + None => quote!(::core::option::Option::None), + } +} + /// The completion entry points, for a CLI that asked for them. /// /// Two pieces: a function that answers a request, and the line in `parse` that notices one. Both @@ -2125,6 +2141,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { } ) }); + let unknown_flags = unknown_flags_tokens(cli); let before_help = option_str(cli.before_help.as_deref()); let before_long_help = option_str(cli.before_long_help.as_deref()); let after_help = option_str(cli.after_help.as_deref()); @@ -2211,6 +2228,7 @@ pub fn emit_args(cli: &Cli) -> TokenStream { pub static COMMAND: ::usage_argv::Command = ::usage_argv::Command { name: #name, key: #command_key, + unknown_flags: #unknown_flags, flags: #flag_table_ref, args: #arg_table_ref, #sub_commands