From ee391ba38fb0b1cfff845a3ae48408d46e7838fd Mon Sep 17 00:00:00 2001 From: jdx <216188+jdx@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:57:01 +0000 Subject: [PATCH 1/3] fix(spec): reject several targets in a relationship property `conflicts="--b --c"` was read as one selector named "--b --c", which matches nothing, so the spec loaded and enforced nothing. Repeating the property kept only the last. Both are now errors that point at the child-node form (`conflicts "--b" "--c"`). The same check applies to `overrides`, `requires`, `required_if`, `required_unless` and `required_unless_all` on flags and positionals. Co-Authored-By: Claude Sonnet 5.5 Entire-Checkpoint: 01M676960BFB8S3TYV2JEYPFDH --- docs/spec/reference/flag.md | 14 ++++++++ lib/src/spec/arg.rs | 40 ++++++++++++++++++++--- lib/src/spec/flag.rs | 65 +++++++++++++++++++++++++++++++++---- lib/src/spec/helpers.rs | 36 ++++++++++++++++++++ 4 files changed, 144 insertions(+), 11 deletions(-) diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index 9e249cd6c..f5287a678 100644 --- a/docs/spec/reference/flag.md +++ b/docs/spec/reference/flag.md @@ -168,6 +168,20 @@ mistake, and silently honouring one of them hides it. A conflict holds in either direction, so declaring it once is enough; it applies to flags that were actually given, not to defaults. +The property form names one flag. To conflict with several, write `conflicts` as a child +node with one argument per flag: + +```kdl +flag "--a" { + conflicts "--b" "--c" // --a cannot be given with --b or with --c +} +``` + +`conflicts="--b --c"` and a repeated `conflicts="--b" conflicts="--c"` are errors when the +spec loads, since the first is one selector that matches nothing and the second would keep +only the last. The same goes for `overrides`, `requires`, `required_if`, +`required_unless` and `required_unless_all`. + Losing an override unsets a flag; it does not unsay the word it was given. `--log-level=v --trace` is still refused when `v` is outside `--log-level`'s [`choices`](/spec/reference/arg) — which diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index 1b06f9f3c..9041fa66a 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -226,6 +226,13 @@ impl SpecArg { pub(crate) fn parse(ctx: &ParsingContext, node: &NodeHelper) -> Result { let mut arg: SpecArg = node.arg(0)?.ensure_string()?.parse()?; + node.ensure_props_unrepeated(&[ + "conflicts", + "requires", + "required_if", + "required_unless", + "required_unless_all", + ])?; for (k, v) in node.props() { match k { "help" => arg.help = Some(v.ensure_string()?), @@ -268,11 +275,13 @@ impl SpecArg { "hide_possible_values" => arg.hide_possible_values = v.ensure_bool()?, "hide_short_help" => arg.hide_short_help = v.ensure_bool()?, "hide_long_help" => arg.hide_long_help = v.ensure_bool()?, - "conflicts" => arg.conflicts = vec![v.ensure_string()?], - "requires" => arg.requires = vec![v.ensure_string()?], - "required_if" => arg.required_if = vec![v.ensure_string()?], - "required_unless" => arg.required_unless = vec![v.ensure_string()?], - "required_unless_all" => arg.required_unless_all = vec![v.ensure_string()?], + "conflicts" => v.set_selector("conflicts", &mut arg.conflicts)?, + "requires" => v.set_selector("requires", &mut arg.requires)?, + "required_if" => v.set_selector("required_if", &mut arg.required_if)?, + "required_unless" => v.set_selector("required_unless", &mut arg.required_unless)?, + "required_unless_all" => { + v.set_selector("required_unless_all", &mut arg.required_unless_all)? + } "var_min" => arg.var_min = v.ensure_usize().map(Some)?, "var_max" => arg.var_max = v.ensure_usize().map(Some)?, "default" => arg.default = vec![v.ensure_string()?], @@ -1754,6 +1763,27 @@ arg "" { assert_eq!(reparsed.cmd.args[0].conflicts, spec.cmd.args[0].conflicts); } + #[test] + fn a_positional_relationship_property_names_one_target() { + for key in [ + "conflicts", + "requires", + "required_if", + "required_unless", + "required_unless_all", + ] { + let several = format!("arg \"[VALUE]\" {key}=\"--a --b\"\n"); + let err = several.parse::().unwrap_err(); + let crate::error::UsageErr::InvalidInput(err, ..) = err else { + panic!("unexpected error: {err:?}"); + }; + assert!( + err.contains(&format!("{key} takes one flag or argument per property")), + "{key}: {err}" + ); + } + } + #[test] fn a_complete_inside_an_arg_is_attached_to_it() { let spec: Spec = r#" diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index 3757eb510..6a17eea96 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -351,6 +351,14 @@ impl SpecFlag { let mut allow_negative_numbers = false; let mut value_terminator: Option = None; let mut delimiter: Option = None; + node.ensure_props_unrepeated(&[ + "conflicts", + "requires", + "overrides", + "required_if", + "required_unless", + "required_unless_all", + ])?; for (k, v) in node.props() { match k { "help" => flag.help = Some(v.ensure_string()?), @@ -358,9 +366,13 @@ impl SpecFlag { "help_long" => flag.help_long = Some(v.ensure_string()?), "help_md" => flag.help_md = Some(v.ensure_string()?), "required" => flag.required = v.ensure_bool()?, - "required_if" => flag.required_if = vec![v.ensure_string()?], - "required_unless" => flag.required_unless = vec![v.ensure_string()?], - "required_unless_all" => flag.required_unless_all = vec![v.ensure_string()?], + "required_if" => v.set_selector("required_if", &mut flag.required_if)?, + "required_unless" => { + v.set_selector("required_unless", &mut flag.required_unless)? + } + "required_unless_all" => { + v.set_selector("required_unless_all", &mut flag.required_unless_all)? + } "var" => flag.var = v.ensure_bool()?, "var_min" => flag.var_min = v.ensure_usize().map(Some)?, "var_max" => flag.var_max = v.ensure_usize().map(Some)?, @@ -402,9 +414,9 @@ impl SpecFlag { flag.default = vec![default_value]; } "negate" => flag.negate = v.ensure_string().map(Some)?, - "overrides" => flag.overrides = vec![v.ensure_string()?], - "conflicts" => flag.conflicts = vec![v.ensure_string()?], - "requires" => flag.requires = vec![v.ensure_string()?], + "overrides" => v.set_selector("overrides", &mut flag.overrides)?, + "conflicts" => v.set_selector("conflicts", &mut flag.conflicts)?, + "requires" => v.set_selector("requires", &mut flag.requires)?, "exclusive" => flag.exclusive = v.ensure_bool()?, "require_equals" => flag.require_equals = v.ensure_bool()?, "value_optional" => flag.value_optional = v.ensure_bool()?, @@ -1678,6 +1690,47 @@ mod tests { assert_snapshot!("myflag: -f --flag ".parse::().unwrap(), @"myflag: -f --flag "); } + #[test] + fn a_relationship_property_names_one_flag() { + for key in [ + "conflicts", + "requires", + "overrides", + "required_if", + "required_unless", + "required_unless_all", + ] { + let several = format!("flag \"--a\" {key}=\"--b --c\"\nflag \"--b\"\nflag \"--c\"\n"); + let err = several.parse::().unwrap_err(); + let crate::error::UsageErr::InvalidInput(err, ..) = err else { + panic!("unexpected error: {err:?}"); + }; + assert!( + err.contains(&format!("{key} takes one flag or argument per property")), + "{key}: {err}" + ); + + let repeated = format!("flag \"--a\" {key}=\"--b\" {key}=\"--c\"\n"); + let err = repeated.parse::().unwrap_err(); + let crate::error::UsageErr::InvalidInput(err, ..) = err else { + panic!("unexpected error: {err:?}"); + }; + assert!( + err.contains(&format!("{key} is given more than once")), + "{key}: {err}" + ); + } + } + + #[test] + fn several_relationship_targets_are_a_child_node() { + let spec: Spec = + "flag \"--a\" {\n conflicts \"--b\" \"--c\"\n}\nflag \"--b\"\nflag \"--c\"\n" + .parse() + .unwrap(); + assert_eq!(spec.cmd.flags[0].conflicts, ["--b", "--c"]); + } + #[test] fn clap_token_boundaries_survive_the_bridge() { let command = clap::Command::new("ex") diff --git a/lib/src/spec/helpers.rs b/lib/src/spec/helpers.rs index cfa38b5a0..dac6f777d 100644 --- a/lib/src/spec/helpers.rs +++ b/lib/src/spec/helpers.rs @@ -134,6 +134,24 @@ impl<'a> NodeHelper<'a> { } Ok(self) } + /// Refuse a property written twice, which would otherwise keep only the last. + pub(crate) fn ensure_props_unrepeated(&self, keys: &[&str]) -> Result<(), UsageErr> { + for key in keys { + let mut written = self + .node + .entries() + .iter() + .filter(|e| e.name().is_some_and(|name| name.value() == *key)); + if let (Some(_), Some(second)) = (written.next(), written.next()) { + bail_parse!( + self.ctx, + second.span(), + "{key} is given more than once; the second would replace the first, so write several as a child node instead: {key} \"--a\" \"--b\"" + ); + } + } + Ok(()) + } pub(crate) fn get(&self, key: &str) -> Option> { self.node.entry(key).map(|e| ParseEntry::new(self.ctx, e)) } @@ -220,6 +238,24 @@ impl ParseEntry<'_> { None => bail_parse!(self.ctx, self.span(), "expected string"), } } + + /// Set a relationship list (`conflicts`, `requires`, …) from its property form. + /// + /// The property names exactly one flag or argument. A value like `"--b --c"` is a + /// single selector that matches nothing, so it is refused rather than left to enforce + /// nothing. Several targets are written as a child node: `conflicts "--b" "--c"`. + pub fn set_selector(&self, key: &str, list: &mut Vec) -> Result<(), UsageErr> { + let selector = self.ensure_string()?; + if selector.split_whitespace().nth(1).is_some() { + bail_parse!( + self.ctx, + self.span(), + "{key} takes one flag or argument per property, but {selector:?} has several; write them as a child node instead: {key} \"--a\" \"--b\"" + ); + } + list.push(selector); + Ok(()) + } } #[cfg(test)] From 7c6695ab3e4a45828cd441bb750516034e602cae Mon Sep 17 00:00:00 2001 From: jdx <216188+jdx@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:25:29 +0000 Subject: [PATCH 2/3] fix(spec): keep selectors that name a positional containing a space Only a value with a dashed word is refused as several flags. A bare selector names a positional, and `arg "[input file]"` is named `input file`, so `conflicts="input file"` matched it before and still does. Co-Authored-By: Claude Sonnet 5.5 Entire-Checkpoint: 01M677VSAAPBNEWMNDCRKKM303 --- docs/spec/reference/flag.md | 6 +++--- lib/src/spec/arg.rs | 9 +++++++++ lib/src/spec/helpers.rs | 10 +++++++++- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index f5287a678..32b71153a 100644 --- a/docs/spec/reference/flag.md +++ b/docs/spec/reference/flag.md @@ -177,9 +177,9 @@ flag "--a" { } ``` -`conflicts="--b --c"` and a repeated `conflicts="--b" conflicts="--c"` are errors when the -spec loads, since the first is one selector that matches nothing and the second would keep -only the last. The same goes for `overrides`, `requires`, `required_if`, +A value naming several flags, like `conflicts="--b --c"`, is an error when the spec loads, +since it is one selector that matches nothing. So is repeating the property, which would +keep only the last. The same goes for `overrides`, `requires`, `required_if`, `required_unless` and `required_unless_all`. Losing an override unsets a flag; it does not unsay the word it was given. `--log-level=v diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index 9041fa66a..dd07a5c4e 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -1784,6 +1784,15 @@ arg "" { } } + #[test] + fn a_selector_may_name_a_positional_with_a_space() { + let spec: Spec = "flag \"--a\" conflicts=\"input file\"\narg \"[input file]\"\n" + .parse() + .unwrap(); + assert_eq!(spec.cmd.flags[0].conflicts, ["input file"]); + assert_eq!(spec.cmd.args[0].name, "input file"); + } + #[test] fn a_complete_inside_an_arg_is_attached_to_it() { let spec: Spec = r#" diff --git a/lib/src/spec/helpers.rs b/lib/src/spec/helpers.rs index dac6f777d..3f8711fde 100644 --- a/lib/src/spec/helpers.rs +++ b/lib/src/spec/helpers.rs @@ -244,9 +244,17 @@ impl ParseEntry<'_> { /// The property names exactly one flag or argument. A value like `"--b --c"` is a /// single selector that matches nothing, so it is refused rather than left to enforce /// nothing. Several targets are written as a child node: `conflicts "--b" "--c"`. + /// + /// Only a value with a dashed word is refused. A bare selector names a positional, and + /// a positional's name may hold a space (`arg "[input file]"`), so `"input file"` is + /// kept as the one name it may be. pub fn set_selector(&self, key: &str, list: &mut Vec) -> Result<(), UsageErr> { let selector = self.ensure_string()?; - if selector.split_whitespace().nth(1).is_some() { + if selector.split_whitespace().nth(1).is_some() + && selector + .split_whitespace() + .any(|word| word.starts_with('-')) + { bail_parse!( self.ctx, self.span(), From 5c5d762f2f1e53d22c9f2bd7a759f4d93ae73dfd Mon Sep 17 00:00:00 2001 From: jdx <216188+jdx@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:34:18 +0000 Subject: [PATCH 3/3] fix(spec): write selectors a property cannot hold as a child node A child node can name a positional like `input - file`, but written back as `conflicts="input - file"` it reads as a list of flags and is refused. Every single-selector serializer now falls back to the child-node form for such a selector, so a spec round-trips. Co-Authored-By: Claude Sonnet 5.5 Entire-Checkpoint: 01M678B5M9RQ2XDZ94PMNE8CYQ --- lib/src/spec/arg.rs | 21 ++++++++++++++++++--- lib/src/spec/flag.rs | 10 +++++----- lib/src/spec/helpers.rs | 17 ++++++++++++----- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/lib/src/spec/arg.rs b/lib/src/spec/arg.rs index dd07a5c4e..0e96598d2 100644 --- a/lib/src/spec/arg.rs +++ b/lib/src/spec/arg.rs @@ -9,7 +9,7 @@ use crate::error::UsageErr; use crate::spec::builder::SpecArgBuilder; use crate::spec::context::ParsingContext; use crate::spec::effect::{EFFECT_VALUES, SpecCommandEffect}; -use crate::spec::helpers::{NodeHelper, string_entry}; +use crate::spec::helpers::{NodeHelper, fits_selector_property, string_entry}; use crate::spec::is_false; use crate::{SpecAdmonition, SpecAdmonitionKind, SpecChoices, SpecComplete, string}; #[cfg(feature = "clap")] @@ -660,7 +660,7 @@ impl From<&SpecArg> for KdlNode { node.push(KdlEntry::new_prop(name, true)); } } - if arg.conflicts.len() == 1 { + if arg.conflicts.len() == 1 && fits_selector_property(&arg.conflicts[0]) { node.push(string_entry(Some("conflicts"), &arg.conflicts[0])); } else if !arg.conflicts.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); @@ -767,7 +767,7 @@ fn required_if_eq_pairs(node: &NodeHelper<'_>) -> Result, } fn serialize_selector_list(node: &mut KdlNode, name: &str, selectors: &[String]) { - if selectors.len() == 1 { + if selectors.len() == 1 && fits_selector_property(&selectors[0]) { node.push(string_entry(Some(name), &selectors[0])); } else if !selectors.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); @@ -1793,6 +1793,21 @@ arg "" { assert_eq!(spec.cmd.args[0].name, "input file"); } + #[test] + fn a_selector_a_property_cannot_hold_round_trips_as_a_child_node() { + let spec: Spec = "arg \"[VALUE]\" { conflicts \"input - file\" }\n" + .parse() + .unwrap(); + let reparsed: Spec = spec.to_string().parse().unwrap(); + assert_eq!(reparsed.cmd.args[0].conflicts, ["input - file"]); + + let spec: Spec = "flag \"--a\" { requires \"input - file\" }\n" + .parse() + .unwrap(); + let reparsed: Spec = spec.to_string().parse().unwrap(); + assert_eq!(reparsed.cmd.flags[0].requires, ["input - file"]); + } + #[test] fn a_complete_inside_an_arg_is_attached_to_it() { let spec: Spec = r#" diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index 6a17eea96..75da7fb8f 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -12,7 +12,7 @@ use crate::spec::arg::SpecDoubleDashChoices; use crate::spec::builder::SpecFlagBuilder; use crate::spec::context::ParsingContext; use crate::spec::effect::{EFFECT_VALUES, SpecCommandEffect}; -use crate::spec::helpers::{NodeHelper, string_entry}; +use crate::spec::helpers::{NodeHelper, fits_selector_property, string_entry}; use crate::spec::is_false; use crate::{ SpecAdmonition, SpecAdmonitionKind, SpecArg, SpecChoices, SpecComplete, SpecRequiredIfEq, @@ -1120,7 +1120,7 @@ impl From<&SpecFlag> for KdlNode { if let Some(negate) = &flag.negate { node.push(string_entry(Some("negate"), negate)); } - if flag.overrides.len() == 1 { + if flag.overrides.len() == 1 && fits_selector_property(&flag.overrides[0]) { node.push(string_entry(Some("overrides"), &flag.overrides[0])); } else if !flag.overrides.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); @@ -1130,7 +1130,7 @@ impl From<&SpecFlag> for KdlNode { } children.nodes_mut().push(overrides); } - if flag.conflicts.len() == 1 { + if flag.conflicts.len() == 1 && fits_selector_property(&flag.conflicts[0]) { node.push(string_entry(Some("conflicts"), &flag.conflicts[0])); } else if !flag.conflicts.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); @@ -1140,7 +1140,7 @@ impl From<&SpecFlag> for KdlNode { } children.nodes_mut().push(conflicts); } - if flag.requires.len() == 1 { + if flag.requires.len() == 1 && fits_selector_property(&flag.requires[0]) { node.push(string_entry(Some("requires"), &flag.requires[0])); } else if !flag.requires.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); @@ -1244,7 +1244,7 @@ impl From<&SpecFlag> for KdlNode { } fn serialize_flag_list(node: &mut KdlNode, name: &str, flags: &[String]) { - if flags.len() == 1 { + if flags.len() == 1 && fits_selector_property(&flags[0]) { node.push(string_entry(Some(name), &flags[0])); } else if !flags.is_empty() { let children = node.children_mut().get_or_insert_with(KdlDocument::new); diff --git a/lib/src/spec/helpers.rs b/lib/src/spec/helpers.rs index 3f8711fde..c0b34ef14 100644 --- a/lib/src/spec/helpers.rs +++ b/lib/src/spec/helpers.rs @@ -105,6 +105,17 @@ pub(crate) fn string_entry(key: Option<&str>, value: &str) -> KdlEntry { entry } +/// Whether a selector can be written as `conflicts="…"` and read back as the same one. +/// +/// A value with a dashed word among several is taken for a list of flags, so a selector +/// shaped like that has to be written as a child node instead. +pub(crate) fn fits_selector_property(selector: &str) -> bool { + selector.split_whitespace().nth(1).is_none() + || !selector + .split_whitespace() + .any(|word| word.starts_with('-')) +} + #[derive(Debug)] pub struct NodeHelper<'a> { pub(crate) node: &'a KdlNode, @@ -250,11 +261,7 @@ impl ParseEntry<'_> { /// kept as the one name it may be. pub fn set_selector(&self, key: &str, list: &mut Vec) -> Result<(), UsageErr> { let selector = self.ensure_string()?; - if selector.split_whitespace().nth(1).is_some() - && selector - .split_whitespace() - .any(|word| word.starts_with('-')) - { + if !fits_selector_property(&selector) { bail_parse!( self.ctx, self.span(),