Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(parser): Improve args_conflicts_with_subcommands experience #5298

Merged
merged 10 commits into from
Jan 11, 2024
62 changes: 36 additions & 26 deletions clap_builder/src/error/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,10 @@ fn write_dynamic_context(

match error.kind() {
ErrorKind::ArgumentConflict => {
let invalid_arg = error.get(ContextKind::InvalidArg);
let prior_arg = error.get(ContextKind::PriorArg);
if let (Some(ContextValue::String(invalid_arg)), Some(prior_arg)) =
(invalid_arg, prior_arg)
{
if ContextValue::String(invalid_arg.clone()) == *prior_arg {
let mut prior_arg = error.get(ContextKind::PriorArg);
if let Some(ContextValue::String(invalid_arg)) = error.get(ContextKind::InvalidArg) {
if Some(&ContextValue::String(invalid_arg.clone())) == prior_arg {
prior_arg = None;
let _ = write!(
styled,
"the argument '{}{invalid_arg}{}' cannot be used multiple times",
Expand All @@ -165,36 +163,48 @@ fn write_dynamic_context(
invalid.render(),
invalid.render_reset()
);
}
} else if let Some(ContextValue::String(invalid_arg)) =
error.get(ContextKind::InvalidSubcommand)
{
let _ = write!(
styled,
"the subcommand '{}{invalid_arg}{}' cannot be used with",
invalid.render(),
invalid.render_reset()
);
} else {
styled.push_str(error.kind().as_str().unwrap());
}

match prior_arg {
ContextValue::Strings(values) => {
styled.push_str(":");
for v in values {
let _ = write!(
styled,
"\n{TAB}{}{v}{}",
invalid.render(),
invalid.render_reset()
);
}
}
ContextValue::String(value) => {
if let Some(prior_arg) = prior_arg {
match prior_arg {
ContextValue::Strings(values) => {
styled.push_str(":");
for v in values {
let _ = write!(
styled,
" '{}{value}{}'",
"\n{TAB}{}{v}{}",
invalid.render(),
invalid.render_reset()
);
}
_ => {
styled.push_str(" one or more of the other specified arguments");
}
}
ContextValue::String(value) => {
let _ = write!(
styled,
" '{}{value}{}'",
invalid.render(),
invalid.render_reset()
);
}
_ => {
styled.push_str(" one or more of the other specified arguments");
}
}
true
} else {
false
}

true
}
ErrorKind::NoEquals => {
let invalid_arg = error.get(ContextKind::InvalidArg);
Expand Down
28 changes: 28 additions & 0 deletions clap_builder/src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,34 @@ impl<F: ErrorFormatter> Error<F> {
err
}

pub(crate) fn subcommand_conflict(
cmd: &Command,
sub: String,
mut others: Vec<String>,
usage: Option<StyledStr>,
) -> Self {
let mut err = Self::new(ErrorKind::ArgumentConflict).with_cmd(cmd);

#[cfg(feature = "error-context")]
{
let others = match others.len() {
0 => ContextValue::None,
1 => ContextValue::String(others.pop().unwrap()),
_ => ContextValue::Strings(others),
};
err = err.extend_context_unchecked([
(ContextKind::InvalidSubcommand, ContextValue::String(sub)),
(ContextKind::PriorArg, others),
]);
if let Some(usage) = usage {
err = err
.insert_context_unchecked(ContextKind::Usage, ContextValue::StyledStr(usage));
}
}

err
}

pub(crate) fn empty_value(cmd: &Command, good_vals: &[String], arg: String) -> Self {
Self::invalid_value(cmd, "".to_owned(), good_vals, arg)
}
Expand Down
37 changes: 32 additions & 5 deletions clap_builder/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,27 @@ impl<'cmd> Parser<'cmd> {
} else {
// Start error processing
let _ = self.resolve_pending(matcher);
return Err(self.match_arg_error(&arg_os, valid_arg_found, trailing_values));
return Err(self.match_arg_error(
&arg_os,
valid_arg_found,
trailing_values,
matcher,
));
}
}

if let Some(ref pos_sc_name) = subcmd_name {
if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found {
return Err(ClapError::subcommand_conflict(
self.cmd,
pos_sc_name.clone(),
matcher
.arg_ids()
.map(|id| self.cmd.find(id).unwrap().to_string())
.collect(),
Usage::new(self.cmd).create_usage_with_title(&[]),
));
}
let sc_name = self
.cmd
.find_subcommand(pos_sc_name)
Expand All @@ -470,6 +486,7 @@ impl<'cmd> Parser<'cmd> {
arg_os: &clap_lex::ParsedArg<'_>,
valid_arg_found: bool,
trailing_values: bool,
matcher: &ArgMatcher,
) -> ClapError {
// If argument follows a `--`
if trailing_values {
Expand All @@ -490,7 +507,19 @@ impl<'cmd> Parser<'cmd> {
&& self.cmd.has_positionals()
&& (arg_os.is_long() || arg_os.is_short());

if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found) {
if self.cmd.has_subcommands() {
if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found {
return ClapError::subcommand_conflict(
self.cmd,
arg_os.display().to_string(),
matcher
.arg_ids()
.map(|id| self.cmd.find(id).unwrap().to_string())
.collect(),
Usage::new(self.cmd).create_usage_with_title(&[]),
);
}

let candidates = suggestions::did_you_mean(
&arg_os.display().to_string(),
self.cmd.all_subcommand_names(),
Expand All @@ -508,9 +537,7 @@ impl<'cmd> Parser<'cmd> {
}

// If the argument must be a subcommand.
if self.cmd.has_subcommands()
&& (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set())
{
if !self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set() {
return ClapError::unrecognized_subcommand(
self.cmd,
arg_os.display().to_string(),
Expand Down
178 changes: 139 additions & 39 deletions tests/builder/conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,145 @@ fn exclusive_with_required() {
cmd.clone().try_get_matches_from(["bug", "--test"]).unwrap();
}

#[test]
#[cfg(feature = "error-context")]
fn option_conflicts_with_subcommand() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub1' cannot be used with '--place <place id>'

Usage: test [OPTIONS]
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(-p --place <"place id"> "Place ID to open"))
.subcommand(Command::new("sub1"));

utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn positional_conflicts_with_subcommand() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub1' cannot be used with '<arg1>'

Usage: test <arg1>
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(<arg1> "some arg"))
.subcommand(Command::new("sub1"));

utils::assert_output(cmd, "test value1 sub1", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_long_flag() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'

Usage: test [OPTIONS]
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub").long_flag("sub"));

utils::assert_output(cmd, "test --hello --sub", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_short_flag() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'

Usage: test [OPTIONS]
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub").short_flag('s'));

utils::assert_output(cmd, "test --hello -s", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn positional_conflicts_with_subcommand_precedent() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '<arg1>'

Usage: test <arg1>
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand_precedence_over_arg(true)
.arg(arg!(<arg1> "some arg"))
.subcommand(Command::new("sub"));

utils::assert_output(cmd, "test hello sub", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_precedent() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'

Usage: test [OPTIONS]
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand_precedence_over_arg(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub"));

utils::assert_output(cmd, "test --hello sub", CONFLICT_ERR, true);
}

#[test]
fn subcommand_conflict_negates_required() {
let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand(Command::new("config"))
.arg(arg!(-p --place <"place id"> "Place ID to open").required(true));

let result = cmd.try_get_matches_from(["test", "config"]);
assert!(
result.is_ok(),
"args_conflicts_with_subcommands should ignore required: {}",
result.unwrap_err()
);
let m = result.unwrap();
assert_eq!(m.subcommand_name().unwrap(), "config");
}

#[test]
fn args_negate_subcommands_one_level() {
let res = Command::new("disablehelp")
Expand Down Expand Up @@ -729,42 +868,3 @@ fn args_negate_subcommands_two_levels() {
Some("sub2")
);
}

#[test]
#[cfg(feature = "error-context")]
fn subcommand_conflict_error_message() {
static CONFLICT_ERR: &str = "\
error: unexpected argument 'sub1' found

Usage: test [OPTIONS]
test <COMMAND>

For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(-p --place <"place id"> "Place ID to open"))
.subcommand(
Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
);

utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true);
}

#[test]
fn subcommand_conflict_negates_required() {
let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand(Command::new("config"))
.arg(arg!(-p --place <"place id"> "Place ID to open").required(true));

let result = cmd.try_get_matches_from(["test", "config"]);
assert!(
result.is_ok(),
"args_conflicts_with_subcommands should ignore required: {}",
result.unwrap_err()
);
let m = result.unwrap();
assert_eq!(m.subcommand_name().unwrap(), "config");
}