fix(spec): reject several targets in a relationship property

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
+}
+```
+
+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
 --trace` is still refused when `v` is outside `--log-level`'s
 [`choices`](/spec/reference/arg) — which

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")]
@@ -226,6 +226,13 @@ impl SpecArg {
 
     pub(crate) fn parse(ctx: &ParsingContext, node: &NodeHelper) -> Result<Self, UsageErr> {
         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()?],
@@ -651,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);
@@ -758,7 +767,7 @@ fn required_if_eq_pairs(node: &NodeHelper<'_>) -> Result<Vec<SpecRequiredIfEq>,
 }
 
 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);
@@ -1754,6 +1763,51 @@ arg "<input>" {
         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::<Spec>().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_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_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#"

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,
@@ -351,16 +351,28 @@ impl SpecFlag {
         let mut allow_negative_numbers = false;
         let mut value_terminator: Option<String> = None;
         let mut delimiter: Option<String> = 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()?),
                 "long_help" => flag.help_long = Some(v.ensure_string()?),
                 "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()?,
@@ -1108,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);
@@ -1118,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);
@@ -1128,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);
@@ -1232,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);
@@ -1678,6 +1690,47 @@ mod tests {
         assert_snapshot!("myflag: -f --flag <arg>".parse::<SpecFlag>().unwrap(), @"myflag: -f --flag <arg>");
     }
 
+    #[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::<Spec>().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::<Spec>().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")

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,
@@ -134,6 +145,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<ParseEntry<'_>> {
         self.node.entry(key).map(|e| ParseEntry::new(self.ctx, e))
     }
@@ -220,6 +249,28 @@ 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"`.
+    ///
+    /// 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<String>) -> Result<(), UsageErr> {
+        let selector = self.ensure_string()?;
+        if !fits_selector_property(&selector) {
+            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)]