From dedbabd402e83d50ec57d369b79157c6aa1ab649 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 4 May 2022 11:05:34 -0500 Subject: [PATCH 1/3] fix(error): Don't duplicate args in usage Gave up trying to decipher the existing logic for safe ways to de-duplicate manually and switched to an `IndexSet` to enforce only one of each argument exists. Fixes #3556 --- src/output/usage.rs | 12 ++++++------ src/parse/validator.rs | 5 ++++- tests/builder/require.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index be2cf23c154..f223d9c127a 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -333,14 +333,14 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { incls: &[Id], matcher: Option<&ArgMatcher>, incl_last: bool, - ) -> Vec { + ) -> IndexSet { debug!( "Usage::get_required_usage_from: incls={:?}, matcher={:?}, incl_last={:?}", incls, matcher.is_some(), incl_last ); - let mut ret_val = Vec::new(); + let mut ret_val = IndexSet::new(); let mut unrolled_reqs = IndexSet::new(); @@ -399,7 +399,7 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { { debug!("Usage::get_required_usage_from:iter:{:?}", a); let arg = self.cmd.find(a).expect(INTERNAL_ERROR_MSG).to_string(); - ret_val.push(arg); + ret_val.insert(arg); } let mut g_vec: Vec = vec![]; for g in unrolled_reqs @@ -423,7 +423,7 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { g_vec.push(elem); } } - ret_val.extend_from_slice(&g_vec); + ret_val.extend(g_vec); let mut pvec = unrolled_reqs .iter() @@ -438,9 +438,9 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { pvec.sort_by_key(|(ind, _)| *ind); // sort by index for (_, p) in pvec { - debug!("Usage::get_required_usage_from:iter:{:?}", p.id); + debug!("Usage::get_required_usage_from:push:{:?}", p.id); if !args_in_groups.contains(&p.id) { - ret_val.push(p.to_string()); + ret_val.insert(p.to_string()); } } diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 5b801de88d3..5d03811d830 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -582,7 +582,10 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { let usg = Usage::new(self.cmd).required(&self.required); - let req_args = usg.get_required_usage_from(&incl, Some(matcher), true); + let req_args = usg + .get_required_usage_from(&incl, Some(matcher), true) + .into_iter() + .collect::>(); debug!( "Validator::missing_required_error: req_args={:#?}", diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 7caa4795135..8136a7dd4f4 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -1345,3 +1345,30 @@ fn required_unless_all_on_default_value() { assert!(result.is_err(), "{:?}", result.unwrap()); } + +#[test] +fn required_error_doesnt_duplicate() { + let cmd = Command::new("Clap-created-USAGE-string-bug") + .arg(Arg::new("a").required(true)) + .arg( + Arg::new("b") + .short('b') + .takes_value(true) + .conflicts_with("c"), + ) + .arg( + Arg::new("c") + .short('c') + .takes_value(true) + .conflicts_with("b"), + ); + const EXPECTED: &str = "\ +error: The argument '-b ' cannot be used with '-c ' + +USAGE: + clap-test -b + +For more information try --help +"; + utils::assert_output(cmd, "clap-test aaa -b bbb -c ccc", EXPECTED, true); +} From f7c2deaa476e74f801bb080fc9de018cec8f43a2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 5 May 2022 12:14:46 -0500 Subject: [PATCH 2/3] fix(help): Don't show hidden arguments for conflicts This makes it consistent with other errors where we show arguments to the user. --- src/parse/validator.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 5d03811d830..92702af6a9a 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -290,6 +290,10 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { let used_filtered: Vec = matcher .arg_names() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) + .filter(|n| { + // Filter out the args we don't want to specify. + self.cmd.find(n).map_or(true, |a| !a.is_hide_set()) + }) .filter(|key| !conflicting_keys.contains(key)) .cloned() .collect(); From e23c786f62d6ecbf5ca6f966fa782d3412db8134 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 5 May 2022 17:02:46 -0500 Subject: [PATCH 3/3] refactor(help): Remove redundant required check With us moving the required de-duplication up a level, it made this check redundant. By removing this check, we're more likely to have an item in the `incls` which forces a smart usage and reduces the chance of an `[ARGS]` or `[OPTIONS]`, so a couple of tests changed. --- src/parse/validator.rs | 4 +--- tests/builder/double_require.rs | 4 ++-- tests/builder/require.rs | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 92702af6a9a..4a29bb3694b 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -601,9 +601,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) .filter(|n| { // Filter out the args we don't want to specify. - self.cmd - .find(n) - .map_or(true, |a| !a.is_hide_set() && !self.required.contains(&a.id)) + self.cmd.find(n).map_or(true, |a| !a.is_hide_set()) }) .cloned() .chain(incl) diff --git a/tests/builder/double_require.rs b/tests/builder/double_require.rs index 30495c5872f..13470cff6c1 100644 --- a/tests/builder/double_require.rs +++ b/tests/builder/double_require.rs @@ -16,7 +16,7 @@ static ONLY_B_ERROR: &str = "error: The following required arguments were not pr -c USAGE: - prog [OPTIONS] -b -c + prog -b -c For more information try --help "; @@ -25,7 +25,7 @@ static ONLY_C_ERROR: &str = "error: The following required arguments were not pr -b USAGE: - prog [OPTIONS] -c -b + prog -c -b For more information try --help "; diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 8136a7dd4f4..f10609df857 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -170,7 +170,7 @@ static POSITIONAL_REQ_IF_VAL: &str = "error: The following required arguments we USAGE: - clap-test [ARGS] + clap-test For more information try --help ";