From 43c1fa30d0c093cf03553c5ad818137e28a4de4a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:26:30 -0500 Subject: [PATCH 1/7] refactor(parser): Reduce code duplication --- src/parser/arg_matcher.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index 65102c2bc6f..1b04087f0d8 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -153,20 +153,11 @@ impl ArgMatcher { } pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) { - let id = arg.get_id().clone(); - debug!("ArgMatcher::start_occurrence_of_arg: id={:?}", id); - let ma = self.entry(id).or_insert(MatchedArg::new_arg(arg)); - debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id())); - ma.set_source(ValueSource::CommandLine); - ma.new_val_group(); + self.start_custom_arg(arg, ValueSource::CommandLine); } pub(crate) fn start_occurrence_of_group(&mut self, id: Id) { - debug!("ArgMatcher::start_occurrence_of_group: id={:?}", id); - let ma = self.entry(id).or_insert(MatchedArg::new_group()); - debug_assert_eq!(ma.type_id(), None); - ma.set_source(ValueSource::CommandLine); - ma.new_val_group(); + self.start_custom_group(id, ValueSource::CommandLine); } pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) { From 84828e8da97c193731bd7eee7250099a2ad54def Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:28:57 -0500 Subject: [PATCH 2/7] refactor(parser): Remove function wrappers --- src/parser/arg_matcher.rs | 8 -------- src/parser/parser.rs | 4 ++-- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index 1b04087f0d8..93bac7e1734 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -152,14 +152,6 @@ impl ArgMatcher { ma.new_val_group(); } - pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) { - self.start_custom_arg(arg, ValueSource::CommandLine); - } - - pub(crate) fn start_occurrence_of_group(&mut self, id: Id) { - self.start_custom_group(id, ValueSource::CommandLine); - } - pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) { let id = Id::from_static_ref(Id::EXTERNAL); debug!("ArgMatcher::start_occurrence_of_external: id={:?}", id,); diff --git a/src/parser/parser.rs b/src/parser/parser.rs index dec42b77a2b..818ca95c98e 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1509,10 +1509,10 @@ impl<'cmd> Parser<'cmd> { // With each new occurrence, remove overrides from prior occurrences self.remove_overrides(arg, matcher); - matcher.start_occurrence_of_arg(arg); + matcher.start_custom_arg(arg, ValueSource::CommandLine); // Increment or create the group "args" for group in self.cmd.groups_for_arg(arg.get_id()) { - matcher.start_occurrence_of_group(group); + matcher.start_custom_group(group, ValueSource::CommandLine); } } } From 8cf6d116e3e785714b8ecce648ff444f41321cf3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:31:09 -0500 Subject: [PATCH 3/7] refactor(parser): Reduce code duplication --- src/parser/parser.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 818ca95c98e..521c3eecd93 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1506,14 +1506,7 @@ impl<'cmd> Parser<'cmd> { /// Increase occurrence of specific argument and the grouped arg it's in. fn start_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg) { - // With each new occurrence, remove overrides from prior occurrences - self.remove_overrides(arg, matcher); - - matcher.start_custom_arg(arg, ValueSource::CommandLine); - // Increment or create the group "args" - for group in self.cmd.groups_for_arg(arg.get_id()) { - matcher.start_custom_group(group, ValueSource::CommandLine); - } + self.start_custom_arg(matcher, arg, ValueSource::CommandLine); } } From 0f30ac76f15e629bd5e0c5ba093a5138457e4dfb Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:32:21 -0500 Subject: [PATCH 4/7] refactor(parser): Remove function wrappers --- src/parser/parser.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 521c3eecd93..4cbc5c32899 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1503,11 +1503,6 @@ impl<'cmd> Parser<'cmd> { matcher.start_custom_group(group, source); } } - - /// Increase occurrence of specific argument and the grouped arg it's in. - fn start_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg) { - self.start_custom_arg(matcher, arg, ValueSource::CommandLine); - } } // Error, Help, and Version Methods @@ -1542,7 +1537,7 @@ impl<'cmd> Parser<'cmd> { // Add the arg to the matches to build a proper usage string if let Some((name, _)) = did_you_mean.as_ref() { if let Some(arg) = self.cmd.get_keymap().get(&name.as_ref()) { - self.start_occurrence_of_arg(matcher, arg); + self.start_custom_arg(matcher, arg, ValueSource::CommandLine); } } From e704adb4ff55d26190b7d1b524ec899083e7f42d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:34:19 -0500 Subject: [PATCH 5/7] refactor(parser): Centralize group handling Since groups are only associated with the occurrence, we can move the assigning of it to the occurrence start. This will help centralize other checks --- src/parser/parser.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 4cbc5c32899..62471334e50 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1078,15 +1078,6 @@ impl<'cmd> Parser<'cmd> { matcher.add_index_to(arg.get_id(), self.cur_idx.get()); } - // Increment or create the group "args" - for group in self.cmd.groups_for_arg(arg.get_id()) { - matcher.add_val_to( - &group, - AnyValue::new(arg.get_id().clone()), - OsString::from(arg.get_id().as_str()), - ); - } - Ok(()) } @@ -1500,7 +1491,12 @@ impl<'cmd> Parser<'cmd> { } matcher.start_custom_arg(arg, source); for group in self.cmd.groups_for_arg(arg.get_id()) { - matcher.start_custom_group(group, source); + matcher.start_custom_group(group.clone(), source); + matcher.add_val_to( + &group, + AnyValue::new(arg.get_id().clone()), + OsString::from(arg.get_id().as_str()), + ); } } } From e98fc7f6ebc0cb54a6357bf01d1387a4860f17b6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:46:39 -0500 Subject: [PATCH 6/7] refactor(parser): Centralize knowledge for explicit --- src/parser/matches/matched_arg.rs | 2 +- src/parser/matches/value_source.rs | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs index 22d5b414c14..6a80625f856 100644 --- a/src/parser/matches/matched_arg.rs +++ b/src/parser/matches/matched_arg.rs @@ -130,7 +130,7 @@ impl MatchedArg { } pub(crate) fn check_explicit(&self, predicate: &ArgPredicate) -> bool { - if self.source == Some(ValueSource::DefaultValue) { + if self.source.map(|s| !s.is_explicit()).unwrap_or(false) { return false; } diff --git a/src/parser/matches/value_source.rs b/src/parser/matches/value_source.rs index fb762d2af68..db45d9c0fd0 100644 --- a/src/parser/matches/value_source.rs +++ b/src/parser/matches/value_source.rs @@ -9,3 +9,9 @@ pub enum ValueSource { /// Value was passed in on the command-line CommandLine, } + +impl ValueSource { + pub(crate) fn is_explicit(self) -> bool { + self != Self::DefaultValue + } +} From d0dcaac2aba81852b2e3f03ad3390799a9ceac90 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 12 Oct 2022 07:39:55 -0500 Subject: [PATCH 7/7] fix(parser): Only add ArgGroup to ArgMatches for command-line This will fix `clap_derive`s behavior for optional-flattened groups as it will properly detect when the group is present (#3566). While I consider this a bug and not part of compatibility guarentees, I still want to keep in mind user impact which could still prevent this. Defaults will make the group always-present which has little value and if anything is relying on this, it is probably an application bug. --- src/parser/parser.rs | 16 +++++++++------- tests/builder/groups.rs | 9 ++++++--- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 62471334e50..9a224cd8849 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1490,13 +1490,15 @@ impl<'cmd> Parser<'cmd> { self.remove_overrides(arg, matcher); } matcher.start_custom_arg(arg, source); - for group in self.cmd.groups_for_arg(arg.get_id()) { - matcher.start_custom_group(group.clone(), source); - matcher.add_val_to( - &group, - AnyValue::new(arg.get_id().clone()), - OsString::from(arg.get_id().as_str()), - ); + if source.is_explicit() { + for group in self.cmd.groups_for_arg(arg.get_id()) { + matcher.start_custom_group(group.clone(), source); + matcher.add_val_to( + &group, + AnyValue::new(arg.get_id().clone()), + OsString::from(arg.get_id().as_str()), + ); + } } } } diff --git a/tests/builder/groups.rs b/tests/builder/groups.rs index 17bd9c37b7a..2150a2b6272 100644 --- a/tests/builder/groups.rs +++ b/tests/builder/groups.rs @@ -73,9 +73,10 @@ fn group_single_value() { #[test] fn group_empty() { let res = Command::new("group") + .arg(arg!(-f --flag "some flag")) .arg(arg!(-c --color [color] "some option")) .arg(arg!(-n --hostname "another option")) - .group(ArgGroup::new("grp").args(["hostname", "color"])) + .group(ArgGroup::new("grp").args(["hostname", "color", "flag"])) .try_get_matches_from(vec![""]); assert!(res.is_ok(), "{}", res.unwrap_err()); @@ -87,12 +88,13 @@ fn group_empty() { #[test] fn group_required_flags_empty() { let result = Command::new("group") + .arg(arg!(-f --flag "some flag")) .arg(arg!(-c --color "some option")) .arg(arg!(-n --hostname "another option")) .group( ArgGroup::new("grp") .required(true) - .args(["hostname", "color"]), + .args(["hostname", "color", "flag"]), ) .try_get_matches_from(vec![""]); assert!(result.is_err()); @@ -103,9 +105,10 @@ fn group_required_flags_empty() { #[test] fn group_multi_value_single_arg() { let res = Command::new("group") + .arg(arg!(-f --flag "some flag")) .arg(arg!(-c --color "some option").num_args(1..)) .arg(arg!(-n --hostname "another option")) - .group(ArgGroup::new("grp").args(["hostname", "color"])) + .group(ArgGroup::new("grp").args(["hostname", "color", "flag"])) .try_get_matches_from(vec!["", "-c", "blue", "red", "green"]); assert!(res.is_ok(), "{:?}", res.unwrap_err().kind());