Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/usage.usage.kdl
Original file line number Diff line number Diff line change
Expand Up @@ -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 <FILE>
}
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 <OUT_DIR>
Expand Down
2 changes: 2 additions & 0 deletions conformance/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion conformance/src/reference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
138 changes: 127 additions & 11 deletions corpus/08-choices-and-required.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,83 +7,199 @@
"doc": "A value among the declared choices is accepted.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--shell <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 <shell>\" {\n choices \"bash\" \"zsh\" \"fish\"\n}\n",
"argv": ["--shell", "csh"],
"expect": { "error": "invalid_choice" },
"expect": {
"error": "invalid_choice"
},
"layer": "post-binding"
},
{
"id": "choice-is-case-sensitive",
"doc": "Choices match exactly. Case-insensitive matching would have to be declared, not assumed.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--shell <shell>\" {\n choices \"bash\" \"zsh\"\n}\n",
"argv": ["--shell", "ZSH"],
"expect": { "error": "invalid_choice" },
"expect": {
"error": "invalid_choice"
},
"layer": "post-binding"
},
{
"id": "choice-on-arg",
"doc": "Positionals carry choices too.",
"spec": "name \"ex\"\nbin \"ex\"\narg \"<shell>\" {\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 <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 <file>\" required=#true\n",
"argv": [],
"expect": { "error": "missing_required_flag" },
"expect": {
"error": "missing_required_flag"
},
"layer": "post-binding"
},
{
"id": "required-unless-satisfied-by-other",
"doc": "`required_unless` is satisfied when the named alternative is present.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--file <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 <file>\" required_unless=\"--stdin\"\nflag \"--stdin\"\n",
"argv": [],
"expect": { "error": "missing_required_flag" },
"expect": {
"error": "missing_required_flag"
},
"layer": "post-binding"
},
{
"id": "overrides-last-wins",
"doc": "Two flags that override each other leave only the last one given.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--file <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 <file>\" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url <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 <file>\" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url <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 <file>\" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url <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 <file>\" conflicts=\"--stdin\"\nflag \"--stdin\"\nflag \"--url <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 <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 <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"
},
{
"id": "flag-var-too-few",
"doc": "`var_min` is enforced for a repeatable flag, not only for positionals.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--include <pattern>\" var=#true var_min=2\n",
"argv": ["--include", "a"],
"expect": { "error": "var_too_few" },
"expect": {
"error": "var_too_few"
},
"layer": "post-binding"
},
{
"id": "flag-var-too-many",
"doc": "`var_max` is likewise enforced for a repeatable flag.",
"spec": "name \"ex\"\nbin \"ex\"\nflag \"--include <pattern>\" var=#true var_max=1\n",
"argv": ["--include", "a", "--include", "b"],
"expect": { "error": "var_too_many" },
"expect": {
"error": "var_too_many"
},
"layer": "post-binding"
}
]
Expand Down
3 changes: 2 additions & 1 deletion docs/cli/reference/commands.json
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,8 @@
"short": ["m"],
"long": ["multi"],
"hide": false,
"global": false
"global": false,
"conflicts": ["--out-file"]
},
{
"name": "html-encode",
Expand Down
4 changes: 3 additions & 1 deletion docs/spec/argv.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down
16 changes: 16 additions & 0 deletions docs/spec/reference/flag.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ flag "--dir <dir>" // args named "<dir>" will be completed as directories
flag "--file <file>" required_if="--dir" // if --dir is set, --file must also be set
flag "--file <file>" required_unless="--dir" // either --file or --dir must be present
flag "--file <file>" overrides="--stdin" // --file and --stdin override each other; the last one wins
flag "--file <file>" conflicts="--stdin" // --file and --stdin cannot be given together

flag "--stdin" {
conflicts "--file" "--url" // several, one per argument
}

flag "--shell <shell>" {
choices "bash" "zsh" "fish" // <shell> must be one of the choices
Expand All @@ -53,6 +58,17 @@ flag "--file <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
Expand Down
54 changes: 54 additions & 0 deletions lib/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2531,6 +2559,32 @@ flag "--file <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 <f>\" 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
Expand Down
23 changes: 22 additions & 1 deletion lib/src/spec/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 `--<name>` would
// be a selector matching nothing, which is worse: it reads as a
// relationship that holds.
(None, None) => None,
})
.collect();
Comment thread
cursor[bot] marked this conversation as resolved.
spec.flags.push(flag)
}
}
spec.subcommand_required = cmd.is_subcommand_required_set();
Expand Down
Loading