From 9985181318e2e3184cf65d7a0a2791231aefc4ef Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 04:33:34 +0000 Subject: [PATCH 1/8] feat(spec): split a value the way clap splits one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--tags a,b,c` as three values. The spec could record a clap *default* split by a delimiter — `lib/src/spec/arg.rs` has done that since the `--fs-events` fix — and had no way to say the same thing about a value someone types, so an adopter using `value_delimiter` lost behaviour rather than spelling. flag "--tags " var=#true delimiter="," Split during the parse, before anything judges what came out, which is the part that matters: `choices` is asked about each value rather than about the word that carried them, and `var_min`/`var_max` count the values the user meant rather than the words they typed. Judging first would reject `--env dev,prod` against a list that both halves are on. A delimiter needs somewhere to put what it splits, so it goes with `var`. On a single-value flag everything after the first separator would be dropped silently — refused where it is written instead, in all three places a spec can declare one, and a compile error in the derive, where the field has to be a `Vec`. The derive splits on the cold path, first in `check` and after the environment has filled what argv left out, since `TAGS=a,b` is one word too. The binder is untouched: it collects words and knows nothing about what a value *is*, which is what keeps the hot path out of this. The bridge carries it — `Arg::get_value_delimiter` is public, and it is the same getter the default splitting already used. Only where several values can land, because clap refuses `value_delimiter` with `num_args(1)` itself and a spec recording one there would be a spec this crate then declines to parse. Co-Authored-By: Claude Opus 5 --- argv/src/spec.rs | 7 +++ conformance/tests/post_binding.rs | 64 ++++++++++++++++++++++ derive/src/codegen.rs | 35 ++++++++++++ derive/src/lib.rs | 1 + derive/src/model.rs | 37 +++++++++++++ docs/spec/reference/flag.md | 15 ++++++ lib/src/parse.rs | 89 ++++++++++++++++++++++++++----- lib/src/spec/arg.rs | 86 +++++++++++++++++++++++++++++ lib/src/spec/cmd.rs | 16 +++++- lib/src/spec/flag.rs | 52 ++++++++++++++++++ lib/src/spec/mod.rs | 16 +++++- xtask/src/shadow.rs | 6 +++ 12 files changed, 409 insertions(+), 15 deletions(-) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index 770c8ccd6..ab1b2ccee 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -611,6 +611,9 @@ pub struct FlagMeta<'a> { /// flag win, this reports it: the combination has no meaning, so honouring one /// side silently would hide a mistake. pub conflicts: &'a [&'a str], + /// The character one word is split on to make several values, as clap's + /// `value_delimiter` does. Only ever set where several values can land. + pub delimiter: Option, /// Whether this flag must be given on its own. /// /// The whole-command form of [`conflicts`](Self::conflicts): everything the command @@ -653,6 +656,7 @@ impl FlagMeta<'_> { var_max: None, overrides: &[], conflicts: &[], + delimiter: None, exclusive: false, requires: &[], required_if: &[], @@ -1160,6 +1164,9 @@ fn write_flag(out: &mut String, meta: &FlagMeta<'_>, depth: usize) -> core::fmt: if meta.exclusive { out.push_str(" exclusive=#true"); } + if let Some(delimiter) = meta.delimiter { + write!(out, " delimiter={}", quoted(&delimiter.to_string()))?; + } write_single_list(out, "requires", meta.requires)?; write_single_list(out, "required_if", meta.required_if)?; write_single_list(out, "required_unless", meta.required_unless)?; diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 880a386ea..e5e49bf69 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -1113,3 +1113,67 @@ fn a_child_spelling_stays_exclusive_beside_an_ancestor_spelling() { let a = argv(["run", "-c", "--verbose"]); MixedAliasExclusive::parse_from(&a).expect("the ancestor's own spelling was never exclusive"); } + +/// A CLI whose values arrive several to a word. +#[derive(Cli)] +#[usage(bin = "ex3")] +struct Splitting { + /// Tags to apply + #[usage(long, delimiter = ',', var_max = 3)] + tags: Vec, + /// Where to look + #[usage(long, delimiter = ':', choices("src", "docs"))] + paths: Vec, +} + +#[test] +fn a_delimiter_makes_one_word_several_values() { + let a = argv(["--tags", "a,b,c"]); + assert_eq!( + Splitting::parse_from(&a).expect("split").tags, + ["a", "b", "c"] + ); + + // Several occurrences, each split. + let a = argv(["--tags", "a,b", "--tags", "c"]); + assert_eq!( + Splitting::parse_from(&a).expect("split").tags, + ["a", "b", "c"] + ); + + // A word with no separator in it is one value, as it was before. + let a = argv(["--tags", "a"]); + assert_eq!(Splitting::parse_from(&a).expect("split").tags, ["a"]); +} + +#[test] +fn split_values_are_judged_and_counted_as_values() { + // The split runs before every check, so `choices` sees each value rather than the + // word that carried them, and the bounds count what the user meant. + let a = argv(["--paths", "src:docs"]); + assert_eq!( + Splitting::parse_from(&a).expect("both are choices").paths, + ["src", "docs"] + ); + + let a = argv(["--paths", "src:nowhere"]); + assert!(matches!( + Splitting::parse_from(&a), + Err(Error::InvalidChoice { .. }) + )); + + let a = argv(["--tags", "a,b,c,d"]); + assert!(matches!( + Splitting::parse_from(&a), + Err(Error::VarTooMany { got: 4, .. }) + )); +} + +#[test] +fn a_delimiter_reaches_the_spec() { + let kdl = Splitting::to_kdl(); + assert!(kdl.contains(r#"delimiter=",""#), "{kdl}"); + let spec: usage::Spec = kdl.parse().expect("the emitted spec should parse"); + let tags = spec.cmd.flags.iter().find(|f| f.name == "tags").unwrap(); + assert_eq!(tags.arg.as_ref().unwrap().delimiter, Some(',')); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 99fb57e54..f7e946ae6 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -913,6 +913,10 @@ fn flag_meta(i: usize, field: &Field, owner: &syn::Ident) -> TokenStream { let conflicts = &field.conflicts; let requires = &field.requires; let exclusive = field.exclusive; + let delimiter = match field.delimiter { + Some(c) => quote!(::std::option::Option::Some(#c)), + None => quote!(::std::option::Option::None), + }; let required_if = &field.required_if; let required_unless = &field.required_unless; @@ -949,6 +953,7 @@ fn flag_meta(i: usize, field: &Field, owner: &syn::Ident) -> TokenStream { conflicts: &[#(#conflicts),*], requires: &[#(#requires),*], exclusive: #exclusive, + delimiter: #delimiter, required_if: &[#(#required_if),*], required_unless: &[#(#required_unless),*], ..usage_argv::spec::FlagMeta::EMPTY @@ -3376,6 +3381,32 @@ fn post_binding(cli: &Cli) -> TokenStream { let declared_defaults = declared_defaults(cli); let env_fallbacks = env_fallbacks(cli); + // One word becomes several values, before anything judges them. Run after the + // environment fills what argv left out — a `TAGS=a,b` is one word too — and before + // every check, so `choices` sees each value rather than the word that carried them, + // and `var_min`/`var_max` count what the user meant rather than what they typed. + // + // On the cold path deliberately: the binder collects words and knows nothing about + // what a value *is*, which is what keeps it free of everything in this function. + let delimiter_splits = cli.fields.iter().filter_map(|f| { + let delimiter = f.delimiter?; + let ident = &f.ident; + // A one-byte separator, which every delimiter anyone writes is. Refused at the + // attribute otherwise, rather than splitting on half a character. + let byte = u8::try_from(u32::from(delimiter)).ok()?; + Some(quote! { + if !partial.#ident.is_empty() { + let mut __usage_split: ::std::vec::Vec<::std::vec::Vec> = + ::std::vec::Vec::with_capacity(partial.#ident.len()); + for value in &partial.#ident { + for part in value.split(|b| *b == #byte) { + __usage_split.push(part.to_vec()); + } + } + partial.#ident = __usage_split; + } + }) + }); let required_checks = cli.fields.iter().filter_map(|f| { // A `String` has nowhere to put "absent", so the type is the declaration; a collection @@ -3831,6 +3862,10 @@ fn post_binding(cli: &Cli) -> TokenStream { // the order `start` used to give them. #declared_defaults #env_fallbacks + // Splitting before any check that counts or judges values, so `choices` sees a + // value rather than the word that carried several, and the bounds count what the + // user meant. After the environment, since `TAGS=a,b` is one word too. + #(#delimiter_splits)* let __usage_exclusive_present = #exclusive_present; #(#duplicate_checks)* // Before required-ness: "you gave two flags that cannot go together" is the diff --git a/derive/src/lib.rs b/derive/src/lib.rs index 1d925c273..7f0a27a09 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -215,6 +215,7 @@ //! | `requires = "--other"` | a flag that must also be given when this one is | //! | `group = "input"` | the group this flag is one of; see below | //! | `exclusive` | this flag has to be given on its own, positionals included | +//! | `delimiter = ','` | one word becomes several values; the field has to be a `Vec` | //! | `required_if = "--other"` | a flag whose presence makes this one necessary | //! | `required_unless = "--other"` | a flag whose presence makes this one unnecessary | //! diff --git a/derive/src/model.rs b/derive/src/model.rs index 013727c80..668d0c395 100644 --- a/derive/src/model.rs +++ b/derive/src/model.rs @@ -205,6 +205,12 @@ pub struct Field { /// one lives on the flag the rule is about, which is where clap puts it and where a /// reader looks for it. pub requires: Vec, + /// The character a value is split on, making one word several values. + /// + /// Only where several can land, so the field has to be a collection — checked at + /// compile time, since a delimiter on a single-value field would drop everything + /// after the first separator. + pub delimiter: Option, /// Whether this flag must be given on its own — clap's `exclusive`. pub exclusive: bool, /// The group this flag belongs to, if any. Properties live on the group's own @@ -854,6 +860,19 @@ impl Cli { } } + // A delimiter splits one word into several values, so the field has to be able to + // hold several. Anything else would drop everything after the first separator — + // silently, and only at run time, which is the worst way to find out. + for field in &self.fields { + if field.delimiter.is_some() && field.shape != Shape::Many { + return Err(syn::Error::new( + field.span, + "`delimiter` makes one word several values, so the field needs to be \ + a `Vec`", + )); + } + } + // Groups: every member is a flag, every declared group has members, and a group // holds at least two of them — the same floor the spec enforces, checked here so // it fails where it is written rather than when the spec is emitted. @@ -1055,6 +1074,7 @@ impl Field { overrides: Vec::new(), conflicts: Vec::new(), requires: Vec::new(), + delimiter: None, exclusive: false, group: None, required_if: Vec::new(), @@ -1151,6 +1171,7 @@ impl Field { overrides: Vec::new(), conflicts: Vec::new(), requires: Vec::new(), + delimiter: None, exclusive: false, group: None, required_if: Vec::new(), @@ -1212,6 +1233,7 @@ impl Field { let mut requires: Vec = Vec::new(); let mut group: Option = None; let mut exclusive = false; + let mut delimiter: Option = None; let mut required_if: Vec = Vec::new(); let mut required_unless: Vec = Vec::new(); @@ -1306,6 +1328,19 @@ impl Field { "requires" => requires = selectors(&meta)?, "group" => group = Some(string_value(&meta)?), "exclusive" => exclusive = flag_value(&meta)?, + "delimiter" => { + let c = char_value(&meta)?; + if !c.is_ascii() { + return Err(syn::Error::new_spanned( + &path, + format!( + "`delimiter = {c:?}` is more than one byte, and a \ + value is split by bytes; use an ASCII separator" + ), + )); + } + delimiter = Some(c); + } "required_if" => required_if = selectors(&meta)?, "required_unless" => required_unless = selectors(&meta)?, "value_enum" => value_enum = flag_value(&meta)?, @@ -1350,6 +1385,7 @@ impl Field { `count`, `hide`, `arg`, `env`, `default`, `choices`, \ `var_min`, `var_max`, `value_enum`, `value_hint`, `overrides`, \ `conflicts`, `requires`, `group`, `exclusive`, \ + `delimiter`, \ `required_if`, \ `required_unless`, `help_heading`, `value_name`, \ `verbatim_doc_comment`, \ @@ -1908,6 +1944,7 @@ impl Field { overrides, conflicts, requires, + delimiter, exclusive, group, required_if, diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index eb51fe8d4..b615951f3 100644 --- a/docs/spec/reference/flag.md +++ b/docs/spec/reference/flag.md @@ -33,6 +33,7 @@ flag "--file " overrides="--stdin" // --file and --stdin override each oth flag "--file " conflicts="--stdin" // --file and --stdin cannot be given together flag "--out " requires="--format" // giving --out means --format must be given too flag "--dump" exclusive=#true // --dump has to be given on its own +flag "--tags " var=#true delimiter="," // --tags a,b,c is three values flag "--stdin" { conflicts "--file" "--url" // several, one per argument @@ -115,6 +116,20 @@ Only what was supplied counts, the same rule `conflicts` follows. A flag with a anything, and counting it would make the exclusive flag unusable on any command that has a default. +## `delimiter` + +One word, several values: `--tags a,b,c` is three. clap spells this `value_delimiter`, +and a spec generated from a clap command carries it. + +The split happens during the parse, before anything judges what it produced, so +[`choices`](/spec/reference/arg) is asked about each value rather than about the word +that carried them, and `var_min`/`var_max` count the values the user meant rather than +the words they typed. + +A delimiter needs somewhere to put what it splits, so it goes with `var=#true` — on a +single-value flag everything after the first separator would be dropped, and that is +refused where it is written rather than at a prompt. + ## `global` A `global` flag is recognized by the command that declares it and by everything below it, so diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 17e2f12fc..d760928b2 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1164,7 +1164,14 @@ fn parse_partial_with_env( .or_insert_with(|| ParseValue::MultiString(vec![])) .try_as_multi_string_mut() .unwrap(); - arr.push(w); + // One word, several values, when the argument says so: `--tags a,b,c`. + // Split here rather than after the parse, so everything downstream — + // `var_max` stopping the collection, `choices`, `var_min` — counts the + // values the user meant rather than the words they typed. + match arg.delimiter { + Some(delimiter) => arr.extend(w.split(delimiter).map(str::to_string)), + None => arr.push(w), + } if arr.len() >= arg.var_max.unwrap_or(usize::MAX) { next_arg_idx += 1; } @@ -1948,28 +1955,45 @@ fn drain_pending_flag_values( ) -> miette::Result { while let Some(flag) = flag_awaiting_value.pop() { let arg = flag.arg.as_ref().unwrap(); - if validate_choices( - spec, - cmd, - errors, - ChoiceTarget::option(&flag), - word, - arg.choices.as_ref(), - custom_env, - )? { - return Ok(true); + // Split before anything judges the word, because after the split it is no longer + // one value: `--env dev,prod` is two, and `choices` has to be asked about each. + // Judging first would reject the whole word against a list neither half is on. + let parts: Vec = match arg.delimiter { + Some(delimiter) => word.split(delimiter).map(str::to_string).collect(), + None => vec![std::mem::take(word)], + }; + for part in &parts { + if validate_choices( + spec, + cmd, + errors, + ChoiceTarget::option(&flag), + part, + arg.choices.as_ref(), + custom_env, + )? { + return Ok(true); + } } - let value = std::mem::take(word); + word.clear(); + let value = parts.join(&arg.delimiter.map(String::from).unwrap_or_default()); // Two ways to hold several values, and both record a list: a `var` flag // collects one per occurrence, a variadic argument collects several from one. if flag.var || arg.var { + // Read before the flag is moved into the map, and it is only a `char`. + let delimiter = arg.delimiter; let arr = flags .entry(flag) .or_insert_with(|| ParseValue::MultiString(vec![])) .try_as_multi_string_mut() .unwrap(); - arr.push(value); + match delimiter { + Some(delimiter) => arr.extend(value.split(delimiter).map(str::to_string)), + None => arr.push(value), + } } else { + // Nowhere for a second value to go, so the word stands as it was typed. A + // delimiter on a flag that takes one value is refused where it is written. flags.insert(flag, ParseValue::String(value)); } } @@ -3070,6 +3094,45 @@ flag "--file " required_unless="--stdin" } } + #[test] + fn a_delimiter_turns_one_word_into_several_values() { + let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--tags \" var=#true delimiter=\",\"\narg \"[files]...\" var=#true delimiter=\":\"\n" + .parse() + .unwrap(); + + let parsed = parse(&spec, &input(&["ex", "--tags", "a,b,c", "x:y"])).unwrap(); + let multi = |value: &ParseValue| match value { + ParseValue::MultiString(values) => values.clone(), + other => panic!("expected several values, got {other:?}"), + }; + let tags = parsed + .flags + .iter() + .find(|(f, _)| f.name == "tags") + .map(|(_, v)| v) + .unwrap(); + assert_eq!(multi(tags), vec!["a", "b", "c"]); + assert_eq!(multi(parsed.args.values().next().unwrap()), vec!["x", "y"]); + } + + #[test] + fn a_split_value_is_counted_and_judged_as_values() { + // Split during the parse rather than after it, so everything downstream sees the + // values the user meant rather than the words they typed: `choices` judges each + // one, and the bounds count them. + let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--env \" var=#true delimiter=\",\" var_max=2 {\n choices \"dev\" \"prod\"\n}\n" + .parse() + .unwrap(); + + parse(&spec, &input(&["ex", "--env", "dev,prod"])).expect("two values, both allowed"); + let err = parse(&spec, &input(&["ex", "--env", "dev,staging"])).unwrap_err(); + assert!(err.to_string().contains("staging"), "{err}"); + assert!( + parse(&spec, &input(&["ex", "--env", "dev,prod,dev"])).is_err(), + "three values should breach var_max=2" + ); + } + #[test] fn an_exclusive_flag_has_to_be_alone() { let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--dump\" exclusive=#true\nflag \"--verbose\"\narg \"[target]\"\n" diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index 8c8375a18..75add44c2 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -74,6 +74,14 @@ pub struct SpecArg { /// Maximum number of values for variadic arguments #[serde(skip_serializing_if = "Option::is_none")] pub var_max: Option, + /// The character a single word is split on to produce several values. + /// + /// `--tags a,b,c` as three values rather than one, which is clap's + /// `value_delimiter`. Only meaningful where several values can land, so it goes with + /// [`SpecArg::var`]; declaring it anywhere else is refused rather than silently + /// dropping everything after the first separator. + #[serde(skip_serializing_if = "Option::is_none")] + pub delimiter: Option, /// Whether to hide this argument from help output pub hide: bool, /// Default value(s) if the argument is not provided @@ -112,6 +120,18 @@ impl SpecArg { "required" => arg.required = v.ensure_bool()?, "double_dash" => arg.double_dash = v.ensure_string()?.parse()?, "var" => arg.var = v.ensure_bool()?, + "delimiter" => { + let raw = v.ensure_string()?; + let mut chars = raw.chars(); + match (chars.next(), chars.next()) { + (Some(c), None) => arg.delimiter = Some(c), + _ => bail_parse!( + ctx, + v.entry.span(), + "a delimiter is one character, and {raw:?} is not" + ), + } + } "hide" => arg.hide = v.ensure_bool()?, "var_min" => arg.var_min = v.ensure_usize().map(Some)?, "var_max" => arg.var_max = v.ensure_usize().map(Some)?, @@ -241,6 +261,9 @@ impl From<&SpecArg> for KdlNode { if let Some(max) = arg.var_max { node.push(KdlEntry::new_prop("var_max", max as i128)); } + if let Some(delimiter) = arg.delimiter { + node.push(string_entry(Some("delimiter"), &delimiter.to_string())); + } if arg.hide { node.push(KdlEntry::new_prop("hide", true)); } @@ -400,6 +423,9 @@ impl From<&clap::Arg> for SpecArg { var, var_max: None, var_min: None, + // clap answers for this one, and the same getter `default_values` already + // uses just above: a default is split by it, and so is a typed value. + delimiter: arg.get_value_delimiter(), hide, default: default_values(arg), choices: None, @@ -435,6 +461,66 @@ impl Hash for SpecArg { } } +#[cfg(test)] +mod delimiter_tests { + use crate::Spec; + + #[test] + fn a_delimiter_round_trips_and_comes_across_from_clap() { + let spec: Spec = "flag \"--tags \" var=#true delimiter=\",\"\n" + .parse() + .unwrap(); + let arg = spec.cmd.flags[0].arg.as_ref().unwrap(); + assert_eq!(arg.delimiter, Some(',')); + + let reparsed: Spec = spec.to_string().parse().unwrap(); + let arg = reparsed.cmd.flags[0].arg.as_ref().unwrap(); + assert_eq!(arg.delimiter, Some(','), "{spec}"); + + // clap answers for this one, through the same getter the default splitting + // already used. + let cmd = clap::Command::new("ex").arg( + clap::Arg::new("tags") + .long("tags") + .value_delimiter(',') + .num_args(1..) + .default_value("a,b"), + ); + let spec = Spec::from(&cmd); + let flag = &spec.cmd.flags[0]; + assert_eq!(flag.arg.as_ref().unwrap().delimiter, Some(',')); + // And the default is still recorded split, which is the same statement. On the + // flag rather than on its argument, which is where the bridge puts a flag's. + assert_eq!(flag.default, vec!["a", "b"]); + } + + #[test] + fn a_delimiter_needs_somewhere_to_put_what_it_splits() { + // Without `var` everything after the first separator would be dropped, silently. + let err = "flag \"--tags \" delimiter=\",\"\n" + .parse::() + .unwrap_err(); + assert!(format!("{err:?}").contains("one value"), "{err:?}"); + + let err = "arg \"[tags]\" delimiter=\",\"\n" + .parse::() + .unwrap_err(); + assert!(format!("{err:?}").contains("one value"), "{err:?}"); + + // A flag that takes no value has nothing to split at all. + let err = "flag \"--quiet\" delimiter=\",\"\n" + .parse::() + .unwrap_err(); + assert!(format!("{err:?}").contains("takes none"), "{err:?}"); + + // One character, or it is not a delimiter. + let err = "flag \"--tags \" var=#true delimiter=\"::\"\n" + .parse::() + .unwrap_err(); + assert!(format!("{err:?}").contains("one character"), "{err:?}"); + } +} + #[cfg(test)] mod tests { use crate::Spec; diff --git a/lib/src/spec/cmd.rs b/lib/src/spec/cmd.rs index 4ef200b96..86ab21e96 100644 --- a/lib/src/spec/cmd.rs +++ b/lib/src/spec/cmd.rs @@ -305,7 +305,21 @@ impl SpecCommand { for child in node.children() { match child.name() { "flag" => cmd.flags.push(SpecFlag::parse(ctx, &child)?), - "arg" => cmd.args.push(SpecArg::parse(ctx, &child)?), + "arg" => { + let arg = SpecArg::parse(ctx, &child)?; + // As on a flag: splitting a word that has room for one value would + // drop everything after the first separator. + if arg.delimiter.is_some() && !arg.var { + bail_parse!( + ctx, + child.node.name().span(), + "argument <{}> has a delimiter and holds one value; add \ + `var=#true` for the values it splits into", + arg.name + ); + } + cmd.args.push(arg); + } "mount" => cmd.mounts.push(SpecMount::parse(ctx, &child)?), "group" => cmd.groups.push(SpecGroup::parse(ctx, &child)?), "cmd" => { diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index 479860776..d5eefbf65 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -150,6 +150,7 @@ impl SpecFlag { pub(crate) fn parse(ctx: &ParsingContext, node: &NodeHelper) -> Result { let mut flag: Self = node.arg(0)?.ensure_string()?.parse()?; let mut allow_hyphen_values = false; + let mut delimiter: Option = None; for (k, v) in node.props() { match k { "help" => flag.help = Some(v.ensure_string()?), @@ -186,6 +187,10 @@ impl SpecFlag { "conflicts" => flag.conflicts = vec![v.ensure_string()?], "requires" => flag.requires = vec![v.ensure_string()?], "exclusive" => flag.exclusive = v.ensure_bool()?, + // Written on the flag and kept on its argument, as `allow_hyphen_values` + // is: the value is what gets split, and `flag "--tags "` is where a + // reader writes something about that value. + "delimiter" => delimiter = Some(v.ensure_string()?), "effect" => { let raw = v.ensure_string()?; match raw.parse() { @@ -318,6 +323,41 @@ impl SpecFlag { if allow_hyphen_values { flag.set_allow_hyphen_values(ctx, node.node.name().span(), true)?; } + if let Some(raw) = delimiter { + let mut chars = raw.chars(); + let Some(delimiter) = chars.next().filter(|_| chars.next().is_none()) else { + bail_parse!( + ctx, + node.node.name().span(), + "a delimiter is one character, and {raw:?} is not" + ); + }; + let Some(arg) = flag.arg.as_mut() else { + bail_parse!( + ctx, + node.node.name().span(), + "`delimiter` splits a value, and flag --{} takes none", + flag.name + ); + }; + arg.delimiter = Some(delimiter); + } + // A delimiter with nowhere to put the extra values would drop everything after + // the first separator, silently. Refused where it is written instead — and `var` + // on either the flag or its argument is somewhere for them to go, since both are + // ways of saying the flag holds a list. + if flag.arg.as_ref().is_some_and(|a| a.delimiter.is_some()) && !flag.var { + let takes_several = flag.arg.as_ref().is_some_and(|a| a.var); + if !takes_several { + bail_parse!( + ctx, + node.node.name().span(), + "flag --{} has a delimiter and holds one value; add `var=#true` for \ + the values it splits into", + flag.name + ); + } + } flag.usage = flag.usage(); flag.help_first_line = flag.help.as_ref().map(|s| string::first_line(s)); Ok(flag) @@ -609,6 +649,18 @@ impl From<&clap::Arg> for SpecFlag { }); } + // The flag's argument is built from its value name rather than from the + // clap `Arg`, so what the `Arg` says about the *value* has to be carried + // here — the `From<&clap::Arg> for SpecArg` impl never sees this one. + // + // Only where several values can land. clap refuses `value_delimiter` with + // `num_args(1)` itself, and a spec that recorded one on a single-value + // argument would be a spec this crate then declines to parse. + if var || c.get_num_args().is_some_and(|n| n.max_values() > 1) { + arg.var = true; + arg.delimiter = c.get_value_delimiter(); + } + Some(arg) } else { None diff --git a/lib/src/spec/mod.rs b/lib/src/spec/mod.rs index 14af4300b..87c7d9c6f 100644 --- a/lib/src/spec/mod.rs +++ b/lib/src/spec/mod.rs @@ -223,7 +223,21 @@ impl Spec { schema.after_help_long = Some(node.arg(0)?.ensure_string()?) } "usage" => schema.usage = node.arg(0)?.ensure_string()?, - "arg" => schema.cmd.args.push(SpecArg::parse(ctx, &node)?), + "arg" => { + let arg = SpecArg::parse(ctx, &node)?; + // The same rule the `cmd` block applies: a delimiter with nowhere to + // put what it splits drops everything after the first separator. + if arg.delimiter.is_some() && !arg.var { + bail_parse!( + ctx, + node.node.name().span(), + "argument <{}> has a delimiter and holds one value; add \ + `var=#true` for the values it splits into", + arg.name + ); + } + schema.cmd.args.push(arg); + } "flag" => schema.cmd.flags.push(SpecFlag::parse(ctx, &node)?), // The root command's groups, as its flags and arguments are: a spec // whose top level declares flags can group them there too. diff --git a/xtask/src/shadow.rs b/xtask/src/shadow.rs index c62232da0..43dec61fb 100644 --- a/xtask/src/shadow.rs +++ b/xtask/src/shadow.rs @@ -845,6 +845,9 @@ fn usage_flag_opts( if flag.exclusive { opts.push("exclusive = true".into()); } + if let Some(delimiter) = flag.arg.as_ref().and_then(|a| a.delimiter) { + opts.push(format!("delimiter = {delimiter:?}")); + } if !flag.required_if.is_empty() { opts.push(selector_list("required_if", &flag.required_if)); } @@ -963,6 +966,9 @@ fn clap_flag_opts( if flag.exclusive { opts.push("exclusive = true".into()); } + if let Some(delimiter) = flag.arg.as_ref().and_then(|a| a.delimiter) { + opts.push(format!("value_delimiter = {delimiter:?}")); + } if flag.global { opts.push("global = true".into()); } From 7d0c1235fc353c9692fa203b18d19c8e0adb7452 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 04:51:05 +0000 Subject: [PATCH 2/8] fix(spec): split a positional before judging it, and keep clap's delimiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from review, both of them the same mistake in two places: treating a word that carries several values as one value. **A positional judged the whole word.** The flag path split first and the positional path did not, so `src:docs` was rejected against a list that both halves are on, and a bad half was reported as the whole word. Split first now, as the flag path does, and the collection below reuses what the split produced rather than doing it again. **The bridge dropped a delimiter clap would have used.** It was carried only for `Append`/`Count` or a `num_args` above one, on the reasoning that clap refuses a delimiter with `num_args(1)`. clap does no such thing: its parser reaches for `arg.get_value_delimiter()` before it looks at anything else, so `ArgAction::Set` with `value_delimiter(',')` is one word becoming several — and that is the common spelling. The generated spec left a CLI whose defaults split and whose typed values did not. A delimiter *is* the statement that several values can land, so it brings `var` with it rather than waiting for one, which also keeps the emitted spec parseable by the rule that a delimiter needs somewhere to put what it splits. Co-Authored-By: Claude Opus 5 --- lib/src/parse.rs | 65 ++++++++++++++++++++++++++++++++------------ lib/src/spec/arg.rs | 21 ++++++++++++++ lib/src/spec/flag.rs | 19 +++++++++---- 3 files changed, 83 insertions(+), 22 deletions(-) diff --git a/lib/src/parse.rs b/lib/src/parse.rs index d760928b2..053fccd9e 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1138,15 +1138,30 @@ fn parse_partial_with_env( // in an error rather than in `unexpected word`. continue; } - if validate_choices( - spec, - &out.cmd, - &mut out.errors, - ChoiceTarget::arg(arg), - &w, - arg.choices.as_ref(), - custom_env, - )? { + // Split before judging, as the flag path does: after the split the word is + // no longer one value, and `choices` has to be asked about each. Judging + // first rejects `src:docs` against a list that both halves are on, and + // names the whole word rather than the half that was wrong. + let parts: Vec = match arg.delimiter { + Some(delimiter) => w.split(delimiter).map(str::to_string).collect(), + None => vec![w.clone()], + }; + let mut refused = false; + for part in &parts { + if validate_choices( + spec, + &out.cmd, + &mut out.errors, + ChoiceTarget::arg(arg), + part, + arg.choices.as_ref(), + custom_env, + )? { + refused = true; + break; + } + } + if refused { record_cursor(&mut out, next_arg_idx, seen_double_dash); return Ok((out, overridden_flags)); } @@ -1164,14 +1179,10 @@ fn parse_partial_with_env( .or_insert_with(|| ParseValue::MultiString(vec![])) .try_as_multi_string_mut() .unwrap(); - // One word, several values, when the argument says so: `--tags a,b,c`. - // Split here rather than after the parse, so everything downstream — - // `var_max` stopping the collection, `choices`, `var_min` — counts the - // values the user meant rather than the words they typed. - match arg.delimiter { - Some(delimiter) => arr.extend(w.split(delimiter).map(str::to_string)), - None => arr.push(w), - } + // The values this word carried, split above so that everything + // downstream — `choices`, `var_max` stopping the collection, `var_min` — + // counts the values the user meant rather than the words they typed. + arr.extend(parts.iter().cloned()); if arr.len() >= arg.var_max.unwrap_or(usize::MAX) { next_arg_idx += 1; } @@ -3115,6 +3126,26 @@ flag "--file " required_unless="--stdin" assert_eq!(multi(parsed.args.values().next().unwrap()), vec!["x", "y"]); } + #[test] + fn a_positional_splits_before_its_choices_are_asked() { + // The flag path did this and the positional path did not, so a word whose parts + // were all choices was rejected as one value, and a bad half was reported as the + // whole word. + let spec: Spec = "name \"ex\"\nbin \"ex\"\narg \"[paths]...\" var=#true delimiter=\":\" {\n choices \"src\" \"docs\"\n}\n" + .parse() + .unwrap(); + + parse(&spec, &input(&["ex", "src:docs"])).expect("both halves are choices"); + + let err = parse(&spec, &input(&["ex", "src:nowhere"])).unwrap_err(); + let message = err.to_string(); + assert!(message.contains("nowhere"), "{message}"); + assert!( + !message.contains("src:nowhere"), + "the bad half should be named, not the whole word: {message}" + ); + } + #[test] fn a_split_value_is_counted_and_judged_as_values() { // Split during the parse rather than after it, so everything downstream sees the diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index 75add44c2..e84b251b0 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -494,6 +494,27 @@ mod delimiter_tests { assert_eq!(flag.default, vec!["a", "b"]); } + #[test] + fn a_single_valued_clap_arg_keeps_its_delimiter() { + // clap's parser splits whenever a delimiter is set, whatever `num_args` says, so + // `ArgAction::Set` with `value_delimiter(',')` is one word becoming several — the + // common spelling. Reading it as single-valued dropped the delimiter and left a + // CLI whose defaults split and whose typed values did not. + let cmd = clap::Command::new("ex").arg( + clap::Arg::new("tags") + .long("tags") + .action(clap::ArgAction::Set) + .value_delimiter(','), + ); + let spec = Spec::from(&cmd); + let arg = spec.cmd.flags[0].arg.as_ref().unwrap(); + assert_eq!(arg.delimiter, Some(',')); + // And it says so: a delimiter is the statement that several values can land, so + // the emitted spec has somewhere to put them and parses back. + assert!(arg.var, "a delimiter brings `var` with it"); + let _: Spec = spec.to_string().parse().expect("{spec}"); + } + #[test] fn a_delimiter_needs_somewhere_to_put_what_it_splits() { // Without `var` everything after the first separator would be dropped, silently. diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index d5eefbf65..ef5780af8 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -653,12 +653,21 @@ impl From<&clap::Arg> for SpecFlag { // clap `Arg`, so what the `Arg` says about the *value* has to be carried // here — the `From<&clap::Arg> for SpecArg` impl never sees this one. // - // Only where several values can land. clap refuses `value_delimiter` with - // `num_args(1)` itself, and a spec that recorded one on a single-value - // argument would be a spec this crate then declines to parse. - if var || c.get_num_args().is_some_and(|n| n.max_values() > 1) { + // A delimiter *is* the statement that several values can land, so it brings + // `var` with it rather than waiting for one. + // + // Gating this on the action or on `num_args` was wrong: clap's parser splits + // whenever a delimiter is set — `parser.rs` reaches for + // `arg.get_value_delimiter()` before it looks at anything else — so + // `ArgAction::Set` with `value_delimiter(',')` is one word becoming several, + // and that is the common spelling. Reading it as single-valued dropped the + // delimiter and left a CLI whose defaults split and whose typed values did + // not. + if let Some(delimiter) = c.get_value_delimiter() { + arg.var = true; + arg.delimiter = Some(delimiter); + } else if var || c.get_num_args().is_some_and(|n| n.max_values() > 1) { arg.var = true; - arg.delimiter = c.get_value_delimiter(); } Some(arg) From 2d186adf1b613731dc2a79a025d97ff35fc60f18 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:24:45 +0000 Subject: [PATCH 3/8] fix(spec): split clap positional delimiters --- lib/src/spec/arg.rs | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index e84b251b0..e60e31ece 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -390,10 +390,11 @@ impl From<&clap::Arg> for SpecArg { let help_long = arg.get_long_help().map(|s| s.to_string()); let help_first_line = help.as_ref().map(|s| string::first_line(s)); let hide = arg.is_hide_set(); + let delimiter = arg.get_value_delimiter(); let var = matches!( arg.get_action(), clap::ArgAction::Count | clap::ArgAction::Append - ); + ) || delimiter.is_some(); let choices = arg .get_possible_values() .iter() @@ -425,7 +426,7 @@ impl From<&clap::Arg> for SpecArg { var_min: None, // clap answers for this one, and the same getter `default_values` already // uses just above: a default is split by it, and so is a typed value. - delimiter: arg.get_value_delimiter(), + delimiter, hide, default: default_values(arg), choices: None, @@ -515,6 +516,36 @@ mod delimiter_tests { let _: Spec = spec.to_string().parse().expect("{spec}"); } + #[test] + fn a_single_valued_clap_positional_splits_into_stored_values() { + // The positional bridge uses `SpecArg::from(&clap::Arg)` directly, unlike a + // flag. A delimiter therefore has to make that argument variadic here too or + // parsing validates the split parts and then stores the original unsplit word. + let cmd = clap::Command::new("ex").arg( + clap::Arg::new("tags") + .action(clap::ArgAction::Set) + .value_delimiter(',') + .value_parser(["a", "b"]), + ); + let spec = Spec::from(&cmd); + let arg = &spec.cmd.args[0]; + assert!(arg.var, "a positional delimiter brings `var` with it"); + assert_eq!(arg.delimiter, Some(',')); + + let input = ["ex", "a,b"].map(str::to_string); + let parsed = crate::parse(&spec, &input).expect("both split values are choices"); + let value = parsed + .args + .values() + .next() + .expect("the positional was stored"); + assert!(matches!( + value, + crate::parse::ParseValue::MultiString(values) + if values == &["a".to_string(), "b".to_string()] + )); + } + #[test] fn a_delimiter_needs_somewhere_to_put_what_it_splits() { // Without `var` everything after the first separator would be dropped, silently. From f934a55a6919aba257addff2994bafe549615ebc Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:22:09 +0000 Subject: [PATCH 4/8] refactor(parse): reuse split flag values --- lib/src/parse.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 053fccd9e..3c0a2af34 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1987,25 +1987,22 @@ fn drain_pending_flag_values( } } word.clear(); - let value = parts.join(&arg.delimiter.map(String::from).unwrap_or_default()); // Two ways to hold several values, and both record a list: a `var` flag // collects one per occurrence, a variadic argument collects several from one. if flag.var || arg.var { - // Read before the flag is moved into the map, and it is only a `char`. - let delimiter = arg.delimiter; let arr = flags .entry(flag) .or_insert_with(|| ParseValue::MultiString(vec![])) .try_as_multi_string_mut() .unwrap(); - match delimiter { - Some(delimiter) => arr.extend(value.split(delimiter).map(str::to_string)), - None => arr.push(value), - } + arr.extend(parts); } else { // Nowhere for a second value to go, so the word stands as it was typed. A // delimiter on a flag that takes one value is refused where it is written. - flags.insert(flag, ParseValue::String(value)); + flags.insert( + flag, + ParseValue::String(parts.into_iter().next().unwrap_or_default()), + ); } } Ok(false) From b69e49a236eb44cb87e270da323129f0a7b9b572 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:29:37 +0000 Subject: [PATCH 5/8] fix(spec): emit positional delimiters --- argv/src/spec.rs | 6 ++++++ conformance/tests/post_binding.rs | 10 ++++++++++ derive/src/codegen.rs | 5 +++++ 3 files changed, 21 insertions(+) diff --git a/argv/src/spec.rs b/argv/src/spec.rs index ab1b2ccee..b4e5cab21 100644 --- a/argv/src/spec.rs +++ b/argv/src/spec.rs @@ -682,6 +682,8 @@ pub struct ArgMeta<'a> { pub hide: bool, pub var_min: Option, pub var_max: Option, + /// The character one word is split on to make several positional values. + pub delimiter: Option, /// Heading to list this argument under in help output. pub help_heading: Option<&'a str>, /// What answers for this argument when a shell asks. See [`FlagMeta::complete`]. @@ -705,6 +707,7 @@ impl ArgMeta<'_> { hide: false, var_min: None, var_max: None, + delimiter: None, help_heading: None, }; } @@ -1247,6 +1250,9 @@ fn write_arg(out: &mut String, meta: &ArgMeta<'_>, depth: usize) -> core::fmt::R if let Some(max) = meta.var_max { write!(out, " var_max={max}")?; } + if let Some(delimiter) = meta.delimiter { + write!(out, " delimiter={}", quoted(&delimiter.to_string()))?; + } if meta.arg.double_dash != DoubleDash::Optional { let mode = match meta.arg.double_dash { DoubleDash::Required => "required", diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index e5e49bf69..94bf59457 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -1124,6 +1124,9 @@ struct Splitting { /// Where to look #[usage(long, delimiter = ':', choices("src", "docs"))] paths: Vec, + /// Labels to attach + #[usage(arg, delimiter = ';')] + labels: Vec, } #[test] @@ -1144,6 +1147,12 @@ fn a_delimiter_makes_one_word_several_values() { // A word with no separator in it is one value, as it was before. let a = argv(["--tags", "a"]); assert_eq!(Splitting::parse_from(&a).expect("split").tags, ["a"]); + + let a = argv(["one;two"]); + assert_eq!( + Splitting::parse_from(&a).expect("split positional").labels, + ["one", "two"] + ); } #[test] @@ -1176,4 +1185,5 @@ fn a_delimiter_reaches_the_spec() { let spec: usage::Spec = kdl.parse().expect("the emitted spec should parse"); let tags = spec.cmd.flags.iter().find(|f| f.name == "tags").unwrap(); assert_eq!(tags.arg.as_ref().unwrap().delimiter, Some(',')); + assert_eq!(spec.cmd.args[0].delimiter, Some(';')); } diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index f7e946ae6..4ee496ad0 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -978,6 +978,10 @@ fn arg_meta(i: usize, field: &Field, owner: &syn::Ident) -> TokenStream { let required = field.shape == Shape::Required || field.required_collection; let choices = choices_tokens(field); let (var_min, var_max) = bounds_tokens(field); + let delimiter = match field.delimiter { + Some(c) => quote!(::std::option::Option::Some(#c)), + None => quote!(::std::option::Option::None), + }; let (completer_decl, completer) = completer_tokens(i, field, "arg", owner); quote! { @@ -996,6 +1000,7 @@ fn arg_meta(i: usize, field: &Field, owner: &syn::Ident) -> TokenStream { choices: #choices, var_min: #var_min, var_max: #var_max, + delimiter: #delimiter, ..usage_argv::spec::ArgMeta::EMPTY }; } From f403c84decbaf31d4483ec1a32362cffc72e8e98 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 18:29:07 +0000 Subject: [PATCH 6/8] fix(argv): a bound counts values, so binding has to know the delimiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `var_max` is enforced by binding: a variadic stops on its bound and the next field takes the rest, so before delimiters a collection could reach the bound but never pass it. A delimiter breaks that assumption — one word is no longer one value, and `--include a,b,c` is already three on the single word the collection was entitled to take. Binding counted words, so the bound was silently exceeded, for positionals and for variadic flags alike. The parser tables gain the delimiter, beside the `var_max` they already carry and for the same stated reason: it decides *where* a word lands. Binding counts the values a word holds rather than the word, and a word cannot be split between two owners, so passing the bound is an error where reaching it is just a stopping place. usage-lib already counted values but only ever stopped, so it gets the same report at the end of an occurrence's run. Both keep the rule the corpus documents: the bound is on what one occurrence takes, not on the list the occurrences build up, so `--include a,b --include c,d` is two twice and not four. No corpus vector for this yet, deliberately. The corpus is what every implementation answers, and the Go parser has no delimiter support at all — a vector here would fail it for want of the feature rather than for want of this rule. It belongs with whichever change teaches Go to split. The rule is held across the derive and usage-lib by the conformance test in the meantime, which is the agreement this change is about. The derive path is unchanged in cost — the shadow benchmark is bit-identical at 63,822 instructions, mise declaring no delimiters for `values_in` to count. Co-Authored-By: Claude Opus 5 --- argv/src/lib.rs | 76 +++++++++++++++++++++++++++---- conformance/src/argv.rs | 6 +++ conformance/src/tables.rs | 11 +++++ conformance/tests/post_binding.rs | 59 ++++++++++++++++++++++++ derive/src/codegen.rs | 18 ++++++++ lib/src/parse.rs | 44 ++++++++++++++++++ 6 files changed, 205 insertions(+), 9 deletions(-) diff --git a/argv/src/lib.rs b/argv/src/lib.rs index 277fa1d54..6b1d949ec 100644 --- a/argv/src/lib.rs +++ b/argv/src/lib.rs @@ -255,6 +255,14 @@ pub struct Flag<'a> { /// `var=#true` — is bounded on how many times it was *given*, which no single token /// can decide, so that bound stays with the metadata and is checked after the parse. pub var_max: ::core::option::Option, + /// The byte that makes one word several values, if the flag declares one. + /// + /// Here rather than with the metadata for the same reason [`var_max`](Self::var_max) + /// is: it decides *where* a word lands. A bound counts values, and a delimiter is what + /// makes a word stop being one of them — `--include a,b,c` is three, so a `var_max` of + /// two is already past its bound on the single word it was entitled to take. Binding + /// cannot count without it. + pub delimiter: ::core::option::Option, /// Whether the flag is recognized by every command beneath the one that /// declares it. pub global: bool, @@ -271,6 +279,7 @@ impl Flag<'_> { takes_value: false, variadic: false, var_max: ::core::option::Option::None, + delimiter: ::core::option::Option::None, global: false, }; @@ -297,6 +306,11 @@ pub struct Arg<'a> { /// command. `u32` rather than `usize` because a CLI that bounds a variadic above four /// billion has other problems, and this table is read on the hot path. pub var_max: ::core::option::Option, + /// The byte that makes one word several values, if the argument declares one. + /// + /// See [`Flag::delimiter`]: a bound counts values, and only this says how many values a + /// word carries. + pub delimiter: ::core::option::Option, /// This argument's relationship to the `--` separator. pub double_dash: DoubleDash, /// Unused by binding, kept so a table entry can carry its own name for @@ -310,6 +324,7 @@ impl Arg<'_> { key: 0, var: false, var_max: ::core::option::Option::None, + delimiter: ::core::option::Option::None, double_dash: DoubleDash::Optional, name: "", }; @@ -1119,12 +1134,23 @@ impl<'t, 'v> Parser<'t, 'v> { match self.argv.get(self.pos) { Some(next) if !is_flag_like(bytes(next)) && bytes(next) != b"--" => { self.pos += 1; - self.collected += 1; + self.collected += values_in(bytes(next), flag.delimiter); // Same rule as a positional: a bounded occurrence takes that many and // leaves the rest to whatever follows. if flag.var_max.is_some_and(|max| self.collected >= max) { self.collecting = None; } + // Stopping is only the same as staying within the bound while one word + // is one value. A delimited word can carry the occurrence past it in a + // single step, and that word cannot be split between two owners, so the + // overshoot is an error rather than a place to stop. + if let Some(max) = flag.var_max.filter(|max| self.collected > *max) { + return Some(Err(Error::VarTooMany { + name: flag.name, + max: max as usize, + got: self.collected as usize, + })); + } return Some(Ok(Event::Flag { flag, value: Some(bytes(next)), @@ -1215,7 +1241,7 @@ impl<'t, 'v> Parser<'t, 'v> { None }; if flag.variadic { - self.start_collecting(flag); + self.start_collecting(flag, value.unwrap_or(b""))?; } return Ok(Event::Flag { flag, @@ -1310,7 +1336,7 @@ impl<'t, 'v> Parser<'t, 'v> { rest }; if flag.variadic { - self.start_collecting(flag); + self.start_collecting(flag, value)?; } Ok(Event::Flag { flag, @@ -1420,7 +1446,17 @@ impl<'t, 'v> Parser<'t, 'v> { // bound, at which point the words after it belong to whatever comes next. That is // what makes `[a]… [b]` expressible at all. if arg.var { - self.arg_taken += 1; + self.arg_taken += values_in(token, arg.delimiter); + // Before advancing, which resets the count: as with a variadic flag, reaching + // the bound and passing it are the same event once a word can carry several + // values, and only the second is a mistake. + if let Some(max) = arg.var_max.filter(|max| self.arg_taken > *max) { + return Err(Error::VarTooMany { + name: arg.name, + max: max as usize, + got: self.arg_taken as usize, + }); + } if arg.var_max.is_some_and(|max| self.arg_taken >= max) { self.advance_arg(); } @@ -1459,15 +1495,24 @@ impl<'t, 'v> Parser<'t, 'v> { /// A variadic flag occurrence begins, counting from zero. /// - /// The value it was given on the same token counts, which is why this starts at one: - /// `--include a b` with `var_max=2` takes `a` and `b`, not three words. - fn start_collecting(&mut self, flag: &'t Flag<'t>) { - self.collected = 1; - self.collecting = if flag.var_max.is_some_and(|max| max <= 1) { + /// The value it was given on the same token counts, which is why this starts at what + /// that value holds: `--include a b` with `var_max=2` takes `a` and `b`, not three + /// words — and `--include a,b` has already taken both on the one token. + fn start_collecting(&mut self, flag: &'t Flag<'t>, first: &[u8]) -> Result<(), Error<'t, 'v>> { + self.collected = values_in(first, flag.delimiter); + if let Some(max) = flag.var_max.filter(|max| self.collected > *max) { + return Err(Error::VarTooMany { + name: flag.name, + max: max as usize, + got: self.collected as usize, + }); + } + self.collecting = if flag.var_max.is_some_and(|max| self.collected >= max) { None } else { Some(flag) }; + Ok(()) } fn next_arg(&self) -> Option<&'t Arg<'t>> { @@ -1530,6 +1575,19 @@ fn bytes<'v>(s: &'v &'v OsStr) -> &'v [u8] { s.as_encoded_bytes() } +/// How many values one word carries. +/// +/// One, until a delimiter is declared — and then one per separator, counting the same way +/// splitting on it does: `a,b` is two, `a,` is two with an empty second, and `` is one. +/// Counted rather than split because binding only needs the number, and the split itself +/// belongs to the layer that owns the values. +fn values_in(word: &[u8], delimiter: ::core::option::Option) -> u32 { + match delimiter { + Some(d) => 1 + word.iter().filter(|b| **b == d).count() as u32, + None => 1, + } +} + /// Whether a token should be read as a flag. /// /// `-` alone is a value, conventionally stdin. A negative number is a value too, diff --git a/conformance/src/argv.rs b/conformance/src/argv.rs index 5cf57190f..ddccf31c5 100644 --- a/conformance/src/argv.rs +++ b/conformance/src/argv.rs @@ -167,6 +167,12 @@ fn code(err: Error<'_, '_>) -> ErrorCode { Error::MissingFlagValue { .. } => ErrorCode::MissingFlagValue, Error::UnexpectedArg { .. } => ErrorCode::UnexpectedArg, Error::ArgRequiresDoubleDash { .. } => ErrorCode::ArgRequiresDoubleDash, + // Binding does raise this one, and only this one of the bounds: a `var_max` stops a + // collection rather than judging it, so it can only be *exceeded* when a delimiter + // makes one word several values — which is a question about where a word lands, and + // so the parser's. `var_min` remains the layer above's, having nothing to do with + // where anything landed. + Error::VarTooMany { .. } => ErrorCode::VarTooMany, Error::TooDeep => panic!("no corpus spec is anywhere near MAX_DEPTH"), // The parser cannot raise these — they come from the layer above it, which // this harness does not exercise: it builds tables from a spec rather than diff --git a/conformance/src/tables.rs b/conformance/src/tables.rs index ad532c40b..35ef248a2 100644 --- a/conformance/src/tables.rs +++ b/conformance/src/tables.rs @@ -258,6 +258,16 @@ fn build_flag(f: &SpecFlag) -> &'static Flag<'static> { // Saturating rather than truncating: `4294967296 as u32` is zero, which would read // as "stop at once" rather than "no real limit". .map(|max| u32::try_from(max).unwrap_or(u32::MAX)), + // A bound counts values, and this is what says how many a word carries. ASCII, not + // "fits in a byte": `§` is one byte as a scalar and two as UTF-8, and matching its + // low byte would find the continuation bytes inside unrelated characters. The spec + // refuses non-ASCII, so this only ever discards something already rejected. + delimiter: f + .arg + .as_ref() + .and_then(|a| a.delimiter) + .filter(char::is_ascii) + .map(|d| d as u8), global: f.global, })) } @@ -271,6 +281,7 @@ fn build_arg(a: &SpecArg) -> &'static Arg<'static> { .var_max .filter(|_| a.var) .map(|max| u32::try_from(max).unwrap_or(u32::MAX)), + delimiter: a.delimiter.filter(char::is_ascii).map(|d| d as u8), double_dash: double_dash(&a.double_dash), })) } diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index 94bf59457..67be61f91 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -1187,3 +1187,62 @@ fn a_delimiter_reaches_the_spec() { assert_eq!(tags.arg.as_ref().unwrap().delimiter, Some(',')); assert_eq!(spec.cmd.args[0].delimiter, Some(';')); } + +/// A CLI whose bounded collections take their values several to a word. +#[allow(dead_code)] +#[derive(Cli)] +#[usage(bin = "ex4")] +struct BoundedSplitting { + /// Patterns, at most two per occurrence + #[usage(long, variadic, delimiter = ',', var_max = 2)] + include: Vec, + /// Targets, at most two + #[usage(delimiter = ':', var_max = 2)] + targets: Vec, +} + +#[test] +fn a_bound_counts_the_values_a_word_carried() { + // `var_max` bounds values, and a delimiter is what decides how many values a word is. + // Counting words instead let one word carry an occurrence straight past its bound. + let a = argv(["--include", "a,b,c"]); + assert!( + matches!( + BoundedSplitting::parse_from(&a), + Err(Error::VarTooMany { max: 2, got: 3, .. }) + ), + "three values out of one word is still three values" + ); + + let a = argv(["x:y:z"]); + assert!( + matches!( + BoundedSplitting::parse_from(&a), + Err(Error::VarTooMany { max: 2, got: 3, .. }) + ), + "a positional counts the same way" + ); +} + +#[test] +fn a_split_bound_still_counts_one_occurrence_at_a_time() { + // The rule the corpus documents for plain words holds for split ones: the bound is on + // what one occurrence takes, not on the list the occurrences build up. Reading the + // total would make the same declaration mean fewer values the more often it is given. + let a = argv(["--include", "a,b", "--include", "c,d"]); + assert_eq!( + BoundedSplitting::parse_from(&a) + .expect("two per occurrence is within the bound") + .include, + ["a", "b", "c", "d"] + ); + + // And a word carrying exactly the bound is still allowed. + let a = argv(["--include", "a,b"]); + assert_eq!( + BoundedSplitting::parse_from(&a) + .expect("exactly two") + .include, + ["a", "b"] + ); +} diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index 4ee496ad0..c486c99fc 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -763,6 +763,7 @@ fn flag_table(i: usize, field: &Field) -> TokenStream { None => quote!(::std::option::Option::None), }; + let table_delimiter = table_delimiter(field); quote! { pub static #name: usage_argv::Flag = usage_argv::Flag { key: #key, @@ -773,11 +774,26 @@ fn flag_table(i: usize, field: &Field) -> TokenStream { takes_value: #takes_value, variadic: #variadic, var_max: #var_max, + delimiter: #table_delimiter, global: #global, }; } } +/// The delimiter as the parser tables want it: a byte, or nothing. +/// +/// Validated to one byte where the attribute is read, so a `char` that does not fit is +/// already impossible here rather than silently truncated. +fn table_delimiter(field: &Field) -> TokenStream { + match field + .delimiter + .and_then(|d| u8::try_from(u32::from(d)).ok()) + { + Some(byte) => quote!(::std::option::Option::Some(#byte)), + None => quote!(::std::option::Option::None), + } +} + fn arg_table(i: usize, field: &Field) -> TokenStream { let name = format_ident!("ARG_{i}"); let key = key_ident("ARG", Some(i)); @@ -802,12 +818,14 @@ fn arg_table(i: usize, field: &Field) -> TokenStream { None => quote!(::std::option::Option::None), }; + let table_delimiter = table_delimiter(field); quote! { pub static #name: usage_argv::Arg = usage_argv::Arg { key: #key, name: #field_name, var: #var, var_max: #var_max, + delimiter: #table_delimiter, double_dash: #double_dash, }; } diff --git a/lib/src/parse.rs b/lib/src/parse.rs index 3c0a2af34..bf9204a7f 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1943,6 +1943,26 @@ fn collect_variadic_flag_values( return Ok(true); } } + // The loop stops once the occurrence has reached its bound, which without a delimiter is + // exactly when it has taken `max` words. A delimiter breaks that: one word can carry + // several values, so the run can end up *past* the bound rather than on it, and stopping + // is no longer the same as staying within it. `--include a,b,c` under `var_max=2` is the + // case — three values out of the one word the loop was entitled to take. + // + // Counted against `carried` like the loop itself, so this stays a statement about the + // occurrence rather than about the list the occurrences build up. + let taken = flags + .get(flag) + .map(value_count) + .unwrap_or(0) + .saturating_sub(carried); + if taken > max { + errors.push(UsageErr::VarFlagTooMany { + name: flag.name.clone(), + max, + got: taken, + }); + } Ok(false) } @@ -3161,6 +3181,30 @@ flag "--file " required_unless="--stdin" ); } + #[test] + fn a_split_bound_counts_one_occurrence_at_a_time() { + // The bound on a variadic flag *argument* is what one occurrence may take. Without a + // delimiter the collection simply stops at it, so it could never be exceeded; a word + // carrying several values can carry an occurrence past it in one step, and that is + // the only way this bound is ever breached. + let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--include ...\" delimiter=\",\" {\n arg \"...\" var=#true var_max=2\n}\n" + .parse() + .unwrap(); + + parse(&spec, &input(&["ex", "--include", "a,b"])).expect("exactly the bound is fine"); + assert!( + parse(&spec, &input(&["ex", "--include", "a,b,c"])).is_err(), + "three values out of one word is still three values" + ); + // The rule the corpus documents for plain words, on split ones: a second occurrence + // starts counting again rather than adding to the first. + parse( + &spec, + &input(&["ex", "--include", "a,b", "--include", "c,d"]), + ) + .expect("two per occurrence, twice, is within the bound"); + } + #[test] fn an_exclusive_flag_has_to_be_alone() { let spec: Spec = "name \"ex\"\nbin \"ex\"\nflag \"--dump\" exclusive=#true\nflag \"--verbose\"\narg \"[target]\"\n" From 58ae97256fff2d81a1086b1eaca58174c4a39814 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:07:43 +0000 Subject: [PATCH 7/8] fix(spec): a delimiter is one byte, and the spec now says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The KDL side accepted any single Unicode scalar while everything below it splits by byte, so the two disagreed in both directions. A separator above U+00FF was dropped on the way into the binding tables and the CLI quietly stopped splitting. Worse, one between U+0080 and U+00FF *fits* in a byte as a scalar while taking two in UTF-8, so matching that byte found the continuation bytes inside unrelated characters: with `delimiter="§"`, binding split `aЧbЧc` into three values and refused it against `var_max=2`, while usage-lib accepted it. Rejecting a command line nobody wrote a separator into is the worse half of the bug, and it arrived with the tables learning the delimiter at all. So the spec enforces what the derive already enforced where it reads the same property, in the same words: one byte, use an ASCII separator. Both conversions into the tables now filter on `is_ascii` rather than on fitting in a `u8`, which is the distinction that was wrong. The clap bridge records only an ASCII delimiter too. clap splits by character and may well have a wider one; the spec keeps `var`, so the values still arrive, and drops only its account of how they were separated — a spec that recorded it could not be written back out, since `to_kdl` would emit what parsing now refuses. Co-Authored-By: Claude Opus 5 --- derive/src/codegen.rs | 8 +++---- lib/src/spec/arg.rs | 54 +++++++++++++++++++++++++++++++++++++++++-- lib/src/spec/flag.rs | 20 +++++++++++++++- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index c486c99fc..5278c7ab4 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -785,10 +785,10 @@ fn flag_table(i: usize, field: &Field) -> TokenStream { /// Validated to one byte where the attribute is read, so a `char` that does not fit is /// already impossible here rather than silently truncated. fn table_delimiter(field: &Field) -> TokenStream { - match field - .delimiter - .and_then(|d| u8::try_from(u32::from(d)).ok()) - { + // ASCII, matching the rule the attribute enforces: splitting is by byte, and a + // separator that is one byte as a scalar but two as UTF-8 would match the continuation + // bytes inside unrelated characters. + match field.delimiter.filter(char::is_ascii).map(|d| d as u8) { Some(byte) => quote!(::std::option::Option::Some(#byte)), None => quote!(::std::option::Option::None), } diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index e60e31ece..392191dc7 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -124,7 +124,18 @@ impl SpecArg { let raw = v.ensure_string()?; let mut chars = raw.chars(); match (chars.next(), chars.next()) { - (Some(c), None) => arg.delimiter = Some(c), + // ASCII, not merely one character. Splitting is by byte everywhere + // below this — the derive says so where it reads the same property — + // and a non-ASCII separator has no single byte to be. Worse than + // having none: its bytes are continuation bytes, which appear inside + // unrelated characters, so it would split words nobody separated. + (Some(c), None) if c.is_ascii() => arg.delimiter = Some(c), + (Some(c), None) => bail_parse!( + ctx, + v.entry.span(), + "a delimiter is one byte, and {c:?} is more than one; use an \ + ASCII separator" + ), _ => bail_parse!( ctx, v.entry.span(), @@ -390,7 +401,11 @@ impl From<&clap::Arg> for SpecArg { let help_long = arg.get_long_help().map(|s| s.to_string()); let help_first_line = help.as_ref().map(|s| string::first_line(s)); let hide = arg.is_hide_set(); + // One byte only, for the reason given on the flag: a wider separator cannot be + // written back out. `var` below still reads the original, since clap splits on it + // either way and the field does collect several values. let delimiter = arg.get_value_delimiter(); + let recorded_delimiter = delimiter.filter(char::is_ascii); let var = matches!( arg.get_action(), clap::ArgAction::Count | clap::ArgAction::Append @@ -426,7 +441,7 @@ impl From<&clap::Arg> for SpecArg { var_min: None, // clap answers for this one, and the same getter `default_values` already // uses just above: a default is split by it, and so is a typed value. - delimiter, + delimiter: recorded_delimiter, hide, default: default_values(arg), choices: None, @@ -466,6 +481,41 @@ impl Hash for SpecArg { mod delimiter_tests { use crate::Spec; + #[test] + fn a_delimiter_has_to_be_one_byte() { + // Splitting is by byte below the spec. A separator that is one *character* but + // several bytes has no byte to be, and picking its low one would match the + // continuation bytes inside unrelated characters — `§` would split `aЧb`. Refused + // where it is written, which is the derive's rule too. + for spec in [ + "flag \"--tags \" var=#true delimiter=\"§\"\n", + "arg \"[tags]...\" var=#true delimiter=\"、\"\n", + ] { + let err = spec.parse::().unwrap_err(); + assert!(format!("{err:?}").contains("one byte"), "{err:?}"); + } + + // A clap command may still declare one; clap splits on it by character. The spec + // cannot say so, and drops it rather than recording a separator it could not write + // back out — the values still arrive, since `var` is set either way. + let cmd = clap::Command::new("ex").arg( + clap::Arg::new("tags") + .long("tags") + .value_delimiter('、') + .action(clap::ArgAction::Set), + ); + let spec = Spec::from(&cmd); + let arg = spec.cmd.flags[0].arg.as_ref().unwrap(); + assert_eq!( + arg.delimiter, None, + "a separator it cannot write is not recorded" + ); + assert!(arg.var, "clap still splits, so the values still arrive"); + spec.to_string() + .parse::() + .expect("what the bridge produces has to parse back"); + } + #[test] fn a_delimiter_round_trips_and_comes_across_from_clap() { let spec: Spec = "flag \"--tags \" var=#true delimiter=\",\"\n" diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index ef5780af8..9ac8ac8a0 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -332,6 +332,17 @@ impl SpecFlag { "a delimiter is one character, and {raw:?} is not" ); }; + // And one *byte*, for the reason given where an argument reads the same + // property: splitting is by byte below this, and a non-ASCII separator would + // match the continuation bytes inside unrelated characters. + if !delimiter.is_ascii() { + bail_parse!( + ctx, + node.node.name().span(), + "a delimiter is one byte, and {delimiter:?} is more than one; use an \ + ASCII separator" + ); + } let Some(arg) = flag.arg.as_mut() else { bail_parse!( ctx, @@ -665,7 +676,14 @@ impl From<&clap::Arg> for SpecFlag { // not. if let Some(delimiter) = c.get_value_delimiter() { arg.var = true; - arg.delimiter = Some(delimiter); + // Only if it is one byte. Splitting is by byte everywhere below the spec, + // and a spec carrying a wider separator could not be written back out — + // `to_kdl` would emit what parsing then refuses. clap still splits on it, + // so `var` stays: the values arrive, and only the spec's account of how + // they were separated is lost. + if delimiter.is_ascii() { + arg.delimiter = Some(delimiter); + } } else if var || c.get_num_args().is_some_and(|n| n.max_values() > 1) { arg.var = true; } From abc6187ab19fdf3ea2a4a8ccfbbdf1705d269495 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:29:47 +0000 Subject: [PATCH 8/8] fix(conformance): carry the delimiter through the spec-built tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `conformance/src/tables.rs` arrived on main while this branch was open, taking over the table building this branch had been editing in `argv.rs` and constructing each struct field by field — so the `delimiter` added here left four initializers short. Both models want it, and they want it differently: the binding tables take the byte binding counts values by, filtered on `is_ascii` rather than on fitting in a `u8`, and the metadata takes the `char` the spec declared. The short-flag assertion a few lines above already says why that distinction matters — a character that is several bytes in UTF-8 has no single byte that matches anything anyone could type. Co-Authored-By: Claude Opus 5 --- conformance/src/tables.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/conformance/src/tables.rs b/conformance/src/tables.rs index 35ef248a2..5821894ba 100644 --- a/conformance/src/tables.rs +++ b/conformance/src/tables.rs @@ -313,6 +313,9 @@ fn flag_meta( hide: f.hide, count: f.count, repeatable: f.var, + // The separator as declared, a `char`: the metadata is the cold model and says what + // the spec said, where the binding table beside it holds the byte binding counts by. + delimiter: arg.and_then(|a| a.delimiter), var_min: f.var_min.or(arg.and_then(|a| a.var_min)), var_max: f.var_max.or(arg.and_then(|a| a.var_max)), overrides: strs(&f.overrides), @@ -342,6 +345,7 @@ fn arg_meta( choices: a.choices.as_ref().map(|c| strs(&c.choices)).unwrap_or(&[]), required: a.required, hide: a.hide, + delimiter: a.delimiter, var_min: a.var_min, var_max: a.var_max, help_heading: opt(&a.help_heading),