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.

Sign up for GitHub

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

Jump to bottom

Improve error for args_conflicts_with_subcommands #3939

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve error for args_conflicts_with_subcommands #3939

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03ba0ee
Improve error for `args_conflicts_with_subcommands`
cd-work Jul 15, 2022
9943224
Validate error output
cd-work Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 36 additions & 11 deletions src/error/mod.rs
View file
Open in desktop
Expand Up @@ -297,6 +297,22 @@ impl Error {
])
}

pub(crate) fn argument_subcommand_conflict(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hijacking the ArgumentConflict error here, it would probably make sense to introduce a new error kind specifically for this, but I didn't want to spend hours on error boilerplate with a PR without first getting some feedback on the general approach.

cmd: &Command,
arg: String,
subcmd: String,
usage: String,
) -> Self {
Self::new(ErrorKind::ArgumentConflict)
.with_cmd(cmd)
.set_info(vec![subcmd.clone()])
.extend_context_unchecked([
(ContextKind::InvalidArg, ContextValue::String(arg)),
(ContextKind::InvalidSubcommand, ContextValue::String(subcmd)),
(ContextKind::Usage, ContextValue::String(usage)),
])
}

pub(crate) fn empty_value(cmd: &Command, good_vals: &[&str], arg: String) -> Self {
let info = vec![arg.clone()];
let mut err = Self::new(ErrorKind::EmptyValue)
Expand Down Expand Up @@ -618,33 +634,42 @@ impl Error {
fn write_dynamic_context(&self, c: &mut Colorizer) -> bool {
match self.kind() {
ErrorKind::ArgumentConflict => {
let subcmd = self.get_context(ContextKind::InvalidSubcommand);
let invalid_arg = self.get_context(ContextKind::InvalidArg);
let prior_arg = self.get_context(ContextKind::PriorArg);
if let (Some(ContextValue::String(invalid_arg)), Some(prior_arg)) =
(invalid_arg, prior_arg)
{

if let Some(ContextValue::String(invalid_arg)) = invalid_arg {
c.none("The argument '");
c.warning(invalid_arg);
c.none("' cannot be used with");

match prior_arg {
ContextValue::Strings(values) => {
match (prior_arg, subcmd) {
(Some(ContextValue::Strings(values)), _) => {
c.none(":");
for v in values {
c.none("\n ");
c.warning(&**v);
}
}
ContextValue::String(value) => {
true
},
(Some(ContextValue::String(value)), _) => {
c.none(" '");
c.warning(value);
c.none("'");
}
_ => {
true
},
(_, Some(ContextValue::String(value))) => {
c.none(" subcommand '");
c.warning(value);
c.none("'");
true
},
(Some(_), _) | (_, Some(_)) => {
c.none(" one or more of the other specified arguments");
}
true
},
(None, None) => false
}
true
} else {
false
}
Expand Down
38 changes: 25 additions & 13 deletions src/parser/parser.rs
View file
Open in desktop
Expand Up @@ -62,8 +62,8 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
let mut parse_state = ParseState::ValuesDone;
let mut pos_counter = 1;

// Already met any valid arg(then we shouldn't expect subcommands after it).
let mut valid_arg_found = false;
// Already met this valid arg(then we shouldn't expect subcommands after it).
let mut valid_arg_found = None;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turning this into an Option here gives us the nice option to tell the user which flag caused the subcommand to fail. Ideally they can then remove the listed subcommand and if another error pops up due to extra flags, they can repeat the process until all flags are removed.

// If the user already passed '--'. Meaning only positional args follow.
let mut trailing_values = false;

Expand Down Expand Up @@ -142,7 +142,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
// current value matches the next arg.
self.is_new_arg(&n, arg)
|| self
.possible_subcommand(n.to_value(), valid_arg_found)
.possible_subcommand(n.to_value(), &valid_arg_found)
.is_some()
} else {
true
Expand Down Expand Up @@ -175,7 +175,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
|| !matches!(parse_state, ParseState::Opt(_) | ParseState::Pos(_))
{
// Does the arg match a subcommand name, or any of its aliases (if defined)
let sc_name = self.possible_subcommand(arg_os.to_value(), valid_arg_found);
let sc_name = self.possible_subcommand(arg_os.to_value(), &valid_arg_found);
debug!("Parser::get_matches_with: sc={:?}", sc_name);
if let Some(sc_name) = sc_name {
#[allow(deprecated)]
Expand Down Expand Up @@ -413,7 +413,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
} else {
parse_state = ParseState::Pos(arg.id.clone());
}
valid_arg_found = true;
valid_arg_found = Some(arg.to_string());
} else if let Some(external_parser) =
self.cmd.get_external_subcommand_value_parser().cloned()
{
Expand Down Expand Up @@ -479,14 +479,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
fn match_arg_error(
&self,
arg_os: &clap_lex::ParsedArg<'_>,
valid_arg_found: bool,
valid_arg_found: Option<String>,
trailing_values: bool,
) -> ClapError {
// If argument follows a `--`
if trailing_values {
// If the arg matches a subcommand name, or any of its aliases (if defined)
if self
.possible_subcommand(arg_os.to_value(), valid_arg_found)
.possible_subcommand(arg_os.to_value(), &valid_arg_found)
.is_some()
{
return ClapError::unnecessary_double_dash(
Expand All @@ -496,6 +496,18 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
);
}
}

// Conflicting arg and subcommand due to `args_conflicts_with_subcommands`.
let subcmd_match = self.possible_subcommand(arg_os.to_value(), &None);
if let Some((arg, subcmd)) = valid_arg_found.zip(subcmd_match) {
return ClapError::argument_subcommand_conflict(
self.cmd,
arg,
subcmd.into(),
Usage::new(self.cmd).create_usage_with_title(&[]),
);
}
Comment on lines +500 to +509
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to handle this before the error function, but it seems like it's not possible to error out immediately on the subcommand because it might still be a valid argument.

I'm not entirely sure if this is a bullet-proof way to catch this error, but it's the best solution I could find. So let me know if there's a better place to handle this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of my biggest concerns with trying to improve the error in this situation. With this not being a area of priority at the moment, this means you'll need to help carry a lot of the load in analyzing and verifying this doesn't regress other parts of the user experience.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That much should be covered both by tests and by the conditions checked before this error is emitted though, right?

Do you think there's additional changes necessary to ensure this patch does not have any unwanted side effects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its also thinking of all of the different user cases and considering the error message under each one

for todo --flag sub1 we currently give

error: The argument '--flag' cannot be used with subcommand 'sub1'

What other intentions could the user have for todo --flag sub1?

What other combinations of flags might hit this error but be unrelated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other combinations of flags might hit this error but be unrelated?

I mean hopefully none, that was the goal at least. I couldn't find any.

What other intentions could the user have for todo --flag sub1?

Providing additional information on what the user might want to do is just an improvement on the error message though, right? I wouldn't say there's a good recommendation that can be made here other than removing the flag, which I'd say this error does reasonably well.

Ultimately though even if this error can be improved (maybe even in the future), I think if the question is if we can provide a good enough error here, we should also take into account that the status quo is far worse. So much so that it makes the option unusable (imo).


let candidates = suggestions::did_you_mean(
&arg_os.display().to_string(),
self.cmd.all_subcommand_names(),
Expand Down Expand Up @@ -538,12 +550,12 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
fn possible_subcommand(
&self,
arg: Result<&str, &RawOsStr>,
valid_arg_found: bool,
valid_arg_found: &Option<String>,
) -> Option<&str> {
debug!("Parser::possible_subcommand: arg={:?}", arg);
let arg = arg.ok()?;

if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found) {
if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found.is_some()) {
if self.cmd.is_infer_subcommands_set() {
// For subcommand `test`, we accepts it's prefix: `t`, `te`,
// `tes` and `test`.
Expand Down Expand Up @@ -716,7 +728,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
long_arg: Result<&str, &RawOsStr>,
long_value: Option<&RawOsStr>,
parse_state: &ParseState,
valid_arg_found: &mut bool,
valid_arg_found: &mut Option<String>,
trailing_values: bool,
) -> ClapResult<ParseResult> {
// maybe here lifetime should be 'a
Expand Down Expand Up @@ -767,7 +779,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {

if let Some((_long_arg, arg)) = arg {
let ident = Identifier::Long;
*valid_arg_found = true;
*valid_arg_found = Some(arg.to_string());
if arg.is_takes_value_set() {
debug!(
"Parser::parse_long_arg({:?}): Found an arg with value '{:?}'",
Expand Down Expand Up @@ -821,7 +833,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
parse_state: &ParseState,
// change this to possible pos_arg when removing the usage of &mut Parser.
pos_counter: usize,
valid_arg_found: &mut bool,
valid_arg_found: &mut Option<String>,
trailing_values: bool,
) -> ClapResult<ParseResult> {
debug!("Parser::parse_short_arg: short_arg={:?}", short_arg);
Expand Down Expand Up @@ -889,7 +901,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
"Parser::parse_short_arg:iter:{}: Found valid opt or flag",
c
);
*valid_arg_found = true;
*valid_arg_found = Some(arg.to_string());
if !arg.is_takes_value_set() {
ret =
self.react(Some(ident), ValueSource::CommandLine, arg, vec![], matcher)?;
Expand Down
12 changes: 12 additions & 0 deletions tests/builder/app_settings.rs
View file
Open in desktop
Expand Up @@ -744,6 +744,18 @@ fn args_negate_subcommands_two_levels() {
);
}

#[test]
fn args_conflict_subcommands_error() {
let res = Command::new("todo")
.args_conflicts_with_subcommands(true)
.arg(arg!(--flag))
.subcommand(Command::new("sub1"))
.try_get_matches_from(vec!["", "--flag", "sub1"]);

assert!(res.is_err());
assert_eq!(res.unwrap_err().kind(), ErrorKind::ArgumentConflict);
}
cd-work marked this conversation as resolved.
Show resolved Hide resolved

#[test]
fn propagate_vals_down() {
let m = Command::new("myprog")
Expand Down