diff --git a/cli/usage.usage.kdl b/cli/usage.usage.kdl index 00d1cc8f2..5eff561c6 100644 --- a/cli/usage.usage.kdl +++ b/cli/usage.usage.kdl @@ -161,7 +161,7 @@ Common sections: - 1: User commands - 5: File formats - 7: Miscellaneous - 8: Sy flag "-f --file" help="A usage spec taken in as a file, use \"-\" to read from stdin" required=#true { arg } - flag "-m --multi" help="Render each subcommand as a separate markdown file" + flag "-m --multi" help="Render each subcommand as a separate markdown file" conflicts=--out-file flag --html-encode help="Escape HTML in markdown" flag --out-dir help="Output markdown files to this directory (required when using --multi)" effect=write { arg diff --git a/conformance/src/lib.rs b/conformance/src/lib.rs index 77ddfe527..125b4ec97 100644 --- a/conformance/src/lib.rs +++ b/conformance/src/lib.rs @@ -157,6 +157,8 @@ pub enum ErrorCode { VarTooFew, /// A variadic got more values than `var_max`. VarTooMany, + /// Two flags declared to conflict were both given. + ConflictingFlags, } /// Whether the reference implementation matches a vector's expectation. diff --git a/conformance/src/reference.rs b/conformance/src/reference.rs index 9a78584c2..317c87934 100644 --- a/conformance/src/reference.rs +++ b/conformance/src/reference.rs @@ -120,7 +120,9 @@ fn classify(msg: &str) -> Observed { } else if msg.contains("Invalid flag") { // usage-lib funnels several distinct situations through InvalidFlag, so // the reason has to be read to tell them apart. - if msg.contains("requires an argument") || msg.contains("missing value") { + if msg.contains("conflicts with") { + ErrorCode::ConflictingFlags + } else if msg.contains("requires an argument") || msg.contains("missing value") { ErrorCode::MissingFlagValue } else if msg.contains("Invalid choice") || msg.contains("expected one of") { ErrorCode::InvalidChoice diff --git a/corpus/08-choices-and-required.json b/corpus/08-choices-and-required.json index aadc52baa..a42f9031c 100644 --- a/corpus/08-choices-and-required.json +++ b/corpus/08-choices-and-required.json @@ -7,14 +7,22 @@ "doc": "A value among the declared choices is accepted.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--shell \" {\n choices \"bash\" \"zsh\" \"fish\"\n}\n", "argv": ["--shell", "zsh"], - "expect": { "ok": { "flags": { "shell": "zsh" } } } + "expect": { + "ok": { + "flags": { + "shell": "zsh" + } + } + } }, { "id": "choice-invalid", "doc": "A value outside the declared choices is rejected.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--shell \" {\n choices \"bash\" \"zsh\" \"fish\"\n}\n", "argv": ["--shell", "csh"], - "expect": { "error": "invalid_choice" }, + "expect": { + "error": "invalid_choice" + }, "layer": "post-binding" }, { @@ -22,7 +30,9 @@ "doc": "Choices match exactly. Case-insensitive matching would have to be declared, not assumed.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--shell \" {\n choices \"bash\" \"zsh\"\n}\n", "argv": ["--shell", "ZSH"], - "expect": { "error": "invalid_choice" }, + "expect": { + "error": "invalid_choice" + }, "layer": "post-binding" }, { @@ -30,21 +40,35 @@ "doc": "Positionals carry choices too.", "spec": "name \"ex\"\nbin \"ex\"\narg \"\" {\n choices \"bash\" \"zsh\"\n}\n", "argv": ["bash"], - "expect": { "ok": { "args": { "shell": "bash" } } } + "expect": { + "ok": { + "args": { + "shell": "bash" + } + } + } }, { "id": "required-flag-present", "doc": "A required flag that is given parses normally.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" required=#true\n", "argv": ["--file", "a.txt"], - "expect": { "ok": { "flags": { "file": "a.txt" } } } + "expect": { + "ok": { + "flags": { + "file": "a.txt" + } + } + } }, { "id": "required-flag-missing", "doc": "A required flag that is absent is an error.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" required=#true\n", "argv": [], - "expect": { "error": "missing_required_flag" }, + "expect": { + "error": "missing_required_flag" + }, "layer": "post-binding" }, { @@ -52,14 +76,22 @@ "doc": "`required_unless` is satisfied when the named alternative is present.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" required_unless=\"--stdin\"\nflag \"--stdin\"\n", "argv": ["--stdin"], - "expect": { "ok": { "flags": { "stdin": true } } } + "expect": { + "ok": { + "flags": { + "stdin": true + } + } + } }, { "id": "required-unless-unsatisfied", "doc": "With neither flag present, the requirement stands.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" required_unless=\"--stdin\"\nflag \"--stdin\"\n", "argv": [], - "expect": { "error": "missing_required_flag" }, + "expect": { + "error": "missing_required_flag" + }, "layer": "post-binding" }, { @@ -67,7 +99,87 @@ "doc": "Two flags that override each other leave only the last one given.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" overrides=\"--stdin\"\nflag \"--stdin\" overrides=\"--file\"\n", "argv": ["--stdin", "--file", "a.txt"], - "expect": { "ok": { "flags": { "file": "a.txt" } } }, + "expect": { + "ok": { + "flags": { + "file": "a.txt" + } + } + }, + "layer": "post-binding" + }, + { + "id": "conflicts-both-given", + "doc": "Two flags declared to conflict cannot be given together. Unlike `overrides`, where the last one wins, a conflict is a mistake to report rather than an order to resolve.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url \"\n", + "argv": ["--file", "a.txt", "--stdin"], + "expect": { + "error": "conflicting_flags" + }, + "layer": "post-binding" + }, + { + "id": "conflicts-either-order", + "doc": "The conflict is declared on `--file` only, and still applies when `--stdin` comes first: the relationship is between the two flags, not between a flag and the tokens after it.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url \"\n", + "argv": ["--stdin", "--file", "a.txt"], + "expect": { + "error": "conflicting_flags" + }, + "layer": "post-binding" + }, + { + "id": "conflicts-one-given", + "doc": "A declared conflict costs nothing when only one side is given.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url \"\n", + "argv": ["--stdin"], + "expect": { + "ok": { + "flags": { + "stdin": true + } + } + } + }, + { + "id": "conflicts-unrelated-flag", + "doc": "Only the named flag conflicts; an unnamed one alongside it is fine.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url \"\n", + "argv": ["--file", "a.txt", "--url", "u"], + "expect": { + "ok": { + "flags": { + "file": "a.txt", + "url": "u" + } + } + } + }, + { + "id": "conflicts-one-side-from-env", + "doc": "A value from the environment counts as given: `--file` conflicts with `--stdin` even though only `--stdin` was typed. clap, argparse and usage all treat an environment variable as a value source rather than as a weaker kind of default.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" env=\"EX_FILE\" conflicts=\"--stdin\"\nflag \"--stdin\" env=\"EX_STDIN\"\n", + "argv": ["--stdin"], + "env": { + "EX_FILE": "a.txt" + }, + "expect": { + "error": "conflicting_flags" + }, + "layer": "post-binding" + }, + { + "id": "conflicts-both-sides-from-env", + "doc": "Neither side was typed and the conflict still holds, which is what makes the rule uniform: the check asks whether a flag has a value, not how it got one.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--file \" env=\"EX_FILE\" conflicts=\"--stdin\"\nflag \"--stdin\" env=\"EX_STDIN\"\n", + "argv": [], + "env": { + "EX_FILE": "a.txt", + "EX_STDIN": "1" + }, + "expect": { + "error": "conflicting_flags" + }, "layer": "post-binding" }, { @@ -75,7 +187,9 @@ "doc": "`var_min` is enforced for a repeatable flag, not only for positionals.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--include \" var=#true var_min=2\n", "argv": ["--include", "a"], - "expect": { "error": "var_too_few" }, + "expect": { + "error": "var_too_few" + }, "layer": "post-binding" }, { @@ -83,7 +197,9 @@ "doc": "`var_max` is likewise enforced for a repeatable flag.", "spec": "name \"ex\"\nbin \"ex\"\nflag \"--include \" var=#true var_max=1\n", "argv": ["--include", "a", "--include", "b"], - "expect": { "error": "var_too_many" }, + "expect": { + "error": "var_too_many" + }, "layer": "post-binding" } ] diff --git a/docs/cli/reference/commands.json b/docs/cli/reference/commands.json index 7fb6e4911..d1c1559d0 100644 --- a/docs/cli/reference/commands.json +++ b/docs/cli/reference/commands.json @@ -676,7 +676,8 @@ "short": ["m"], "long": ["multi"], "hide": false, - "global": false + "global": false, + "conflicts": ["--out-file"] }, { "name": "html-encode", diff --git a/docs/spec/argv.md b/docs/spec/argv.md index 2367cba79..edcad6d2c 100644 --- a/docs/spec/argv.md +++ b/docs/spec/argv.md @@ -265,6 +265,7 @@ told apart mechanically. | `arg_requires_double_dash` | a `double_dash="required"` argument got a value too early | | `var_too_few` | fewer values than `var_min` | | `var_too_many` | more values than `var_max` | +| `conflicting_flags` | two flags declared to `conflict` were both given | Choices match exactly; case-insensitive matching would have to be declared rather than assumed. @@ -301,7 +302,8 @@ Any implementation in any language can run these. In this repository, Each vector says which layer of a parser it is a question for. Most are `binding` — which token becomes which flag or argument — and a parser that reads argv is expected to answer all of those. The rest are `post-binding`: `required`, -`choices`, `env` fallback, defaults, `var_min`/`var_max`, and `overrides` are +`choices`, `env` fallback, defaults, `var_min`/`var_max`, `overrides`, and +`conflicts` are decided once the last token has been read, and need to know a value's type, so a binding-only parser leaves them to the layer that owns the target struct. diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index 1851f4c07..0b10bafec 100644 --- a/docs/spec/reference/flag.md +++ b/docs/spec/reference/flag.md @@ -30,6 +30,11 @@ flag "--dir " // args named "" will be completed as directories flag "--file " required_if="--dir" // if --dir is set, --file must also be set flag "--file " required_unless="--dir" // either --file or --dir must be present flag "--file " overrides="--stdin" // --file and --stdin override each other; the last one wins +flag "--file " conflicts="--stdin" // --file and --stdin cannot be given together + +flag "--stdin" { + conflicts "--file" "--url" // several, one per argument +} flag "--shell " { choices "bash" "zsh" "fish" // must be one of the choices @@ -53,6 +58,17 @@ flag "--file " { } ``` +## `conflicts` and `overrides` + +Both describe a pair of flags that should not be in effect at once, and they differ in +what to do about it. `overrides` resolves the collision — the last one given wins, which +is what you want for `--color`/`--no-color`, where a later flag is a correction. `conflicts` +reports it, for flags whose combination has no sensible meaning at all: giving both is a +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. + ## `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 3b6d5f3b7..15b11d927 100644 --- a/lib/src/parse.rs +++ b/lib/src/parse.rs @@ -1128,6 +1128,34 @@ fn parse_partial_with_env( } } + // Conflicts are a question about the invocation as a whole rather than about any one + // token, so they are checked here beside the requirement checks rather than at the + // point a flag is matched — the flag it conflicts with may still be ahead of it. + // Its own loop: the requirement loop below skips the flags that *were* given, which + // is exactly the set this needs. + // + // A value from the environment counts on both sides, matching what + // `selector_is_explicit` says about the other flag: the question is whether a flag + // has a value, not how it got one. That is what clap does, and an asymmetric rule + // would make the same pair of flags a conflict or not depending on which one + // happened to be typed. + for flag in unique_flags(out.available_flags.values()) { + let given = out.flags.contains_key(flag) || flag_has_env(flag, custom_env); + if !given || overridden_flags.contains(&flag.name) { + continue; + } + for other in &flag.conflicts { + if selector_is_explicit(other, &out, &overridden_flags, custom_env) { + out.errors.push(UsageErr::InvalidFlag { + token: format!("--{}", flag.name), + reason: format!("conflicts with {other}"), + span: (0, 0).into(), + input: format!("--{} {other}", flag.name), + }); + } + } + } + for flag in unique_flags(out.available_flags.values()) { if out.flags.contains_key(flag) || overridden_flags.contains(&flag.name) { continue; @@ -2531,6 +2559,32 @@ flag "--file " required_unless="--stdin" } } + #[test] + fn conflicting_flags_are_rejected_in_either_order() { + // Declared once, on `--file`, which is all clap exposes — so the check has to + // be order-independent by looking at every flag that was given rather than at + // the one that declared the conflict. + let spec: Spec = + "name \"ex\"\nbin \"ex\"\nflag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\"\n" + .parse() + .unwrap(); + + for words in [ + &["ex", "--file", "a.txt", "--stdin"][..], + &["ex", "--stdin", "--file", "a.txt"][..], + ] { + let err = parse(&spec, &input(words)).unwrap_err(); + assert!( + err.to_string().contains("conflicts with --stdin"), + "{words:?} should be refused: {err}" + ); + } + + // Either one alone is fine. + parse(&spec, &input(&["ex", "--stdin"])).unwrap(); + parse(&spec, &input(&["ex", "--file", "a.txt"])).unwrap(); + } + #[test] fn unknown_flags_are_values_by_default() { // The default, and the reason it is the default: a spec often parses a diff --git a/lib/src/spec/cmd.rs b/lib/src/spec/cmd.rs index 702eb3b89..63e560c73 100644 --- a/lib/src/spec/cmd.rs +++ b/lib/src/spec/cmd.rs @@ -806,7 +806,28 @@ impl From<&clap::Command> for SpecCommand { if arg.is_positional() { spec.args.push(arg.into()) } else { - spec.flags.push(arg.into()) + let mut flag: SpecFlag = arg.into(); + // clap keeps conflicts on the command rather than on the argument, so + // this is the only place both are in view. Written with dashes, + // matching how the spec refers to a flag everywhere else. + // + // A short-only flag is named `-s`, which selectors accept as readily as + // `--long`: taking only the long form would have dropped the conflict + // and let the spec accept a combination clap rejects. + flag.conflicts = cmd + .get_arg_conflicts_with(arg) + .iter() + .filter_map(|other| match (other.get_long(), other.get_short()) { + (Some(long), _) => Some(format!("--{long}")), + (None, Some(short)) => Some(format!("-{short}")), + // A positional, which the spec has no way to name in a + // conflict. Dropping it is a loss, but writing `--` would + // be a selector matching nothing, which is worse: it reads as a + // relationship that holds. + (None, None) => None, + }) + .collect(); + spec.flags.push(flag) } } spec.subcommand_required = cmd.is_subcommand_required_set(); diff --git a/lib/src/spec/flag.rs b/lib/src/spec/flag.rs index 1812a36b3..dd2fdaa38 100644 --- a/lib/src/spec/flag.rs +++ b/lib/src/spec/flag.rs @@ -94,6 +94,14 @@ pub struct SpecFlag { /// Flags that this flag mutually overrides; the last one provided wins #[serde(skip_serializing_if = "Vec::is_empty")] pub overrides: Vec, + /// Flags that cannot be given alongside this one. + /// + /// Distinct from [`SpecFlag::overrides`], which is about the *last* one winning: + /// conflicting flags are a mistake to report, not an order to resolve. clap has + /// had `conflicts_with` for years and mise uses it forty times, so a spec + /// generated from a clap command was losing it. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub conflicts: Vec, /// Raises the effect of the command when this flag is supplied. /// See [`crate::spec::effect::SpecCommandEffect`]; never lowers it. #[serde(skip_serializing_if = "Option::is_none")] @@ -153,6 +161,7 @@ impl SpecFlag { } "negate" => flag.negate = v.ensure_string().map(Some)?, "overrides" => flag.overrides = vec![v.ensure_string()?], + "conflicts" => flag.conflicts = vec![v.ensure_string()?], "effect" => { let raw = v.ensure_string()?; match raw.parse() { @@ -246,6 +255,13 @@ impl SpecFlag { "help_heading" => { flag.help_heading = child.arg(0)?.ensure_string().map(Some)?; } + "conflicts" => { + flag.conflicts = child + .ensure_arg_len(1..)? + .args() + .map(|arg| arg.ensure_string()) + .collect::>>()?; + } "overrides" => { flag.overrides = child .ensure_arg_len(1..)? @@ -390,6 +406,16 @@ impl From<&SpecFlag> for KdlNode { } children.nodes_mut().push(overrides); } + if flag.conflicts.len() == 1 { + 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); + let mut conflicts = KdlNode::new("conflicts"); + for target in &flag.conflicts { + conflicts.push(string_entry(None, target)); + } + children.nodes_mut().push(conflicts); + } if let Some(env) = &flag.env { node.push(string_entry(Some("env"), env)); } @@ -554,6 +580,7 @@ impl From<&clap::Arg> for SpecFlag { required, required_if: vec![], required_unless: vec![], + conflicts: vec![], help, help_long, help_md: None, @@ -569,6 +596,8 @@ impl From<&clap::Arg> for SpecFlag { deprecated: None, negate: None, overrides: vec![], + // Filled by the command conversion: clap keeps conflicts on the + // `Command`, not the `Arg`, so an `Arg` alone cannot see them. // clap has no way to express this; consumers set it on the derived // spec (see the effect docs). effect: None, @@ -674,6 +703,55 @@ mod tests { assert_snapshot!("myflag: -f --flag ".parse::().unwrap(), @"myflag: -f --flag "); } + #[test] + fn conflicts_round_trip_and_come_across_from_clap() { + // Both spellings, as `overrides` has: a property for one, a child node for + // several. + let spec: Spec = "flag \"--file \" conflicts=\"--stdin\"\nflag \"--stdin\" {\n conflicts \"--file\" \"--url\"\n}\nflag \"--url \"\n" + .parse() + .unwrap(); + assert_eq!(spec.cmd.flags[0].conflicts, vec!["--stdin".to_string()]); + assert_eq!( + spec.cmd.flags[1].conflicts, + vec!["--file".to_string(), "--url".to_string()] + ); + + let reparsed: Spec = spec.to_string().parse().unwrap(); + assert_eq!(reparsed.cmd.flags[1].conflicts.len(), 2, "{spec}"); + } + + #[cfg(feature = "clap")] + #[test] + fn conflicts_survive_the_clap_bridge() { + // clap has had `conflicts_with` for years and mise declares forty of them; the + // bridge was dropping every one, because clap keeps conflicts on the command + // rather than on the argument. + let cmd = clap::Command::new("ex") + .arg(clap::Arg::new("file").long("file").conflicts_with("stdin")) + .arg(clap::Arg::new("stdin").long("stdin")); + let spec: Spec = (&cmd).into(); + + let file = spec.cmd.flags.iter().find(|f| f.name == "file").unwrap(); + assert_eq!(file.conflicts, vec!["--stdin".to_string()]); + + // Only the declared direction: clap validates a conflict both ways but reports + // it only from the argument that declared it. Recording it once is enough, + // because the check looks at every flag that was given — see the parser test + // that rejects either order. + let stdin = spec.cmd.flags.iter().find(|f| f.name == "stdin").unwrap(); + assert!(stdin.conflicts.is_empty()); + + // A short-only target is named `-q`, since that is the only name it has. + // Taking only the long form dropped the conflict and left the spec accepting a + // combination clap rejects. + let shorts = clap::Command::new("ex") + .arg(clap::Arg::new("loud").long("loud").conflicts_with("quiet")) + .arg(clap::Arg::new("quiet").short('q')); + let spec: Spec = (&shorts).into(); + let loud = spec.cmd.flags.iter().find(|f| f.name == "loud").unwrap(); + assert_eq!(loud.conflicts, vec!["-q".to_string()]); + } + #[test] fn a_serialized_spec_can_always_be_read_back() { // Both of these produced KDL that this crate could not reparse: a node