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