From dacc8ff43a091b676096d070bf75a231428d4b5f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 16:59:58 -0500 Subject: [PATCH 01/13] refactor(parser): Consolidate presence check --- src/parser/validator.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 1a8bbdce7f8..49a91684616 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -280,10 +280,12 @@ impl<'cmd> Validator<'cmd> { // Validate the conditionally required args for a in self.cmd.get_arguments() { + if matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) { + continue; + } + for (other, val) in &a.r_ifs { - if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) - && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) - { + if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) { return self.missing_required_error(matcher, vec![a.id.clone()]); } } @@ -291,10 +293,7 @@ impl<'cmd> Validator<'cmd> { let match_all = a.r_ifs_all.iter().all(|(other, val)| { matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) }); - if match_all - && !a.r_ifs_all.is_empty() - && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) - { + if match_all && !a.r_ifs_all.is_empty() { return self.missing_required_error(matcher, vec![a.id.clone()]); } } From 3c466c63f7778d9881fd55ec5e4876450103c855 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 17:02:05 -0500 Subject: [PATCH 02/13] refactor(parser): Flatten required_unless logic --- src/parser/validator.rs | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 49a91684616..e3baaeefd29 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -298,7 +298,20 @@ impl<'cmd> Validator<'cmd> { } } - self.validate_required_unless(matcher)?; + debug!("Validator::validate_required_unless"); + let failed_args: Vec<_> = self + .cmd + .get_arguments() + .filter(|&a| { + (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) + && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) + && self.fails_arg_required_unless(a, matcher) + }) + .map(|a| a.id.clone()) + .collect(); + if !failed_args.is_empty() { + self.missing_required_error(matcher, failed_args)?; + } Ok(()) } @@ -314,25 +327,6 @@ impl<'cmd> Validator<'cmd> { !conflicts.is_empty() } - fn validate_required_unless(&self, matcher: &ArgMatcher) -> ClapResult<()> { - debug!("Validator::validate_required_unless"); - let failed_args: Vec<_> = self - .cmd - .get_arguments() - .filter(|&a| { - (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) - && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) - && self.fails_arg_required_unless(a, matcher) - }) - .map(|a| a.id.clone()) - .collect(); - if failed_args.is_empty() { - Ok(()) - } else { - self.missing_required_error(matcher, failed_args) - } - } - // Failing a required unless means, the arg's "unless" wasn't present, and neither were they fn fails_arg_required_unless(&self, a: &Arg, matcher: &ArgMatcher) -> bool { debug!("Validator::fails_arg_required_unless: a={:?}", a.get_id()); From 14d910dee1e4221f9bd2017ff156bbd8042a5d44 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 17:04:17 -0500 Subject: [PATCH 03/13] refactor(parser): Prefer explicit for loops --- src/parser/validator.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index e3baaeefd29..f84cb0dad9b 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -299,16 +299,15 @@ impl<'cmd> Validator<'cmd> { } debug!("Validator::validate_required_unless"); - let failed_args: Vec<_> = self - .cmd - .get_arguments() - .filter(|&a| { - (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) - && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) - && self.fails_arg_required_unless(a, matcher) - }) - .map(|a| a.id.clone()) - .collect(); + let mut failed_args = Vec::new(); + for a in self.cmd.get_arguments() { + if (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) + && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) + && self.fails_arg_required_unless(a, matcher) + { + failed_args.push(a.id.clone()); + } + } if !failed_args.is_empty() { self.missing_required_error(matcher, failed_args)?; } From 0c9b9d7ec8814db7ab2a9a925a9617b4431f2166 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 19:37:29 -0500 Subject: [PATCH 04/13] fix(parser): Show all required errors at once This also has the side effect of always using the "smart usage" which is why the tests changed. --- examples/derive_ref/custom-bool.md | 2 +- examples/tutorial_builder/04_03_relations.md | 2 +- examples/tutorial_derive/04_03_relations.md | 2 +- src/parser/validator.rs | 39 +++++++++++++++----- tests/builder/default_vals.rs | 2 +- tests/builder/require.rs | 4 +- 6 files changed, 36 insertions(+), 15 deletions(-) diff --git a/examples/derive_ref/custom-bool.md b/examples/derive_ref/custom-bool.md index f8a5bd86b20..3586af567eb 100644 --- a/examples/derive_ref/custom-bool.md +++ b/examples/derive_ref/custom-bool.md @@ -26,7 +26,7 @@ error: The following required arguments were not provided: Usage: - custom-bool[EXE] [OPTIONS] --foo + custom-bool[EXE] --foo For more information try --help diff --git a/examples/tutorial_builder/04_03_relations.md b/examples/tutorial_builder/04_03_relations.md index 0ff757684ec..aad5c7cb192 100644 --- a/examples/tutorial_builder/04_03_relations.md +++ b/examples/tutorial_builder/04_03_relations.md @@ -25,7 +25,7 @@ error: The following required arguments were not provided: <--set-ver |--major|--minor|--patch> Usage: - 04_03_relations[EXE] [OPTIONS] <--set-ver |--major|--minor|--patch> [INPUT_FILE] + 04_03_relations[EXE] <--set-ver |--major|--minor|--patch> For more information try --help diff --git a/examples/tutorial_derive/04_03_relations.md b/examples/tutorial_derive/04_03_relations.md index dba580b5ebd..b4a69aacff4 100644 --- a/examples/tutorial_derive/04_03_relations.md +++ b/examples/tutorial_derive/04_03_relations.md @@ -25,7 +25,7 @@ error: The following required arguments were not provided: <--set-ver |--major|--minor|--patch> Usage: - 04_03_relations_derive[EXE] [OPTIONS] <--set-ver |--major|--minor|--patch> [INPUT_FILE] + 04_03_relations_derive[EXE] <--set-ver |--major|--minor|--patch> For more information try --help diff --git a/src/parser/validator.rs b/src/parser/validator.rs index f84cb0dad9b..30d74d86c74 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -240,6 +240,8 @@ impl<'cmd> Validator<'cmd> { debug!("Validator::validate_required: required={:?}", self.required); self.gather_requires(matcher); + let mut missing_required = Vec::new(); + let is_exclusive_present = matcher .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) @@ -263,7 +265,11 @@ impl<'cmd> Validator<'cmd> { if let Some(arg) = self.cmd.find(arg_or_group) { debug!("Validator::validate_required:iter: This is an arg"); if !is_exclusive_present && !self.is_missing_required_ok(arg, matcher, conflicts) { - return self.missing_required_error(matcher, vec![]); + debug!( + "Validator::validate_required:iter: Missing {:?}", + arg.get_id() + ); + missing_required.push(arg.get_id().clone()); } } else if let Some(group) = self.cmd.find_group(arg_or_group) { debug!("Validator::validate_required:iter: This is a group"); @@ -273,7 +279,11 @@ impl<'cmd> Validator<'cmd> { .iter() .any(|a| matcher.check_explicit(a, &ArgPredicate::IsPresent)) { - return self.missing_required_error(matcher, vec![]); + debug!( + "Validator::validate_required:iter: Missing {:?}", + group.get_id() + ); + missing_required.push(group.get_id().clone()); } } } @@ -286,7 +296,11 @@ impl<'cmd> Validator<'cmd> { for (other, val) in &a.r_ifs { if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) { - return self.missing_required_error(matcher, vec![a.id.clone()]); + debug!( + "Validator::validate_required:iter: Missing {:?}", + a.get_id() + ); + missing_required.push(a.get_id().clone()); } } @@ -294,22 +308,29 @@ impl<'cmd> Validator<'cmd> { matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) }); if match_all && !a.r_ifs_all.is_empty() { - return self.missing_required_error(matcher, vec![a.id.clone()]); + debug!( + "Validator::validate_required:iter: Missing {:?}", + a.get_id() + ); + missing_required.push(a.get_id().clone()); } } - debug!("Validator::validate_required_unless"); - let mut failed_args = Vec::new(); for a in self.cmd.get_arguments() { if (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) && self.fails_arg_required_unless(a, matcher) { - failed_args.push(a.id.clone()); + debug!( + "Validator::validate_required:iter: Missing {:?}", + a.get_id() + ); + missing_required.push(a.id.clone()); } } - if !failed_args.is_empty() { - self.missing_required_error(matcher, failed_args)?; + + if !missing_required.is_empty() { + self.missing_required_error(matcher, missing_required)?; } Ok(()) diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs index 6c32e614e94..3f04b6b9657 100644 --- a/tests/builder/default_vals.rs +++ b/tests/builder/default_vals.rs @@ -732,7 +732,7 @@ fn default_vals_donnot_show_in_smart_usage() { Usage: - bug [OPTIONS] + bug For more information try --help ", diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 6aeca5d9b1f..92ea15dbcee 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -141,7 +141,7 @@ static POSITIONAL_REQ: &str = "error: The following required arguments were not Usage: - clap-test [ARGS] + clap-test For more information try --help "; @@ -160,7 +160,7 @@ static POSITIONAL_REQ_IF_NO_VAL: &str = "error: The following required arguments Usage: - clap-test [ARGS] + clap-test For more information try --help "; From 26ea3e32d0f864524cb0041e9823c070475736db Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 19:42:21 -0500 Subject: [PATCH 05/13] refactor(parser): Consolidate the loops --- src/parser/validator.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 30d74d86c74..61ea4137e75 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -314,11 +314,8 @@ impl<'cmd> Validator<'cmd> { ); missing_required.push(a.get_id().clone()); } - } - for a in self.cmd.get_arguments() { if (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) - && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) && self.fails_arg_required_unless(a, matcher) { debug!( From ecb632f4878238241c7f131a57c2c27ef3bac306 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:12:01 -0500 Subject: [PATCH 06/13] refactor(usage): Split out group processing --- src/output/usage.rs | 51 ++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index a509c8fe14c..0614238e948 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -396,30 +396,9 @@ impl<'cmd> Usage<'cmd> { ); let mut required_groups_members = FlatSet::new(); - let mut required_opts = FlatSet::new(); let mut required_groups = FlatSet::new(); - let mut required_positionals = FlatSet::new(); for req in unrolled_reqs.iter().chain(incls.iter()) { - if let Some(arg) = self.cmd.find(req) { - let is_present = matcher - .map(|m| m.check_explicit(req, &ArgPredicate::IsPresent)) - .unwrap_or(false); - debug!( - "Usage::get_required_usage_from:iter:{:?} arg is_present={}", - req, is_present - ); - if !is_present { - if arg.is_positional() { - if incl_last || !arg.is_last_set() { - required_positionals - .insert((arg.index.unwrap(), arg.stylized(Some(true)))); - } - } else { - required_opts.insert(arg.stylized(Some(true))); - } - } - } else { - debug_assert!(self.cmd.find_group(req).is_some()); + if self.cmd.find_group(req).is_some() { let group_members = self.cmd.unroll_args_in_group(req); let is_present = matcher .map(|m| { @@ -442,6 +421,34 @@ impl<'cmd> Usage<'cmd> { .map(|arg| arg.stylized(Some(true))), ); } + } else { + debug_assert!(self.cmd.find(req).is_some()); + } + } + + let mut required_opts = FlatSet::new(); + let mut required_positionals = FlatSet::new(); + for req in unrolled_reqs.iter().chain(incls.iter()) { + if let Some(arg) = self.cmd.find(req) { + let is_present = matcher + .map(|m| m.check_explicit(req, &ArgPredicate::IsPresent)) + .unwrap_or(false); + debug!( + "Usage::get_required_usage_from:iter:{:?} arg is_present={}", + req, is_present + ); + if !is_present { + if arg.is_positional() { + if incl_last || !arg.is_last_set() { + required_positionals + .insert((arg.index.unwrap(), arg.stylized(Some(true)))); + } + } else { + required_opts.insert(arg.stylized(Some(true))); + } + } + } else { + debug_assert!(self.cmd.find_group(req).is_some()); } } From 3b17f7fdc9588f19c8d178da2caa0caea6aafc3d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:16:09 -0500 Subject: [PATCH 07/13] refactor(usage): Pull out duplicate check --- src/output/usage.rs | 20 ++++++++++---------- src/util/flat_map.rs | 2 ++ src/util/flat_set.rs | 2 ++ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 0614238e948..3df05125a3b 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -440,11 +440,17 @@ impl<'cmd> Usage<'cmd> { if !is_present { if arg.is_positional() { if incl_last || !arg.is_last_set() { - required_positionals - .insert((arg.index.unwrap(), arg.stylized(Some(true)))); + let stylized = arg.stylized(Some(true)); + if !required_groups_members.contains(&stylized) { + let index = arg.index.unwrap(); + required_positionals.insert((index, stylized)); + } } } else { - required_opts.insert(arg.stylized(Some(true))); + let stylized = arg.stylized(Some(true)); + if !required_groups_members.contains(&stylized) { + required_opts.insert(stylized); + } } } } else { @@ -453,17 +459,11 @@ impl<'cmd> Usage<'cmd> { } let mut ret_val = Vec::new(); - - required_opts.retain(|arg| !required_groups_members.contains(arg)); ret_val.extend(required_opts); - ret_val.extend(required_groups); - required_positionals.sort_by_key(|(ind, _)| *ind); // sort by index for (_, p) in required_positionals { - if !required_groups_members.contains(&p) { - ret_val.push(p); - } + ret_val.push(p); } debug!("Usage::get_required_usage_from: ret_val={:?}", ret_val); diff --git a/src/util/flat_map.rs b/src/util/flat_map.rs index 181950829cf..07e418bf28b 100644 --- a/src/util/flat_map.rs +++ b/src/util/flat_map.rs @@ -1,3 +1,5 @@ +#![allow(dead_code)] + use std::borrow::Borrow; /// Flat (Vec) backed map diff --git a/src/util/flat_set.rs b/src/util/flat_set.rs index db4381710b8..3e0b23dae35 100644 --- a/src/util/flat_set.rs +++ b/src/util/flat_set.rs @@ -1,3 +1,5 @@ +#![allow(dead_code)] + use std::borrow::Borrow; /// Flat (Vec) backed set From 85569514db644cc4d5835790212b4e6792b3ce51 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:18:35 -0500 Subject: [PATCH 08/13] refactor(usage): Make duplicate check more resilient --- src/output/usage.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 3df05125a3b..df32f34dab8 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -414,12 +414,7 @@ impl<'cmd> Usage<'cmd> { if !is_present { let elem = self.cmd.format_group(req); required_groups.insert(elem); - required_groups_members.extend( - group_members - .iter() - .flat_map(|id| self.cmd.find(id)) - .map(|arg| arg.stylized(Some(true))), - ); + required_groups_members.extend(group_members); } } else { debug_assert!(self.cmd.find(req).is_some()); @@ -441,14 +436,14 @@ impl<'cmd> Usage<'cmd> { if arg.is_positional() { if incl_last || !arg.is_last_set() { let stylized = arg.stylized(Some(true)); - if !required_groups_members.contains(&stylized) { + if !required_groups_members.contains(arg.get_id()) { let index = arg.index.unwrap(); required_positionals.insert((index, stylized)); } } } else { let stylized = arg.stylized(Some(true)); - if !required_groups_members.contains(&stylized) { + if !required_groups_members.contains(arg.get_id()) { required_opts.insert(stylized); } } From 09031fb4abe887f7bee5406223cc58751fc68c7e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:22:47 -0500 Subject: [PATCH 09/13] refactor(usage): Consolidate required bookkeeping --- src/output/usage.rs | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index df32f34dab8..e6a24cadd20 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -411,11 +411,13 @@ impl<'cmd> Usage<'cmd> { "Usage::get_required_usage_from:iter:{:?} group is_present={}", req, is_present ); - if !is_present { - let elem = self.cmd.format_group(req); - required_groups.insert(elem); - required_groups_members.extend(group_members); + if is_present { + continue; } + + let elem = self.cmd.format_group(req); + required_groups.insert(elem); + required_groups_members.extend(group_members); } else { debug_assert!(self.cmd.find(req).is_some()); } @@ -425,6 +427,10 @@ impl<'cmd> Usage<'cmd> { let mut required_positionals = FlatSet::new(); for req in unrolled_reqs.iter().chain(incls.iter()) { if let Some(arg) = self.cmd.find(req) { + if required_groups_members.contains(arg.get_id()) { + continue; + } + let is_present = matcher .map(|m| m.check_explicit(req, &ArgPredicate::IsPresent)) .unwrap_or(false); @@ -432,21 +438,18 @@ impl<'cmd> Usage<'cmd> { "Usage::get_required_usage_from:iter:{:?} arg is_present={}", req, is_present ); - if !is_present { - if arg.is_positional() { - if incl_last || !arg.is_last_set() { - let stylized = arg.stylized(Some(true)); - if !required_groups_members.contains(arg.get_id()) { - let index = arg.index.unwrap(); - required_positionals.insert((index, stylized)); - } - } - } else { - let stylized = arg.stylized(Some(true)); - if !required_groups_members.contains(arg.get_id()) { - required_opts.insert(stylized); - } + if is_present { + continue; + } + + let stylized = arg.stylized(Some(true)); + if arg.is_positional() { + if incl_last || !arg.is_last_set() { + let index = arg.index.unwrap(); + required_positionals.insert((index, stylized)); } + } else { + required_opts.insert(stylized); } } else { debug_assert!(self.cmd.find_group(req).is_some()); From 66957f839800f00849f1258d3b95aabdbd0d47f1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:26:08 -0500 Subject: [PATCH 10/13] refactor(usage): Make last check clearer --- src/output/usage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index e6a24cadd20..2fc7d54f94e 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -444,7 +444,7 @@ impl<'cmd> Usage<'cmd> { let stylized = arg.stylized(Some(true)); if arg.is_positional() { - if incl_last || !arg.is_last_set() { + if !arg.is_last_set() || incl_last { let index = arg.index.unwrap(); required_positionals.insert((index, stylized)); } From 619b1fb1872aa33039fc63d79d999e2cdef2dead Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 29 Aug 2022 21:46:57 -0500 Subject: [PATCH 11/13] perF(usage): Defer de-duplicating to later This should drop code size --- src/output/usage.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 2fc7d54f94e..2fe2f36a5ec 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -365,7 +365,7 @@ impl<'cmd> Usage<'cmd> { &required_owned }; - let mut unrolled_reqs = FlatSet::new(); + let mut unrolled_reqs = Vec::new(); for a in required.iter() { let is_relevant = |(val, req_arg): &(ArgPredicate, Id)| -> Option { let required = match val { @@ -384,11 +384,11 @@ impl<'cmd> Usage<'cmd> { for aa in self.cmd.unroll_arg_requires(is_relevant, a) { // if we don't check for duplicates here this causes duplicate error messages // see https://github.com/clap-rs/clap/issues/2770 - unrolled_reqs.insert(aa); + unrolled_reqs.push(aa); } // always include the required arg itself. it will not be enumerated // by unroll_requirements_for_arg. - unrolled_reqs.insert(a.clone()); + unrolled_reqs.push(a.clone()); } debug!( "Usage::get_required_usage_from: unrolled_reqs={:?}", From 2388ace47bc3635f9acbe7b44ed736c2d7ca9d24 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Aug 2022 08:47:03 -0500 Subject: [PATCH 12/13] refactor(parser): Hoist check into iteration --- src/parser/validator.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 61ea4137e75..35840e70aea 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -289,11 +289,11 @@ impl<'cmd> Validator<'cmd> { } // Validate the conditionally required args - for a in self.cmd.get_arguments() { - if matcher.check_explicit(&a.id, &ArgPredicate::IsPresent) { - continue; - } - + for a in self + .cmd + .get_arguments() + .filter(|a| !matcher.check_explicit(a.get_id(), &ArgPredicate::IsPresent)) + { for (other, val) in &a.r_ifs { if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) { debug!( From 8da1f085dd3932002f88f969bbb3a206b863ba26 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Aug 2022 09:31:46 -0500 Subject: [PATCH 13/13] fix(parser): Require earlier, not-present positionals --- src/parser/validator.rs | 36 +++++++++++++++++++++++++++++++++--- tests/builder/require.rs | 3 ++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 35840e70aea..4703cb504d5 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -241,6 +241,7 @@ impl<'cmd> Validator<'cmd> { self.gather_requires(matcher); let mut missing_required = Vec::new(); + let mut highest_index = 0; let is_exclusive_present = matcher .arg_ids() @@ -270,6 +271,9 @@ impl<'cmd> Validator<'cmd> { arg.get_id() ); missing_required.push(arg.get_id().clone()); + if !arg.is_last_set() { + highest_index = highest_index.max(arg.get_index().unwrap_or(0)); + } } } else if let Some(group) = self.cmd.find_group(arg_or_group) { debug!("Validator::validate_required:iter: This is a group"); @@ -294,13 +298,15 @@ impl<'cmd> Validator<'cmd> { .get_arguments() .filter(|a| !matcher.check_explicit(a.get_id(), &ArgPredicate::IsPresent)) { + let mut required = false; + for (other, val) in &a.r_ifs { if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) { debug!( "Validator::validate_required:iter: Missing {:?}", a.get_id() ); - missing_required.push(a.get_id().clone()); + required = true; } } @@ -312,7 +318,7 @@ impl<'cmd> Validator<'cmd> { "Validator::validate_required:iter: Missing {:?}", a.get_id() ); - missing_required.push(a.get_id().clone()); + required = true; } if (!a.r_unless.is_empty() || !a.r_unless_all.is_empty()) @@ -322,7 +328,31 @@ impl<'cmd> Validator<'cmd> { "Validator::validate_required:iter: Missing {:?}", a.get_id() ); - missing_required.push(a.id.clone()); + required = true; + } + + if required { + missing_required.push(a.get_id().clone()); + if !a.is_last_set() { + highest_index = highest_index.max(a.get_index().unwrap_or(0)); + } + } + } + + // For display purposes, include all of the preceding positional arguments + if !self.cmd.is_allow_missing_positional_set() { + for pos in self + .cmd + .get_positionals() + .filter(|a| !matcher.check_explicit(a.get_id(), &ArgPredicate::IsPresent)) + { + if pos.get_index() < Some(highest_index) { + debug!( + "Validator::validate_required:iter: Missing {:?}", + pos.get_id() + ); + missing_required.push(pos.get_id().clone()); + } } } diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 92ea15dbcee..fd9304597cb 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -33,10 +33,11 @@ For more information try --help static MISSING_REQ: &str = "error: The following required arguments were not provided: --long-option-2 + Usage: - clap-test --long-option-2 -F + clap-test --long-option-2 -F For more information try --help ";