Merge pull request #4148 from epage/collect

fix(parser): Improve required error messages

examples/derive_ref/custom-bool.md

@@ -26,7 +26,7 @@ error: The following required arguments were not provided:
     <BOOM>
 
 Usage:
-    custom-bool[EXE] [OPTIONS] --foo <FOO> <BOOM>
+    custom-bool[EXE] --foo <FOO> <BOOM>
 
 For more information try --help
 

examples/tutorial_builder/04_03_relations.md

@@ -25,7 +25,7 @@ error: The following required arguments were not provided:
     <--set-ver <VER>|--major|--minor|--patch>
 
 Usage:
-    04_03_relations[EXE] [OPTIONS] <--set-ver <VER>|--major|--minor|--patch> [INPUT_FILE]
+    04_03_relations[EXE] <--set-ver <VER>|--major|--minor|--patch>
 
 For more information try --help
 

examples/tutorial_derive/04_03_relations.md

@@ -25,7 +25,7 @@ error: The following required arguments were not provided:
     <--set-ver <VER>|--major|--minor|--patch>
 
 Usage:
-    04_03_relations_derive[EXE] [OPTIONS] <--set-ver <VER>|--major|--minor|--patch> [INPUT_FILE]
+    04_03_relations_derive[EXE] <--set-ver <VER>|--major|--minor|--patch>
 
 For more information try --help
 

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<Id> {
                 let required = match val {
@@ -384,42 +384,21 @@ 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={:?}",
             unrolled_reqs
         );
 
         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| {
@@ -432,31 +411,57 @@ 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
-                            .iter()
-                            .flat_map(|id| self.cmd.find(id))
-                            .map(|arg| arg.stylized(Some(true))),
-                    );
+                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());
             }
         }
 
-        let mut ret_val = Vec::new();
+        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) {
+                if required_groups_members.contains(arg.get_id()) {
+                    continue;
+                }
 
-        required_opts.retain(|arg| !required_groups_members.contains(arg));
-        ret_val.extend(required_opts);
+                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 {
+                    continue;
+                }
 
-        ret_val.extend(required_groups);
+                let stylized = arg.stylized(Some(true));
+                if arg.is_positional() {
+                    if !arg.is_last_set() || incl_last {
+                        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());
+            }
+        }
 
+        let mut ret_val = Vec::new();
+        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);

src/parser/validator.rs

@@ -240,6 +240,9 @@ impl<'cmd> Validator<'cmd> {
         debug!("Validator::validate_required: required={:?}", self.required);
         self.gather_requires(matcher);
 
+        let mut missing_required = Vec::new();
+        let mut highest_index = 0;
+
         let is_exclusive_present = matcher
             .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent))
@@ -263,7 +266,14 @@ 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());
+                    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");
@@ -273,33 +283,82 @@ 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());
                 }
             }
         }
 
         // Validate the conditionally required args
-        for a in self.cmd.get_arguments() {
+        for a in self
+            .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()))
-                    && !matcher.check_explicit(&a.id, &ArgPredicate::IsPresent)
-                {
-                    return self.missing_required_error(matcher, vec![a.id.clone()]);
+                if matcher.check_explicit(other, &ArgPredicate::Equals(val.into())) {
+                    debug!(
+                        "Validator::validate_required:iter: Missing {:?}",
+                        a.get_id()
+                    );
+                    required = true;
                 }
             }
 
             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() {
+                debug!(
+                    "Validator::validate_required:iter: Missing {:?}",
+                    a.get_id()
+                );
+                required = true;
+            }
+
+            if (!a.r_unless.is_empty() || !a.r_unless_all.is_empty())
+                && self.fails_arg_required_unless(a, matcher)
+            {
+                debug!(
+                    "Validator::validate_required:iter: Missing {:?}",
+                    a.get_id()
+                );
+                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))
             {
-                return self.missing_required_error(matcher, vec![a.id.clone()]);
+                if pos.get_index() < Some(highest_index) {
+                    debug!(
+                        "Validator::validate_required:iter: Missing {:?}",
+                        pos.get_id()
+                    );
+                    missing_required.push(pos.get_id().clone());
+                }
             }
         }
 
-        self.validate_required_unless(matcher)?;
+        if !missing_required.is_empty() {
+            self.missing_required_error(matcher, missing_required)?;
+        }
 
         Ok(())
     }
@@ -315,25 +374,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());

src/util/flat_map.rs

@@ -1,3 +1,5 @@
+#![allow(dead_code)]
+
 use std::borrow::Borrow;
 
 /// Flat (Vec) backed map

src/util/flat_set.rs

@@ -1,3 +1,5 @@
+#![allow(dead_code)]
+
 use std::borrow::Borrow;
 
 /// Flat (Vec) backed set

tests/builder/default_vals.rs

@@ -732,7 +732,7 @@ fn default_vals_donnot_show_in_smart_usage() {
     <input>
 
 Usage:
-    bug [OPTIONS] <input>
+    bug <input>
 
 For more information try --help
 ",

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 <option2>
+    <positional>
     <positional2>
 
 Usage:
-    clap-test --long-option-2 <option2> -F <positional2>
+    clap-test --long-option-2 <option2> -F <positional> <positional2>
 
 For more information try --help
 ";
@@ -141,7 +142,7 @@ static POSITIONAL_REQ: &str = "error: The following required arguments were not
     <opt>
 
 Usage:
-    clap-test <flag> <opt> [ARGS]
+    clap-test <flag> <opt>
 
 For more information try --help
 ";
@@ -160,7 +161,7 @@ static POSITIONAL_REQ_IF_NO_VAL: &str = "error: The following required arguments
     <flag>
 
 Usage:
-    clap-test <flag> [ARGS]
+    clap-test <flag>
 
 For more information try --help
 ";