From f942f04ef388192249ac32354ce38152201575c4 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:08:41 +0000 Subject: [PATCH 1/5] docs(spec): document when a root mount runs, and overrides_default The mount page describes only the behavior of the first commit in #806 and never picked up what the later ones changed: it still claims flags never trigger discovery, which is untrue of completions and help, and `overrides_default` shipped with no documentation at all. My fault, and worth writing down how: the edits were made by string replacement against text prettier had already rewrapped and turned `*own*` into `_own_`, so both replacements silently matched nothing. I said in the pull request that the page had been corrected, and it had not. Diff verified this time. Co-Authored-By: Claude Fable 5 --- docs/spec/reference/cmd.md | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/docs/spec/reference/cmd.md b/docs/spec/reference/cmd.md index 43e932ed..0714062a 100644 --- a/docs/spec/reference/cmd.md +++ b/docs/spec/reference/cmd.md @@ -95,10 +95,31 @@ cmd "install" mount run="mycli plugin-commands" ``` -The root's mount runs only when a _word_ matches nothing already declared, so -`mycli install` costs nothing extra and only `mycli something-from-a-plugin` pays -for discovery. Flags never trigger it — `mycli --help` does not run your mount -command — so declaring the commands you know about keeps the common path free. +Resolving a mount runs a process, so when the root's mount runs depends on what is +asking for it: + +| asking | when it runs | +| ------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | +| a completion, or rendering help | up front — both need the whole command list, and `mycli ` has no word to go on | +| a parse | only when a word matches nothing already declared, so `mycli install` costs nothing extra and only `mycli something-from-a-plugin` pays for discovery | +| a parse, for a flag | never | + +Declaring the commands you know about therefore keeps ordinary invocations free, +while completions still see everything. + +A `default_subcommand` outranks discovery, because it already says what an unmatched +word means and costs nothing to consult. Without that, a CLI that routes unknown +words to a task runner would spawn its discovery process once per task. A mount can +ask to win anyway: + +```kdl +mount run="mycli plugin-commands" overrides_default=#true +``` + +That setting applies to completions as much as to parses, and deliberately: a +completion offering a command that running would hand to the default subcommand +instead is worse than not offering it. So a root mount under a `default_subcommand` +contributes nothing anywhere until it asks to outrank it. ### Global flags and mounted commands From a44726ae6ecd3c60662631dfc63ab90269b11e0e Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:45:00 +0000 Subject: [PATCH 2/5] feat(spec): make unknown flags configurable, and keep them as values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flag-like token that names no flag stays what it has always been here: a word, offered to the positionals like any other. That is deliberate, and now documented as a decision rather than left as an accident — the grammar said one thing and the parser did another, which was the only indefensible state. Every comparable parser refuses the token, and they are right for what they do: parse their own argv, where a dash-word is a flag or a typo. A usage spec also parses command lines whose flags it does not own — a script forwarding options through `usage exec`, a task whose script is the authority on what it accepts, a completion asked about half-typed input. In those, an unrecognized token is data in transit far more often than a mistake, and refusing it breaks the wrapper for anyone who did not enumerate the flags of the program behind it. The cost is a misspelled `--hekp` becoming an argument, so a CLI that owns all of its flags can say `unknown_flags "error"` and get typo detection. It is settable per CLI and per command, and inherited — a CLI can be strict everywhere except the one command that forwards. `#[usage(unknown_flags = "error")]` says it from a Rust struct, which is the case that usually wants it. Even when refusing, a lone `-` and a negative number stay values. oclif made exactly this mistake on the same switch and had to add the number back. Six corpus vectors move from expecting an error to expecting a value, and five new ones cover the strict mode, the inherited override, and the negative-number carve-out. Co-Authored-By: Claude Fable 5 --- argv/src/lib.rs | 131 ++++++++++++++++++++++++++-- argv/src/spec.rs | 6 ++ conformance/src/argv.rs | 38 +++++++- conformance/tests/derive.rs | 36 +++++++- corpus/01-long-flags.json | 28 +++--- corpus/02-short-flags.json | 21 ++--- corpus/04-subcommands.json | 16 ++-- corpus/05-globals.json | 7 +- derive/src/codegen.rs | 8 ++ derive/src/model.rs | 25 +++++- docs/cli/reference/commands.json | 21 ++++- docs/spec/argv.md | 53 +++++++++++- docs/spec/reference/cmd.md | 16 ++++ lib/src/docs/models.rs | 2 + lib/src/lib.rs | 1 + lib/src/parse.rs | 143 +++++++++++++++++++++++++++++++ lib/src/spec/cmd.rs | 31 +++++++ lib/src/spec/mod.rs | 21 +++++ lib/src/spec/unknown_flags.rs | 49 +++++++++++ 19 files changed, 600 insertions(+), 53 deletions(-) create mode 100644 lib/src/spec/unknown_flags.rs diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 422eb81c..23d2611a 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -101,6 +101,9 @@ pub struct Command<'a> { /// Positional arguments, in the order they are filled. pub args: &'a [&'a Arg<'a>], pub subcommands: &'a [&'a Command<'a>], + /// What an unrecognized flag-like token means here. Already resolved — see + /// [`UnknownFlags`]. + pub unknown_flags: UnknownFlags, /// Caller-assigned identifier, echoed back in [`Event::Command`]. pub key: u32, } @@ -113,6 +116,7 @@ impl Command<'_> { flags: &[], args: &[], subcommands: &[], + unknown_flags: UnknownFlags::Value, key: 0, }; } @@ -198,6 +202,25 @@ impl Arg<'_> { }; } +/// What to do with a flag-like token that names no flag in scope. +/// +/// The default is [`UnknownFlags::Value`]: the token carries on to the positional +/// arguments, because a spec is often parsing a command line whose flags belong to +/// something else — a wrapped tool, a task script. A CLI that owns all of its +/// flags declares [`UnknownFlags::Error`] and gets typo detection instead. +/// +/// Stored per command and already resolved: inheritance is a question for whoever +/// builds the tables, and answering it at compile time keeps it out of the parse. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum UnknownFlags { + /// Offer the token to the positionals. If none can take it, it is an + /// unexpected argument. + #[default] + Value, + /// Reject the token. + Error, +} + /// How an argument relates to the `--` separator. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum DoubleDash { @@ -422,8 +445,13 @@ impl<'t, 'v> Parser<'t, 'v> { // out one at a time, so discovering an unknown letter half way // through would mean the earlier letters had already been applied — // and the grammar rejects the entire token, not the tail of it. - if let Err(e) = self.check_bundle(token) { - return Some(Err(e)); + 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 => { + return Some(Err(e)); + } + Err(_) => return Some(self.word(token)), } self.bundle = &token[1..]; self.bundle_token = token; @@ -467,7 +495,11 @@ impl<'t, 'v> Parser<'t, 'v> { }); } - Err(Error::UnknownFlag { token }) + if self.cmd.unknown_flags == UnknownFlags::Error { + return Err(Error::UnknownFlag { token }); + } + // Not a flag here, so it is a word like any other. + self.word(token) } /// Walk a short-flag token without binding anything, to find out whether all @@ -698,6 +730,25 @@ 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. + static STRICT_INSTALL: Command = Command { + name: "install", + aliases: &["i"], + flags: &[&FORCE], + unknown_flags: UnknownFlags::Error, + key: 100, + ..Command::EMPTY + }; + static STRICT: Command = Command { + name: "ex", + flags: &[&FORCE, &JOBS, &COLOR, &VERBOSE], + args: &[&FILE, &REST], + subcommands: &[&STRICT_INSTALL], + unknown_flags: UnknownFlags::Error, + ..Command::EMPTY + }; static ROOT: Command = Command { name: "ex", flags: &[&FORCE, &JOBS, &COLOR, &VERBOSE], @@ -790,13 +841,57 @@ mod tests { #[test] fn no_abbreviation() { + // A prefix names no flag, so by default it is a value like any other word. let a = argv(["--forc"]); + assert_eq!( + parse(&ROOT, &a).unwrap(), + vec![Event::Arg { + arg: &FILE, + value: b"--forc" + }] + ); + + // And a CLI that owns its flags hears about it, which is the whole reason + // the strict mode exists. assert!(matches!( - parse(&ROOT, &a), + parse(&STRICT, &a), Err(Error::UnknownFlag { token: b"--forc" }) )); } + #[test] + fn an_unknown_flag_is_a_value_by_default() { + // The default, and the case it is for: a command line being forwarded to + // something whose flags this spec does not know. + let a = argv(["--wat", "keep"]); + assert_eq!( + parse(&ROOT, &a).unwrap(), + vec![ + Event::Arg { + arg: &FILE, + value: b"--wat" + }, + Event::Arg { + arg: &REST, + value: b"keep" + }, + ] + ); + + // With nowhere to put it, it is an unexpected argument — the same error an + // extra word gets, rather than a special one about flags. + static ONE: Command = Command { + name: "ex", + args: &[&FILE], + ..Command::EMPTY + }; + let a = argv(["a", "--wat"]); + assert_eq!( + parse(&ONE, &a), + Err(Error::UnexpectedArg { token: b"--wat" }) + ); + } + #[test] fn negation() { let a = argv(["--no-color"]); @@ -920,9 +1015,16 @@ mod tests { ] ); - // `--jobs` belongs to the root and is not global. + // `--jobs` belongs to the root and is not global, so it is not a flag here. + // Strictly that is an unknown flag; leniently it is a word, and `install` + // declares no argument to hold one — either way it is never read as the + // root's flag, which is what this test is about. let a = argv(["install", "--jobs", "8"]); - assert!(matches!(parse(&ROOT, &a), Err(Error::UnknownFlag { .. }))); + assert!(matches!(parse(&STRICT, &a), Err(Error::UnknownFlag { .. }))); + assert!(matches!( + parse(&ROOT, &a), + Err(Error::UnexpectedArg { token: b"--jobs" }) + )); } #[test] @@ -1161,13 +1263,24 @@ mod tests { // flag event came out first, a caller would have applied `-f` from a // command line that was rejected. let a = argv(["-fz"]); - let mut parser = Parser::new(&ROOT, &a); + let mut parser = Parser::new(&STRICT, &a); assert_eq!( parser.next_event(), Some(Err(Error::UnknownFlag { token: b"-fz" })), "an unknown letter must reject the token before any of it is applied" ); assert!(parser.next_event().is_none()); + + // Leniently, the same token is a value — and `-f` is *not* applied, since + // the token was never a bundle at all. + let a = argv(["-fz"]); + assert_eq!( + parse(&ROOT, &a).unwrap(), + vec![Event::Arg { + arg: &FILE, + value: b"-fz" + }] + ); } #[test] @@ -1175,7 +1288,7 @@ mod tests { for (tokens, want) in [(["-z"], &b"-z"[..]), (["-fz"], &b"-fz"[..])] { let a = argv(tokens); assert_eq!( - parse(&ROOT, &a), + parse(&STRICT, &a), Err(Error::UnknownFlag { token: want }), "{tokens:?}" ); @@ -1185,7 +1298,7 @@ mod tests { #[test] fn errors_are_terminal() { let a = argv(["--wat", "--force"]); - let mut parser = Parser::new(&ROOT, &a); + let mut parser = Parser::new(&STRICT, &a); assert!(parser.next_event().unwrap().is_err()); assert!(parser.next_event().is_none()); } diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 6a430231..0851f0f2 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -23,6 +23,7 @@ //! use core::fmt::Write as _; +use crate::UnknownFlags; use crate::{Arg, Command, DoubleDash, Flag}; /// A whole CLI: the root command plus what describes the program itself. @@ -241,6 +242,11 @@ impl Spec<'_> { if let Some(long_about) = self.long_about.or(self.root.long_about) { prop(out, "long_about", long_about)?; } + // Written only when it is not the default, so an ordinary spec stays quiet + // about it. + if self.root.cmd.unknown_flags == UnknownFlags::Error { + prop(out, "unknown_flags", "error")?; + } if let Some(default_subcommand) = self.default_subcommand { prop(out, "default_subcommand", default_subcommand)?; } diff --git a/conformance/src/argv.rs b/conformance/src/argv.rs index ce89385b..2c980877 100644 --- a/conformance/src/argv.rs +++ b/conformance/src/argv.rs @@ -21,7 +21,9 @@ use std::collections::BTreeMap; use std::ffi::OsStr; use usage::{Spec, SpecArg, SpecCommand, SpecFlag}; -use usage_argv::{Arg, Command, DoubleDash, Error, Event, Flag, Parser}; +use usage_argv::{ + Arg, Command, DoubleDash, Error, Event, Flag, Parser, UnknownFlags as ArgvUnknownFlags, +}; use crate::{ErrorCode, Expect, Parsed, Value, Vector}; @@ -58,7 +60,12 @@ pub fn run(vector: &Vector) -> Outcome { return Outcome::OutOfScope(reason); } - let root = build(&spec.cmd); + // 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()), + ); let argv: Vec<&'static OsStr> = vector .argv .iter() @@ -142,6 +149,14 @@ fn code(err: Error<'_, '_>) -> ErrorCode { } } +/// The spec's spelling of the setting, in the parser's terms. +fn convert_unknown_flags(mode: usage::UnknownFlags) -> ArgvUnknownFlags { + match mode { + usage::UnknownFlags::Value => ArgvUnknownFlags::Value, + usage::UnknownFlags::Error => ArgvUnknownFlags::Error, + } +} + /// Which flags accumulate rather than replace. enum Multi { Count, @@ -237,7 +252,17 @@ fn arg_post_binding(arg: &SpecArg) -> Option<&'static str> { } /// Build leaked tables mirroring a spec command. -fn build(cmd: &SpecCommand) -> &'static Command<'static> { +/// +/// `inherited_unknown_flags` is the effective setting from above, which a command +/// that states nothing keeps and passes down. +fn build( + cmd: &SpecCommand, + inherited_unknown_flags: ArgvUnknownFlags, +) -> &'static Command<'static> { + let unknown_flags = cmd + .unknown_flags + .map(convert_unknown_flags) + .unwrap_or(inherited_unknown_flags); let flags: Vec<&'static Flag<'static>> = cmd .flags .iter() @@ -280,7 +305,11 @@ fn build(cmd: &SpecCommand) -> &'static Command<'static> { }) .collect(); - let subcommands: Vec<&'static Command<'static>> = cmd.subcommands.values().map(build).collect(); + let subcommands: Vec<&'static Command<'static>> = cmd + .subcommands + .values() + .map(|sub| build(sub, unknown_flags)) + .collect(); let aliases: Vec<&'static str> = cmd .aliases @@ -295,6 +324,7 @@ fn build(cmd: &SpecCommand) -> &'static Command<'static> { flags: Box::leak(flags.into_boxed_slice()), args: Box::leak(args.into_boxed_slice()), subcommands: Box::leak(subcommands.into_boxed_slice()), + unknown_flags, key: 0, })) } diff --git a/conformance/tests/derive.rs b/conformance/tests/derive.rs index 36a46db9..4be88b42 100644 --- a/conformance/tests/derive.rs +++ b/conformance/tests/derive.rs @@ -226,14 +226,46 @@ fn positionals_fill_in_order_and_the_variadic_takes_the_rest() { assert_eq!(ex.rest, ["two", "three"]); } +/// A CLI that owns every flag it accepts, which is the usual case for a Rust +/// binary — as opposed to a script forwarding options to something else. +#[derive(Cli, Debug)] +#[usage(unknown_flags = "error")] +struct Strict { + /// Overwrite + #[usage(long)] + force: bool, + /// The file + file: String, +} + #[test] -fn a_typo_is_reported_rather_than_bound() { +fn a_typo_is_a_value_by_default() { + // The default: an unrecognized flag is data, because a spec is often parsing a + // command line whose flags belong to something else. let a = argv(["--forse", "x.txt"]); - let err = Ex::parse_from(&a).expect_err("an unknown flag should not parse"); + let ex = Ex::parse_from(&a).expect("should parse"); + assert_eq!(ex.file, "--forse"); + assert_eq!(ex.rest, ["x.txt"]); +} + +#[test] +fn a_typo_is_reported_when_the_cli_owns_its_flags() { + let a = argv(["--forse", "x.txt"]); + let err = Strict::parse_from(&a).expect_err("an unknown flag should not parse"); assert!( matches!(err, usage_argv::Error::UnknownFlag { token } if token == b"--forse"), "got {err:?}" ); + + // And the choice reaches the spec, so docs and completions see it too. + let spec: LibSpec = Strict::to_kdl().parse().expect("valid spec"); + assert_eq!(spec.unknown_flags, Some(usage::UnknownFlags::Error)); + + // A flag it does know still works, so strictness has not broken the parse. + let a = argv(["--force", "x.txt"]); + let strict = Strict::parse_from(&a).expect("should parse"); + assert!(strict.force); + assert_eq!(strict.file, "x.txt"); } #[test] diff --git a/corpus/01-long-flags.json b/corpus/01-long-flags.json index fb5af891..9aa49907 100644 --- a/corpus/01-long-flags.json +++ b/corpus/01-long-flags.json @@ -84,23 +84,17 @@ }, { "id": "long-unknown", - "doc": "An unrecognized long flag is an error, even when the command has a positional that could have held it. Otherwise a typo becomes data.", + "doc": "An unrecognized long flag is offered to the positionals like any other word. A spec is often parsing a command line whose flags belong to something else, so a token it has not heard of is more likely data in transit than a mistake.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--force\"\narg \"[file]\"\n", "argv": ["--wat"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib binds `--wat` to the positional `file`. This is the root of several other divergences here: unrecognized flags fall through to positionals, so they surface as `unexpected_arg` \u2014 or as no error at all \u2014 rather than as `unknown_flag`." - } + "expect": { "ok": { "args": { "file": "--wat" } } } }, { "id": "long-no-abbreviation", - "doc": "A unique prefix of a long name is not accepted. Abbreviation inference makes adding a flag a breaking change for anyone who typed a prefix that was unique until now, so the grammar has none.", + "doc": "A unique prefix of a long name is not accepted as the flag. Abbreviation inference makes adding a flag a breaking change for anyone who typed a prefix that was unique until now, so the grammar has none \u2014 the token becomes a word, and here there is no argument to hold it.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--force\"\n", "argv": ["--for"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib also refuses the abbreviation, but reports `unexpected_arg`: `--for` becomes a positional, and the command has none." - } + "expect": { "error": "unexpected_arg" } }, { "id": "long-repeated-var", @@ -141,6 +135,20 @@ "spec": "name \"ex\"\nbin \"ex\"\nflag \"--color\" negate=\"--no-color\" default=#true\n", "argv": [], "expect": { "ok": { "flags": { "color": true } } } + }, + { + "id": "unknown-flags-error-rejects-the-token", + "doc": "A CLI that owns all of its flags declares `unknown_flags \"error\"` and gets typo detection: the token is refused rather than passed along as data.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\nflag \"--force\"\narg \"[file]\"\n", + "argv": ["--wat"], + "expect": { "error": "unknown_flag" } + }, + { + "id": "unknown-flags-error-still-takes-negative-numbers", + "doc": "Even when refusing unknown flags, a negative number is a value. This is the carve-out oclif had to add back after making rejection its default.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[offset]\"\n", + "argv": ["-1"], + "expect": { "ok": { "args": { "offset": "-1" } } } } ] } diff --git a/corpus/02-short-flags.json b/corpus/02-short-flags.json index 72f4ee24..61273b6a 100644 --- a/corpus/02-short-flags.json +++ b/corpus/02-short-flags.json @@ -60,13 +60,10 @@ }, { "id": "short-bundle-unknown-letter", - "doc": "An unrecognized letter anywhere in a bundle fails the whole token.", + "doc": "A token containing an unrecognized letter is not a bundle at all, so none of its letters are applied \u2014 `-az` does not set `-a`. Here it becomes a word, and there is no argument to hold it.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"-a --all\"\n", "argv": ["-az"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib reports `unexpected_arg`: the whole token falls through to positionals, so `-a` is not applied either." - } + "expect": { "error": "unexpected_arg" } }, { "id": "short-count", @@ -91,13 +88,17 @@ }, { "id": "short-unknown", - "doc": "An unrecognized short flag is an error rather than a positional.", + "doc": "An unrecognized short flag is a word too, for the same reason as the long form.", "spec": "name \"ex\"\nbin \"ex\"\narg \"[file]\"\n", "argv": ["-z"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib binds `-z` to the positional `file`, the same fall-through described in `long-unknown`." - } + "expect": { "ok": { "args": { "file": "-z" } } } + }, + { + "id": "unknown-flags-error-rejects-a-whole-bundle", + "doc": "Refusing unknown flags, an unrecognized letter fails the whole token rather than applying the letters before it \u2014 a partly-applied bundle would be worse than a rejected one.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\nflag \"-a --all\"\n", + "argv": ["-az"], + "expect": { "error": "unknown_flag" } } ] } diff --git a/corpus/04-subcommands.json b/corpus/04-subcommands.json index 2fc99c00..07c304fb 100644 --- a/corpus/04-subcommands.json +++ b/corpus/04-subcommands.json @@ -53,7 +53,7 @@ }, { "id": "cmd-name-wins-over-arg", - "doc": "When a word could be either a subcommand or a positional value, the subcommand wins. A CLI that declares both cannot be given a positional whose text equals a subcommand name — mise documents exactly this hazard for tasks that share a name with a command.", + "doc": "When a word could be either a subcommand or a positional value, the subcommand wins. A CLI that declares both cannot be given a positional whose text equals a subcommand name \u2014 mise documents exactly this hazard for tasks that share a name with a command.", "spec": "name \"ex\"\nbin \"ex\"\narg \"[target]\"\ncmd \"install\"\n", "argv": ["install"], "expect": { "ok": { "cmd": ["install"] } } @@ -67,13 +67,17 @@ }, { "id": "cmd-parent-flag-not-inherited", - "doc": "A parent flag that is not `global` is not accepted after descending into a subcommand.", + "doc": "A parent flag that is not `global` is not a flag after descending into a subcommand \u2014 the token becomes a word there, and this subcommand declares no argument to hold one.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"-q --quiet\"\ncmd \"install\"\n", "argv": ["install", "--quiet"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib agrees the flag is not inherited, but reports `unexpected_arg` because the unrecognized flag becomes a positional first." - } + "expect": { "error": "unexpected_arg" } + }, + { + "id": "cmd-may-override-the-cli-wide-unknown-flags", + "doc": "The setting is inherited but overridable, so a CLI can refuse unknown flags everywhere except the one command that forwards a command line to something else.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\ncmd \"exec\" unknown_flags=\"value\" {\n arg \"[rest]...\"\n}\n", + "argv": ["exec", "--wat"], + "expect": { "ok": { "cmd": ["exec"], "args": { "rest": ["--wat"] } } } } ] } diff --git a/corpus/05-globals.json b/corpus/05-globals.json index d8dd0725..ffa54c2b 100644 --- a/corpus/05-globals.json +++ b/corpus/05-globals.json @@ -34,13 +34,10 @@ }, { "id": "global-declared-on-subcommand-not-visible-at-root", - "doc": "`global` propagates downward only. A flag declared global on a subcommand is not accepted before that subcommand is reached.", + "doc": "`global` propagates downward only. A flag declared global on a subcommand is not a flag before that subcommand is reached, so the token is a word \u2014 and the root declares no argument to hold one.", "spec": "name \"ex\"\nbin \"ex\"\ncmd \"install\" {\n flag \"-f --force\" global=#true\n}\n", "argv": ["--force", "install"], - "expect": { "error": "unknown_flag" }, - "reference": { - "diverges": "usage-lib agrees the flag is not visible here, but reports `unexpected_arg` for the same fall-through reason." - } + "expect": { "error": "unexpected_arg" } }, { "id": "global-shadowed-by-subcommand-flag", diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 97ea4163..150dfc44 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -32,6 +32,13 @@ 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), + }; + let flag_tables = flags.iter().enumerate().map(|(i, f)| flag_table(i, f)); let arg_tables = args.iter().enumerate().map(|(i, f)| arg_table(i, f)); let flag_metas = flags.iter().enumerate().map(|(i, f)| flag_meta(i, f)); @@ -77,6 +84,7 @@ pub fn emit(cli: &Cli) -> TokenStream { #(#arg_tables)* pub static ROOT: Command = Command { + unknown_flags: #unknown_flags, name: #name, flags: &[#(#flag_refs),*], args: &[#(#arg_refs),*], diff --git a/derive/src/model.rs b/derive/src/model.rs index 411fffc3..1f60b0f4 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -17,6 +17,9 @@ pub struct Cli { /// From the struct's doc comment: first paragraph, and the whole thing. pub about: Option, pub long_about: Option, + /// Whether a flag-like token that names no flag is a value or an error. Unset + /// means the spec's default, which is `value`. + pub unknown_flags: Option, pub fields: Vec, } @@ -117,6 +120,7 @@ impl Cli { version: None, about, long_about, + unknown_flags: None, fields: Vec::new(), }; @@ -127,13 +131,30 @@ impl Cli { "name" => cli.name = string_value(&meta)?, "bin" => cli.bin = Some(string_value(&meta)?), "version" => cli.version = Some(string_value(&meta)?), + // A Rust CLI usually owns every flag it accepts, which is the + // case the stricter reading is for — but it is still opt-in, + // since a wrapper forwarding options wants the default. + "unknown_flags" => { + let mode = string_value(&meta)?; + if mode != "value" && mode != "error" { + return Err(syn::Error::new_spanned( + &path, + format!( + "`unknown_flags = \"{mode}\"` is not a mode; write \ + \"value\" to pass an unrecognized flag on to the \ + positionals, or \"error\" to refuse it" + ), + )); + } + cli.unknown_flags = Some(mode); + } other => { return Err(syn::Error::new_spanned( path, format!( "unknown option `{other}` on a struct; usage::Cli takes \ - `name`, `bin`, and `version` here, and the description \ - comes from the doc comment" + `name`, `bin`, `version`, and `unknown_flags` here, \ + and the description comes from the doc comment" ), )); } diff --git a/docs/cli/reference/commands.json b/docs/cli/reference/commands.json index c6b0c85a..7fb6e491 100644 --- a/docs/cli/reference/commands.json +++ b/docs/cli/reference/commands.json @@ -51,6 +51,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "help": "Execute a shell script using bash", "help_long": "Execute a shell script with the specified shell\n\nTypically, this will be called by a script's shebang.\n\nIf using `var=#true` on args/flags, they will be joined with spaces using `shell_words::join()`\nto properly escape and quote values with spaces in them.", @@ -149,6 +150,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Generate shell completion candidates for a partial command line", "help_long": "Generate shell completion candidates for a partial command line\n\nThis is used internally by shell completion scripts to provide intelligent completions for commands, flags, and arguments.", @@ -214,6 +216,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "help": "Execute a script, parsing args and exposing them as environment variables", "name": "exec", @@ -267,6 +270,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "help": "Execute a shell script using fish", "help_long": "Execute a shell script with the specified shell\n\nTypically, this will be called by a script's shebang.\n\nIf using `var=#true` on args/flags, they will be joined with spaces using `shell_words::join()`\nto properly escape and quote values with spaces in them.", @@ -391,6 +395,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Generate shell completion scripts for bash, fish, nu, powershell, or zsh", "name": "completion", @@ -439,6 +444,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Generate a shell init script that auto-completes any usage shebang script on $PATH", "help_long": "Generate a shell init script that auto-completes any usage shebang script on $PATH\n\nSource the output once from your shell rc (e.g. ~/.bashrc) to enable tab-completion for any executable whose first line is a `usage` shebang — no per-script `usage g completion` step required.", @@ -508,6 +514,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Generate Fig completion spec for Amazon Q / Fig", "name": "fig", @@ -558,6 +565,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Outputs a usage spec in json format", "name": "json", @@ -629,6 +637,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "name": "manpage", "aliases": ["man"], @@ -745,6 +754,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Generate markdown documentation from usage specs", "name": "markdown", @@ -851,6 +861,7 @@ ], "mounts": [], "effect": "write", + "unknown_flags": null, "hide": false, "help": "Generate a type-safe SDK from a usage spec", "name": "sdk", @@ -863,6 +874,7 @@ "flags": [], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "subcommand_required": true, "help": "Generate completions, documentation, and other artifacts from usage specs", @@ -921,6 +933,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Lint a usage spec file for common issues", "name": "lint", @@ -971,6 +984,7 @@ ], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Serve a usage spec over the Model Context Protocol", "help_long": "Serve a usage spec over the Model Context Protocol\n\nReads JSON-RPC over stdin and writes responses to stdout, which is how MCP\nclients launch a local server. Point one at `usage mcp -f mycli.usage.kdl`.", @@ -1025,6 +1039,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "help": "Execute a shell script using PowerShell", "help_long": "Execute a shell script with the specified shell\n\nTypically, this will be called by a script's shebang.\n\nIf using `var=#true` on args/flags, they will be joined with spaces using `shell_words::join()`\nto properly escape and quote values with spaces in them.", @@ -1041,6 +1056,7 @@ "flags": [], "mounts": [], "effect": "read", + "unknown_flags": null, "hide": false, "help": "Show the companies sponsoring usage and the jdx.dev open source tools", "name": "sponsors", @@ -1094,6 +1110,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "help": "Execute a shell script using zsh", "help_long": "Execute a shell script with the specified shell\n\nTypically, this will be called by a script's shebang.\n\nIf using `var=#true` on args/flags, they will be joined with spaces using `shell_words::join()`\nto properly escape and quote values with spaces in them.", @@ -1127,6 +1144,7 @@ } ], "mounts": [], + "unknown_flags": null, "hide": false, "name": "usage", "aliases": [], @@ -1142,5 +1160,6 @@ "source_code_link_template": "https://github.com/jdx/usage/blob/main/cli/src/cli/{{path}}.rs", "repository": "https://github.com/jdx/usage", "about": "CLI for working with usage-based CLIs", - "min_usage_version": "4.0" + "min_usage_version": "4.0", + "unknown_flags": null } diff --git a/docs/spec/argv.md b/docs/spec/argv.md index b5d056a8..1be7d6e5 100644 --- a/docs/spec/argv.md +++ b/docs/spec/argv.md @@ -46,7 +46,8 @@ At each token, in order: 2. If the token is exactly `--`, flag interpretation stops. The token is consumed and is not itself a value. 3. If the token is flag-like, it is matched as a flag ([long](#long-flags) or - [short](#short-flags)). No match is an error. + [short](#short-flags)). If nothing matches, see + [unrecognized flags](#unrecognized-flags). 4. Otherwise the token is a word: it selects a [subcommand](#subcommands) if one matches, and is otherwise offered to the command's [positional arguments](#positional-arguments). @@ -58,7 +59,8 @@ A token beginning with `--` is a long flag. The name is the text up to the first **Names match exactly.** `--for` does not match `--force`. Abbreviation inference is deliberately absent: it makes adding a flag a breaking change for -anyone who typed a prefix that was unique until the new flag arrived. +anyone who typed a prefix that was unique until the new flag arrived. A prefix +that matches nothing is then an [unrecognized flag](#unrecognized-flags). For a flag that takes a value, the value comes from one of two forms: @@ -115,11 +117,54 @@ makes `-C/tmp` and `-Edev` work. One `=` immediately after the letter is a separator, matching the long form. Only one: `-j==8` is a value of `=8`. -An unrecognized letter fails the whole token, including any letters before it. A -partially applied bundle would be worse than a rejected one. +A token containing an unrecognized letter is not a bundle at all, so none of its +letters are applied: `-az` does not set `-a` on the way to discovering that `z` +names nothing. What happens to the token instead is described under +[unrecognized flags](#unrecognized-flags). `-` alone is not a flag. It is a value, conventionally meaning stdin. +## Unrecognized flags + +A flag-like token that names no flag in scope **becomes a word**, and is offered to +the positional arguments like any other. With nothing left to hold it, that is an +`unexpected_arg` — the same error an extra word produces, rather than a special one +about flags. + +This is where the grammar parts company with every comparable parser. clap, +argparse, commander, oclif v2+, and POSIX `getopt` all reject the token. They are +right for what they do, which is parse _their own_ argv, where a dash-word can only +be a flag or a typo. A usage spec is also used to parse command lines whose flags it +does not own: + +- a shell script run through `usage exec`, forwarding options to a tool it wraps +- a task's arguments, where the task script is the authority on what it accepts +- a completion, asked about a line that is still half-typed + +In all three, a token the spec has not heard of is far more likely to be data in +transit than a mistake, and refusing it would break the wrapper for everyone who +did not enumerate the flags of the program behind it. + +The cost is real and worth stating plainly: a misspelled `--hekp` becomes an +argument instead of an error, and whether it does depends on whether a positional +is free to take it. A CLI that owns all of its flags can have the stricter reading +by asking: + +```kdl +unknown_flags "error" // for the whole CLI +cmd "exec" unknown_flags="value" // except here, which forwards a command line +``` + +Unlike `effect`, this **is** inherited: the nearest enclosing command that states a +preference wins, then the spec, then `value`. It describes how a command line is +read rather than what a command does, and a CLI that forwards options tends to +forward them at every level. + +Even when refusing, a lone `-` and a negative number stay values — neither is a +misspelled flag, and without the second `--offset -1` could not be written. oclif +made exactly this mistake when it switched to refusing unknown flags, and had to +add the number case back afterwards. + ## Positional arguments A word that does not select a subcommand is offered to the command's arguments in diff --git a/docs/spec/reference/cmd.md b/docs/spec/reference/cmd.md index 0714062a..fab33bc3 100644 --- a/docs/spec/reference/cmd.md +++ b/docs/spec/reference/cmd.md @@ -121,6 +121,22 @@ completion offering a command that running would hand to the default subcommand instead is worse than not offering it. So a root mount under a `default_subcommand` contributes nothing anywhere until it asks to outrank it. +### Unknown flags + +A flag-like token that names no flag becomes a word, offered to the positionals like +any other — because a spec often parses a command line whose flags belong to +something else. A command that owns all of its flags can refuse them instead: + +```kdl +unknown_flags "error" // for the whole CLI +cmd "exec" unknown_flags="value" // except here, which forwards a command line +``` + +The nearest command that states a preference wins, then the spec, then `value`. +Unlike [`effect`](#effect), this is inherited: it describes how a command line is +read rather than what a command does. See +[the argv grammar](/spec/argv#unrecognized-flags) for the reasoning and the cost. + ### Global flags and mounted commands A mounted command describes a different program, so the flags of the commands it is mounted under diff --git a/lib/src/docs/models.rs b/lib/src/docs/models.rs index 7907162d..284557fa 100644 --- a/lib/src/docs/models.rs +++ b/lib/src/docs/models.rs @@ -275,6 +275,8 @@ impl From<&crate::SpecCommand> for SpecCommand { after_help_md, examples, restart_token, + // How a command line is read, which no rendered page shows. + unknown_flags: _, // Rendered above, or deliberately absent from the docs model. args: _, flags: _, diff --git a/lib/src/lib.rs b/lib/src/lib.rs index b6c6e23f..aa40ff22 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -11,6 +11,7 @@ pub use crate::spec::complete::SpecComplete; pub use crate::spec::effect::SpecCommandEffect; pub use crate::spec::flag::SpecFlag; pub use crate::spec::mount::SpecMount; +pub use crate::spec::unknown_flags::UnknownFlags; pub use crate::spec::Spec; #[macro_use] diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 6cca1b85..89cdc826 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -12,6 +12,7 @@ use strum::EnumTryAs; use crate::docs; use crate::error::UsageErr; use crate::spec::arg::SpecDoubleDashChoices; +use crate::spec::unknown_flags::UnknownFlags; use crate::{Spec, SpecArg, SpecChoices, SpecCommand, SpecFlag}; /// Merge a subcommand's flags into the currently available flags when descending @@ -948,6 +949,7 @@ fn parse_partial_with_env( record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok((out, overridden_flags)); } + reject_unknown_flag_if_asked(spec, &out.cmds, &w)?; } // short flags @@ -992,6 +994,7 @@ fn parse_partial_with_env( record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok((out, overridden_flags)); } + reject_unknown_flag_if_asked(spec, &out.cmds, &w)?; if grouped_flag { grouped_flag = false; w.remove(0); @@ -1281,6 +1284,62 @@ impl<'a> ChoiceTarget<'a> { } } +/// Refuse a flag-like token that named nothing, if this command asked for that. +/// +/// Called from the flag branches, where the lookup has just failed and nothing from +/// the token has been applied yet — so a bundle like `-az` is refused whole rather +/// than after setting `-a`. +fn reject_unknown_flag_if_asked( + spec: &Spec, + path: &[SpecCommand], + token: &str, +) -> Result<(), UsageErr> { + // A lone `-` is a value by convention and a negative number is a value because + // no CLI could accept `--offset -1` otherwise. Both reach the short-flag branch + // and neither is a misspelled flag, so neither is refused. oclif made exactly + // this mistake when it switched to refusing unknown flags, and had to add the + // number case back. + if !is_flag_like(token) { + return Ok(()); + } + if effective_unknown_flags(spec, path) != UnknownFlags::Error { + return Ok(()); + } + Err(UsageErr::InvalidFlag { + token: token.to_string(), + reason: "no such flag".to_string(), + span: (0, 0).into(), + input: token.to_string(), + }) +} + +/// Whether a flag-like token that matches nothing is a value or an error, here. +/// +/// The nearest enclosing command that stated a preference wins, then the spec, +/// then the default. Inherited, unlike `effect`: it describes how a command line +/// is read, and a CLI that forwards options tends to forward them at every level. +fn effective_unknown_flags(spec: &Spec, path: &[SpecCommand]) -> UnknownFlags { + path.iter() + .rev() + .find_map(|cmd| cmd.unknown_flags) + .or(spec.unknown_flags) + .unwrap_or_default() +} + +/// Whether a token would be read as a flag, for the purpose of rejecting unknown +/// ones. A lone `-` is a value by convention, and a negative number is a value +/// because no CLI could accept `--offset -1` otherwise. +fn is_flag_like(token: &str) -> bool { + let mut chars = token.chars(); + if chars.next() != Some('-') { + return false; + } + match chars.next() { + None => false, + Some(c) => !c.is_ascii_digit(), + } +} + fn drain_pending_flag_values( spec: &Spec, cmd: &SpecCommand, @@ -2390,6 +2449,90 @@ flag "--file " required_unless="--stdin" } } + #[test] + fn unknown_flags_are_values_by_default() { + // The default, and the reason it is the default: a spec often parses a + // command line whose flags belong to something else. + let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--force\"\narg \"[rest]...\"\n" + .parse() + .unwrap(); + let out = parse( + &spec, + &["ex".to_string(), "--wat".to_string(), "x".to_string()], + ) + .unwrap(); + let rest = out.args.keys().find(|a| a.name == "rest").unwrap(); + assert_eq!(out.args[rest].to_string(), "--wat x"); + } + + #[test] + fn unknown_flags_can_be_rejected_for_the_whole_cli() { + let spec: Spec = + "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\nflag \"--force\"\narg \"[rest]...\"\n" + .parse() + .unwrap(); + let err = parse(&spec, &["ex".to_string(), "--wat".to_string()]).unwrap_err(); + assert!( + err.to_string().contains("--wat"), + "the message should name the token: {err}" + ); + + // A negative number is still a value, which is the carve-out oclif had to + // add back after making this its default. + let out = parse(&spec, &["ex".to_string(), "-1".to_string()]).unwrap(); + let rest = out.args.keys().find(|a| a.name == "rest").unwrap(); + assert_eq!(out.args[rest].to_string(), "-1"); + } + + #[test] + fn a_command_may_override_the_cli_wide_setting() { + // Strict overall, lenient for the one command that forwards options. + let spec: Spec = r#" +name "ex" +bin "ex" +unknown_flags "error" +cmd "exec" unknown_flags="value" { + arg "[rest]..." +} +cmd "build" { + arg "[rest]..." +} +"# + .parse() + .unwrap(); + + let out = parse( + &spec, + &["ex".to_string(), "exec".to_string(), "--wat".to_string()], + ) + .unwrap(); + let rest = out.args.keys().find(|a| a.name == "rest").unwrap(); + assert_eq!(out.args[rest].to_string(), "--wat"); + + assert!( + parse( + &spec, + &["ex".to_string(), "build".to_string(), "--wat".to_string()] + ) + .is_err(), + "a command that says nothing inherits the CLI's choice" + ); + } + + #[test] + fn the_setting_survives_a_round_trip() { + let spec: Spec = + "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\ncmd \"x\" unknown_flags=\"value\"\n" + .parse() + .unwrap(); + let reparsed: Spec = spec.to_string().parse().unwrap(); + assert_eq!(reparsed.unknown_flags, Some(UnknownFlags::Error)); + assert_eq!( + reparsed.cmd.subcommands["x"].unknown_flags, + Some(UnknownFlags::Value) + ); + } + #[test] fn test_default_subcommand() { // Test that default_subcommand routes to the specified subcommand diff --git a/lib/src/spec/cmd.rs b/lib/src/spec/cmd.rs index 8d1a9ded..702eb3b8 100644 --- a/lib/src/spec/cmd.rs +++ b/lib/src/spec/cmd.rs @@ -9,6 +9,7 @@ use crate::spec::effect::{SpecCommandEffect, EFFECT_VALUES}; use crate::spec::helpers::{string_entry, NodeHelper}; use crate::spec::is_false; use crate::spec::mount::SpecMount; +use crate::spec::unknown_flags::UnknownFlags; use crate::{Spec, SpecArg, SpecComplete, SpecFlag}; use indexmap::IndexMap; use itertools::Itertools; @@ -54,6 +55,14 @@ pub struct SpecCommand { /// Not inherited by subcommands. #[serde(skip_serializing_if = "Option::is_none")] pub effect: Option, + /// What to do here with a flag-like token that names no declared flag. + /// + /// Unset means "whatever encloses this command decided" — the nearest command + /// above that set one, or failing that the spec, or failing that + /// [`UnknownFlags::Value`]. Unlike [`SpecCommandEffect`] this *is* inherited, + /// because it describes how a command line is read rather than what a command + /// does, and a CLI that forwards options generally forwards them everywhere. + pub unknown_flags: Option, /// Whether to hide this command from help output pub hide: bool, /// True when this command came from a [`SpecMount`], i.e. it describes another @@ -140,6 +149,7 @@ impl Default for SpecCommand { mounts: vec![], deprecated: None, effect: None, + unknown_flags: None, hide: false, mounted: false, flags_from_mount: false, @@ -250,6 +260,18 @@ impl SpecCommand { "after_help_md" => cmd.after_help_md = Some(v.ensure_string()?), "subcommand_required" => cmd.subcommand_required = v.ensure_bool()?, "hide" => cmd.hide = v.ensure_bool()?, + "unknown_flags" => { + let raw = v.ensure_string()?; + match raw.parse() { + Ok(mode) => cmd.unknown_flags = Some(mode), + Err(_) => bail_parse!( + ctx, + v.entry.span(), + "unsupported unknown_flags {raw}, expected one of: {}", + crate::spec::unknown_flags::UNKNOWN_FLAGS_VALUES + ), + } + } "effect" => { let raw = v.ensure_string()?; match raw.parse() { @@ -456,6 +478,7 @@ impl SpecCommand { complete, deprecated, effect, + unknown_flags, // Recomputed from the merged command, never carried over. full_cmd: _, usage: _, @@ -516,6 +539,9 @@ impl SpecCommand { if effect.is_some() { self.effect = effect; } + if unknown_flags.is_some() { + self.unknown_flags = unknown_flags; + } if deprecated.is_some() { self.deprecated = deprecated; } @@ -607,6 +633,7 @@ impl From<&SpecCommand> for KdlNode { hide, subcommand_required, restart_token, + unknown_flags, aliases, hidden_aliases, help, @@ -720,6 +747,10 @@ impl From<&SpecCommand> for KdlNode { node.entries_mut() .push(string_entry(Some("effect"), effect.as_str())); } + if let Some(unknown_flags) = unknown_flags { + node.entries_mut() + .push(string_entry(Some("unknown_flags"), unknown_flags.as_str())); + } for flag in flags { let children = node.children_mut().get_or_insert_with(KdlDocument::new); children.nodes_mut().push(flag.into()); diff --git a/lib/src/spec/mod.rs b/lib/src/spec/mod.rs index cfe9ba93..835d6d61 100644 --- a/lib/src/spec/mod.rs +++ b/lib/src/spec/mod.rs @@ -10,6 +10,7 @@ pub mod effect; pub mod flag; pub mod helpers; pub mod mount; +pub mod unknown_flags; use indexmap::IndexMap; use kdl::{KdlDocument, KdlEntry, KdlNode}; @@ -78,6 +79,9 @@ pub struct Spec { /// This enables "naked" command syntax like `mise foo` instead of `mise run foo`. #[serde(skip_serializing_if = "Option::is_none")] pub default_subcommand: Option, + /// What to do with a flag-like token that names no declared flag, for the whole + /// CLI. A command may override it; see [`SpecCommand::unknown_flags`]. + pub unknown_flags: Option, } impl Spec { @@ -223,6 +227,18 @@ impl Spec { check_usage_version(&v); schema.min_usage_version = Some(v); } + "unknown_flags" => { + let raw = node.arg(0)?.ensure_string()?; + match raw.parse() { + Ok(mode) => schema.unknown_flags = Some(mode), + Err(_) => bail_parse!( + ctx, + node.span(), + "unsupported unknown_flags {raw}, expected one of: {}", + crate::spec::unknown_flags::UNKNOWN_FLAGS_VALUES + ), + } + } "default_subcommand" => { schema.default_subcommand = Some(node.arg(0)?.ensure_string()?) } @@ -477,6 +493,11 @@ impl Display for Spec { node.push(string_entry(None, min_usage_version)); nodes.push(node); } + if let Some(unknown_flags) = &self.unknown_flags { + let mut node = KdlNode::new("unknown_flags"); + node.push(string_entry(None, unknown_flags.as_str())); + nodes.push(node); + } if let Some(default_subcommand) = &self.default_subcommand { let mut node = KdlNode::new("default_subcommand"); node.push(string_entry(None, default_subcommand)); diff --git a/lib/src/spec/unknown_flags.rs b/lib/src/spec/unknown_flags.rs new file mode 100644 index 00000000..23bd5c37 --- /dev/null +++ b/lib/src/spec/unknown_flags.rs @@ -0,0 +1,49 @@ +use serde::Serialize; +use strum::{Display as StrumDisplay, EnumString}; + +/// What to do with a token that looks like a flag but names no declared flag. +/// +/// The default is [`UnknownFlags::Value`], which is where this parser parts +/// company with clap, argparse, commander, oclif v2+, and POSIX `getopt` — all of +/// which reject the token. The reason is that those parse *their own* argv, where +/// a dash-word can only be a flag or a typo, while a usage spec is also used to +/// parse things whose flags it does not own: +/// +/// - a shell script run through `usage exec`, forwarding options to a tool it wraps +/// - a task's arguments, where the task script is the authority on what it accepts +/// - a completion, asked about a command line that is still being typed +/// +/// In all three, a dash-word the spec has not heard of is far more likely to be +/// data in transit than a mistake, and rejecting it would break the wrapper for +/// everyone who did not enumerate the flags of the program behind it. +/// +/// The cost is real and worth stating: a misspelled `--hekp` becomes an argument +/// instead of an error, and whether it does depends on whether a positional is +/// free to take it. A CLI that owns all of its flags — as opposed to forwarding +/// them — should say [`UnknownFlags::Error`] and get the stricter reading. +#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, EnumString, StrumDisplay, Serialize)] +#[strum(serialize_all = "snake_case")] +#[serde(rename_all = "snake_case")] +pub enum UnknownFlags { + /// Offer the token to the positional arguments, like any other word. If none + /// can take it, it is an unexpected argument — the same error an extra word + /// would produce. + #[default] + Value, + /// Reject the token. A CLI whose flags are all its own gets typo detection + /// this way, at the price of needing `--` to pass a value that begins with a + /// dash. + Error, +} + +impl UnknownFlags { + pub fn as_str(&self) -> &'static str { + match self { + UnknownFlags::Value => "value", + UnknownFlags::Error => "error", + } + } +} + +/// The values a spec may use, for error messages. +pub(crate) const UNKNOWN_FLAGS_VALUES: &str = "value, error"; From 55a9f36d2f72d9b7c5ca83fbcd388a6b9df931eb Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:58:00 +0000 Subject: [PATCH 3/5] fix(parse): read a short token whole before applying any of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, and the first is the interesting one. `-az` with only `-a` declared was read as a bundle: `-a` was applied and `z` left over as a value. A token containing an unrecognized letter is not a bundle at all, so none of its letters apply — the grammar said so and usage-argv did it, but usage-lib did not. The check had to go in phase 1, not just phase 2. Phase 1 keys a short token on its first letter, so `-az` was recorded as a binding for `-a`, and phase 2 trusting that binding is what defeated a check placed only there. It now decides whether the whole token is a bundle where the token is first read. The corpus could not see this: the vector for it declares no argument, so both readings produce `unexpected_arg`. A second vector gives it a free argument, where the difference is the whole point — `-a` unset and `-az` bound as one word. `is_flag_like` also treated anything after a digit as a number, so `-1x` slipped past a CLI that asked for unknown flags to be refused. It now asks whether the token *is* a number. usage-argv gets the same rule, spelled out by hand rather than deferred to a float parse, since it runs on the hot path. And `Spec::merge` dropped `unknown_flags`, so an `include` lost the policy it declared. Plus a docs link that markdownlint could not resolve. Co-Authored-By: Claude Fable 5 --- argv/src/lib.rs | 21 +++++++++++- corpus/02-short-flags.json | 14 ++++++++ docs/spec/reference/cmd.md | 2 +- lib/src/parse.rs | 67 +++++++++++++++++++++++++++++++------- lib/src/spec/mod.rs | 1 + 5 files changed, 92 insertions(+), 13 deletions(-) diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 23d2611a..38049e66 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -679,11 +679,30 @@ fn bytes<'v>(s: &'v &'v OsStr) -> &'v [u8] { /// without which no CLI could accept `--offset -1`. fn is_flag_like(token: &[u8]) -> bool { match token { - [b'-', rest @ ..] if !rest.is_empty() => !rest[0].is_ascii_digit(), + [b'-', rest @ ..] if !rest.is_empty() => !is_number(rest), _ => false, } } +/// Whether the text after a `-` is a number, so `-1` and `-2.5` are values while +/// `-1x` is a flag-shaped token that names nothing. +/// +/// Written out rather than deferred to `f64::from_str` because this runs on the hot +/// path and a parse would have to go through `str`, which means a UTF-8 check on a +/// slice that has already been decided by its bytes. +fn is_number(rest: &[u8]) -> bool { + let mut seen_digit = false; + let mut seen_dot = false; + for &b in rest { + match b { + b'0'..=b'9' => seen_digit = true, + b'.' if !seen_dot => seen_dot = true, + _ => return false, + } + } + seen_digit +} + #[cfg(test)] mod tests { use super::*; diff --git a/corpus/02-short-flags.json b/corpus/02-short-flags.json index 61273b6a..97f6d5cb 100644 --- a/corpus/02-short-flags.json +++ b/corpus/02-short-flags.json @@ -65,6 +65,13 @@ "argv": ["-az"], "expect": { "error": "unexpected_arg" } }, + { + "id": "short-bundle-unknown-letter-applies-nothing", + "doc": "The same rejection, seen from the side that matters: with an argument free to take the token, `-a` must still be unset. A token containing an unrecognized letter was never a bundle, so none of its letters apply \u2014 the previous vector cannot show this, because both readings produce the same error.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"-a --all\"\narg \"[file]\"\n", + "argv": ["-az"], + "expect": { "ok": { "args": { "file": "-az" } } } + }, { "id": "short-count", "doc": "A `count` flag records how many times it appeared, so `-vvv` is three.", @@ -99,6 +106,13 @@ "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\nflag \"-a --all\"\n", "argv": ["-az"], "expect": { "error": "unknown_flag" } + }, + { + "id": "short-digit-token-that-is-not-a-number", + "doc": "`-1x` is not a number, so it is a flag-shaped token that names nothing \u2014 a CLI refusing unknown flags hears about it rather than having it slip through as a value.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[file]\"\n", + "argv": ["-1x"], + "expect": { "error": "unknown_flag" } } ] } diff --git a/docs/spec/reference/cmd.md b/docs/spec/reference/cmd.md index fab33bc3..ea77b4f8 100644 --- a/docs/spec/reference/cmd.md +++ b/docs/spec/reference/cmd.md @@ -135,7 +135,7 @@ cmd "exec" unknown_flags="value" // except here, which forwards a command lin The nearest command that states a preference wins, then the spec, then `value`. Unlike [`effect`](#effect), this is inherited: it describes how a command line is read rather than what a command does. See -[the argv grammar](/spec/argv#unrecognized-flags) for the reasoning and the cost. +[the argv grammar](../argv.md#unrecognized-flags) for the reasoning and the cost. ### Global flags and mounted commands diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 89cdc826..8bbed30d 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -718,7 +718,19 @@ fn parse_partial_with_env( let word = input[idx].clone(); let flag_key = get_flag_key(&word); - if let Some(f) = out.available_flags.get(flag_key).cloned() { + // A short token keys on its first letter, so `-az` would be recorded as + // `-a` and its tail left over. Check the whole token here, where it is + // first read: a token containing an unrecognized letter is not a bundle, + // and recording it as one is what let `-a` be applied from a token that + // never named it. + let is_bundle = + word.starts_with("--") || short_bundle_is_known(&out.available_flags, &word); + if let Some(f) = out + .available_flags + .get(flag_key) + .cloned() + .filter(|_| is_bundle) + { // Skip the flag and keep scanning. Both global and non-global flags may precede // a subcommand (`mycli --verbose run task`, `mycli run --force task`), and // stopping at one would hide the subcommand — and any mount on it — from the @@ -953,7 +965,25 @@ fn parse_partial_with_env( } // short flags - if enable_flags && w.starts_with('-') && w.len() > 1 { + // + // A fresh token is checked whole before any of it is applied: `-az` with only + // `-a` declared is not a bundle at all, so it must not set `a` on the way to + // discovering that `z` names nothing. A grouped continuation is exempt — its + // token was already checked when it arrived. + if enable_flags + && !grouped_flag + // A word phase 1 already resolved to a flag needs no re-checking, and + // the flags in scope have changed since, so re-checking would be wrong. + && binding.is_none() + && w.starts_with('-') + && w.len() > 1 + && is_flag_like(&w) + && !short_bundle_is_known(&out.available_flags, &w) + { + // Refused if this command asked for that; otherwise it carries on below + // as one word, with none of its letters applied. + reject_unknown_flag_if_asked(spec, &out.cmds, &w)?; + } else if enable_flags && w.starts_with('-') && w.len() > 1 { let short = w.chars().nth(1).unwrap(); if let Some(f) = binding .as_ref() @@ -1284,6 +1314,21 @@ impl<'a> ChoiceTarget<'a> { } } +/// Whether every letter of a short token names a flag in scope. +/// +/// Scanning stops at the first letter whose flag takes a value, because everything +/// after it is that value rather than more letters. +fn short_bundle_is_known(available: &BTreeMap>, token: &str) -> bool { + for c in token.chars().skip(1) { + match available.get(&format!("-{c}")) { + None => return false, + Some(f) if f.arg.is_some() => return true, + Some(_) => {} + } + } + true +} + /// Refuse a flag-like token that named nothing, if this command asked for that. /// /// Called from the flag branches, where the lookup has just failed and nothing from @@ -1327,16 +1372,16 @@ fn effective_unknown_flags(spec: &Spec, path: &[SpecCommand]) -> UnknownFlags { } /// Whether a token would be read as a flag, for the purpose of rejecting unknown -/// ones. A lone `-` is a value by convention, and a negative number is a value -/// because no CLI could accept `--offset -1` otherwise. +/// ones. +/// +/// A lone `-` is a value by convention, and a negative number is a value because no +/// CLI could accept `--offset -1` otherwise. The number has to actually be one: +/// treating anything after a digit as numeric would let `-1x` slip past a CLI that +/// asked for unknown flags to be refused. fn is_flag_like(token: &str) -> bool { - let mut chars = token.chars(); - if chars.next() != Some('-') { - return false; - } - match chars.next() { - None => false, - Some(c) => !c.is_ascii_digit(), + match token.strip_prefix('-') { + None | Some("") => false, + Some(rest) => rest.parse::().is_err(), } } diff --git a/lib/src/spec/mod.rs b/lib/src/spec/mod.rs index 835d6d61..53bdc20e 100644 --- a/lib/src/spec/mod.rs +++ b/lib/src/spec/mod.rs @@ -335,6 +335,7 @@ impl Spec { merge_opt!(disable_help); merge_opt!(min_usage_version); merge_opt!(default_subcommand); + merge_opt!(unknown_flags); merge_extend!(complete); merge_extend!(examples); From 938f8f95eaf67abe949e4d53937594473a4ec701 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:25:05 +0000 Subject: [PATCH 4/5] fix(parse): agree about what a number is, in both parsers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two implementations disagreed about `-1e5`. usage-lib asked `f64::from_str`, which accepts an exponent; usage-argv used a hand-written scanner that stopped at `e` and called the token a flag. So a CLI refusing unknown flags would accept `--offset -1e5` under one parser and reject it under the other — exactly the drift the corpus exists to prevent, in the one place the corpus had no vector. One rule now, spelled out identically in both: digits, at most one `.`, and an optional signed exponent. Narrower than a float parse on purpose, since `-inf` is far likelier to be a misspelled flag than a number somebody meant to pass. Four vectors pin the edges — `-1e5`, `-1.5e-3`, `-inf`, `-1e` — so the two cannot drift again without a test saying so. Also emits `unknown_flags` for a subcommand that differs from the command enclosing it. The tables carry the effective value per command, so repeating an inherited answer says nothing, but a command that differs has to say so or the setting never reaches the spec. Co-Authored-By: Claude Fable 5 --- argv/src/lib.rs | 39 ++++++++++++++--- argv/src/spec.rs | 86 +++++++++++++++++++++++++++++++++++++- corpus/02-short-flags.json | 28 +++++++++++++ docs/spec/argv.md | 9 +++- lib/src/parse.rs | 39 ++++++++++++++++- 5 files changed, 190 insertions(+), 11 deletions(-) diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 38049e66..6dcbfc3b 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -684,23 +684,50 @@ fn is_flag_like(token: &[u8]) -> bool { } } -/// Whether the text after a `-` is a number, so `-1` and `-2.5` are values while -/// `-1x` is a flag-shaped token that names nothing. +/// Whether the text after a `-` is a number, so `-1`, `-2.5`, and `-1e5` are values +/// while `-1x` is a flag-shaped token that names nothing. +/// +/// Digits, at most one `.`, and an optional exponent. Deliberately narrower than +/// `f64::from_str`, which also accepts `inf` and `NaN` — `-inf` is far likelier to be +/// a misspelled flag than a number somebody meant to pass. +/// +/// usage-lib applies the same rule, and the corpus pins the edges so the two cannot +/// drift apart: they disagreed about `-1e5` when this was a hand-rolled scanner on +/// one side and a float parse on the other. /// /// Written out rather than deferred to `f64::from_str` because this runs on the hot -/// path and a parse would have to go through `str`, which means a UTF-8 check on a -/// slice that has already been decided by its bytes. +/// path, and a parse would mean a UTF-8 check on a slice already decided by its +/// bytes. fn is_number(rest: &[u8]) -> bool { + let (mantissa, exponent) = match rest.iter().position(|b| matches!(b, b'e' | b'E')) { + Some(at) => (&rest[..at], Some(&rest[at + 1..])), + None => (rest, None), + }; + let mut seen_digit = false; let mut seen_dot = false; - for &b in rest { + for &b in mantissa { match b { b'0'..=b'9' => seen_digit = true, b'.' if !seen_dot => seen_dot = true, _ => return false, } } - seen_digit + if !seen_digit { + return false; + } + + match exponent { + None => true, + // An exponent needs digits of its own, and may carry a sign. + Some(exp) => { + let digits = exp + .strip_prefix(b"+") + .or_else(|| exp.strip_prefix(b"-")) + .unwrap_or(exp); + !digits.is_empty() && digits.iter().all(|b| b.is_ascii_digit()) + } + } } #[cfg(test)] diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 0851f0f2..8357ccef 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -287,6 +287,7 @@ impl Spec<'_> { /// Separate from [`write_command`] because the root's contents sit at the top /// level of the document rather than inside a `cmd` node. fn write_body(out: &mut String, meta: &CommandMeta<'_>, depth: usize) -> core::fmt::Result { + let enclosing_unknown_flags = meta.cmd.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!( @@ -327,12 +328,17 @@ fn write_body(out: &mut String, meta: &CommandMeta<'_>, depth: usize) -> core::f write_arg(out, arg, depth)?; } for sub in meta.subcommands { - write_command(out, sub, depth)?; + write_command(out, sub, depth, enclosing_unknown_flags)?; } Ok(()) } -fn write_command(out: &mut String, meta: &CommandMeta<'_>, depth: usize) -> core::fmt::Result { +fn write_command( + out: &mut String, + meta: &CommandMeta<'_>, + depth: usize, + inherited_unknown_flags: UnknownFlags, +) -> core::fmt::Result { indent(out, depth)?; write!(out, "cmd {}", quoted(meta.cmd.name))?; if let Some(help) = meta.about { @@ -344,6 +350,33 @@ fn write_command(out: &mut String, meta: &CommandMeta<'_>, depth: usize) -> core 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 { + write!( + out, + " unknown_flags={}", + quoted(match meta.cmd.unknown_flags { + UnknownFlags::Value => "value", + UnknownFlags::Error => "error", + }) + )?; + } + // Written only where it changes, since the spec inherits it: the tables hold the + // effective value per command, so the same value as the enclosing one says nothing. + let unknown_flags = meta.cmd.unknown_flags; + if unknown_flags != inherited_unknown_flags { + write!( + out, + " unknown_flags={}", + quoted(match unknown_flags { + UnknownFlags::Value => "value", + UnknownFlags::Error => "error", + }) + )?; + } if let Some(token) = meta.restart_token { write!(out, " restart_token={}", quoted(token))?; } @@ -699,6 +732,55 @@ mod tests { assert_eq!(quoted("one\ntwo"), r#""one\ntwo""#); } + #[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. + static STRICT_SUB: Command = Command { + name: "build", + unknown_flags: UnknownFlags::Error, + ..Command::EMPTY + }; + static LENIENT_SUB: Command = Command { + name: "exec", + unknown_flags: UnknownFlags::Value, + ..Command::EMPTY + }; + static ROOT: Command = Command { + name: "ex", + subcommands: &[&STRICT_SUB, &LENIENT_SUB], + unknown_flags: UnknownFlags::Error, + ..Command::EMPTY + }; + static STRICT_META: CommandMeta = CommandMeta { + cmd: &STRICT_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], + ..CommandMeta::EMPTY + }; + + let mut out = String::new(); + write_body(&mut out, &ROOT_META, 0).unwrap(); + + // `build` matches the root, so it stays quiet; `exec` differs, so it says so. + assert!( + out.contains(r#"cmd "build""#) && !out.contains(r#"cmd "build" unknown_flags"#), + "a matching subcommand should not repeat the setting:\n{out}" + ); + assert!( + out.contains(r#"cmd "exec" unknown_flags="value""#), + "a differing subcommand has to declare it:\n{out}" + ); + } + #[test] fn flag_forms_lists_shorts_then_longs() { static F: Flag = Flag { diff --git a/corpus/02-short-flags.json b/corpus/02-short-flags.json index 97f6d5cb..d603601d 100644 --- a/corpus/02-short-flags.json +++ b/corpus/02-short-flags.json @@ -113,6 +113,34 @@ "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[file]\"\n", "argv": ["-1x"], "expect": { "error": "unknown_flag" } + }, + { + "id": "short-exponent-number-is-a-value", + "doc": "`-1e5` is a number, so it is a value even where unknown flags are refused. The two parsers disagreed about this when one used a float parse and the other a hand-written scanner, which is why the edges are pinned here.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[offset]\"\n", + "argv": ["-1e5"], + "expect": { "ok": { "args": { "offset": "-1e5" } } } + }, + { + "id": "short-fractional-exponent-is-a-value", + "doc": "A signed exponent on a fraction is still a number.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[offset]\"\n", + "argv": ["-1.5e-3"], + "expect": { "ok": { "args": { "offset": "-1.5e-3" } } } + }, + { + "id": "short-infinity-is-not-a-number-here", + "doc": "`-inf` parses as a float but is not treated as one: a token like that is far likelier to be a misspelled flag than a number somebody meant to pass, so a CLI refusing unknown flags hears about it.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[offset]\"\n", + "argv": ["-inf"], + "expect": { "error": "unknown_flag" } + }, + { + "id": "short-bare-exponent-is-not-a-number", + "doc": "An exponent needs digits of its own, so `-1e` names nothing.", + "spec": "name \"ex\"\nbin \"ex\"\nunknown_flags \"error\"\narg \"[offset]\"\n", + "argv": ["-1e"], + "expect": { "error": "unknown_flag" } } ] } diff --git a/docs/spec/argv.md b/docs/spec/argv.md index 1be7d6e5..969d4e40 100644 --- a/docs/spec/argv.md +++ b/docs/spec/argv.md @@ -28,8 +28,13 @@ A **command line** is the tokens after the program name. `mycli install -f x` has three. A token is **flag-like** when it begins with `-`, is longer than one character, -and is not a negative number (`-` followed by a digit). So `--force`, `-f`, and -`-abc` are flag-like; `-`, `-1`, and `-2.5` are not. +and is not a negative number. So `--force`, `-f`, and `-abc` are flag-like; `-`, +`-1`, `-2.5`, and `-1e5` are not. + +A number here means digits, at most one `.`, and an optional exponent — +deliberately narrower than what a float parser accepts, since `-inf` is far +likelier to be a misspelled flag than a number somebody meant to pass. `-1x` and +`-1e` are not numbers either, and so name flags that do not exist. ## Reading a command line diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 8bbed30d..3b6d5f3b 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1381,7 +1381,44 @@ fn effective_unknown_flags(spec: &Spec, path: &[SpecCommand]) -> UnknownFlags { fn is_flag_like(token: &str) -> bool { match token.strip_prefix('-') { None | Some("") => false, - Some(rest) => rest.parse::().is_err(), + Some(rest) => !is_number(rest), + } +} + +/// Digits, at most one `.`, and an optional exponent. +/// +/// Spelled out rather than deferred to `f64::from_str`, which also accepts `inf` and +/// `NaN`: `-inf` is far likelier to be a misspelled flag than a number somebody meant +/// to pass. usage-argv implements the same rule, and the corpus pins the edges — the +/// two disagreed about `-1e5` when one used a float parse and the other did not. +fn is_number(rest: &str) -> bool { + let (mantissa, exponent) = match rest.find(['e', 'E']) { + Some(at) => (&rest[..at], Some(&rest[at + 1..])), + None => (rest, None), + }; + + let mut seen_digit = false; + let mut seen_dot = false; + for c in mantissa.chars() { + match c { + '0'..='9' => seen_digit = true, + '.' if !seen_dot => seen_dot = true, + _ => return false, + } + } + if !seen_digit { + return false; + } + + match exponent { + None => true, + Some(exp) => { + let digits = exp + .strip_prefix('+') + .or_else(|| exp.strip_prefix('-')) + .unwrap_or(exp); + !digits.is_empty() && digits.chars().all(|c| c.is_ascii_digit()) + } } } From 2dd75908c41c18ffe2aaa1b60f912849297e8457 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:28:33 +0000 Subject: [PATCH 5/5] fix(argv): write a subcommand's unknown_flags once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property was written twice, so an overriding subcommand emitted `unknown_flags="value" unknown_flags="value"` — a KDL node carrying the same property twice, which keeps only the last. Exactly the bug this same pull request fixes for `overrides` and `required_unless`, reintroduced two commits later by a botched relocation of the block. The test did not catch it because it asked whether the output *contained* `unknown_flags="value"`, which a doubled write also does. It now finds the line and counts, and I checked it fails on the duplicate before removing it. Also states the numeric grammar exactly — digits, at most one `.`, then optionally `e`/`E`, an optional sign, and at least one digit — rather than leaving "an optional exponent" to be guessed at, and uses the American spelling the locale check wants. Co-Authored-By: Claude Fable 5 --- argv/src/spec.rs | 40 +++++++++++++++++++++------------------- docs/spec/argv.md | 12 +++++++----- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 8357ccef..df5dec92 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -364,19 +364,6 @@ fn write_command( }) )?; } - // Written only where it changes, since the spec inherits it: the tables hold the - // effective value per command, so the same value as the enclosing one says nothing. - let unknown_flags = meta.cmd.unknown_flags; - if unknown_flags != inherited_unknown_flags { - write!( - out, - " unknown_flags={}", - quoted(match unknown_flags { - UnknownFlags::Value => "value", - UnknownFlags::Error => "error", - }) - )?; - } if let Some(token) = meta.restart_token { write!(out, " restart_token={}", quoted(token))?; } @@ -770,14 +757,29 @@ mod tests { let mut out = String::new(); write_body(&mut out, &ROOT_META, 0).unwrap(); - // `build` matches the root, so it stays quiet; `exec` differs, so it says so. + // Counted rather than checked with `contains`, which is how a duplicated + // write survived review: `unknown_flags="value" unknown_flags="value"` contains + // the string it was checked for. A KDL node carrying the same property twice + // keeps only the last, so once is the whole point. + let line = |name: &str| -> String { + out.lines() + .map(str::trim) + .find(|l| l.starts_with(&format!(r#"cmd "{name}""#))) + .unwrap_or_else(|| panic!("no `{name}` command was written:\n{out}")) + .to_string() + }; + + let build = line("build"); assert!( - out.contains(r#"cmd "build""#) && !out.contains(r#"cmd "build" unknown_flags"#), - "a matching subcommand should not repeat the setting:\n{out}" + !build.contains("unknown_flags"), + "a subcommand matching the enclosing command should not repeat it: {build}" ); - assert!( - out.contains(r#"cmd "exec" unknown_flags="value""#), - "a differing subcommand has to declare it:\n{out}" + + let exec = line("exec"); + assert_eq!( + exec.matches(r#"unknown_flags="value""#).count(), + 1, + "a differing subcommand declares it exactly once: {exec}" ); } diff --git a/docs/spec/argv.md b/docs/spec/argv.md index 969d4e40..3fe13b97 100644 --- a/docs/spec/argv.md +++ b/docs/spec/argv.md @@ -31,10 +31,12 @@ A token is **flag-like** when it begins with `-`, is longer than one character, and is not a negative number. So `--force`, `-f`, and `-abc` are flag-like; `-`, `-1`, `-2.5`, and `-1e5` are not. -A number here means digits, at most one `.`, and an optional exponent — -deliberately narrower than what a float parser accepts, since `-inf` is far -likelier to be a misspelled flag than a number somebody meant to pass. `-1x` and -`-1e` are not numbers either, and so name flags that do not exist. +A number here means digits, at most one `.`, and optionally an exponent — `e` or +`E`, an optional `+` or `-`, then at least one digit. So `-1`, `-2.5`, `-1e5`, and +`-1.5e-3` are values. It is deliberately narrower than what a float parser +accepts: `-inf` and `-NaN` parse as floats but are far likelier to be misspelled +flags than numbers somebody meant to pass. `-1x` and `-1e` are not numbers either, +and so name flags that do not exist. ## Reading a command line @@ -168,7 +170,7 @@ forward them at every level. Even when refusing, a lone `-` and a negative number stay values — neither is a misspelled flag, and without the second `--offset -1` could not be written. oclif made exactly this mistake when it switched to refusing unknown flags, and had to -add the number case back afterwards. +add the number case back afterward. ## Positional arguments