Repository: clap-rs/clap
Pull request #3789 (2022-06-04 18:00:22)

refactor: Prep for actions in derive
<!--
Thanks for helping out!

Please link the appropriate issue from your PR.

If you don't have an issue, we'd recommend starting with one first so the PR can focus on the
implementation (unless its an obvious bug or documentation fix that will have
little conversation).
-->



Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index e9ce379c..d6bfc8af 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -144,7 +144,7 @@ jobs:
     - name: Check documentation
       env:
         RUSTDOCFLAGS: -D warnings
-      run: cargo doc --workspace --all-features --no-deps --document-private-items
+      run: make doc
   rustfmt:
     name: rustfmt
     runs-on: ubuntu-latest
diff --git a/Makefile b/Makefile
index 1f3db6ee..7ac4d449 100644
--- a/Makefile
+++ b/Makefile
@@ -35,3 +35,6 @@ clippy-%:
 
 test-ui-%:
 	cargo +${MSRV} test --test derive_ui --features derive ${_FEATURES_${@:test-ui-%=%}}
+
+doc:
+	cargo doc --workspace --all-features --no-deps --document-private-items
diff --git a/src/builder/action.rs b/src/builder/action.rs
index fa1ccd57..acfe2027 100644
--- a/src/builder/action.rs
+++ b/src/builder/action.rs
@@ -9,7 +9,7 @@
 ///     .arg(
 ///         Arg::new("special-help")
 ///             .short('?')
-///             .action(clap::builder::ArgAction::Help)
+///             .action(clap::ArgAction::Help)
 ///     );
 ///
 /// // Existing help still exists
@@ -35,7 +35,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::Set)
+    ///             .action(clap::ArgAction::Set)
     ///     );
     ///
     /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap();
@@ -59,7 +59,7 @@ pub enum ArgAction {
     ///         Arg::new("flag")
     ///             .long("flag")
     ///             .multiple_occurrences(true)
-    ///             .action(clap::builder::ArgAction::Append)
+    ///             .action(clap::ArgAction::Append)
     ///     );
     ///
     /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value1", "--flag", "value2"]).unwrap();
@@ -82,7 +82,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::StoreValue)
+    ///             .action(clap::ArgAction::StoreValue)
     ///     );
     ///
     /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap();
@@ -108,7 +108,7 @@ pub enum ArgAction {
     ///         Arg::new("flag")
     ///             .long("flag")
     ///             .multiple_occurrences(true)
-    ///             .action(clap::builder::ArgAction::IncOccurrence)
+    ///             .action(clap::ArgAction::IncOccurrence)
     ///     );
     ///
     /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
@@ -132,7 +132,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::SetTrue)
+    ///             .action(clap::ArgAction::SetTrue)
     ///     );
     ///
     /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
@@ -167,7 +167,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::SetFalse)
+    ///             .action(clap::ArgAction::SetFalse)
     ///     );
     ///
     /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
@@ -202,7 +202,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::Count)
+    ///             .action(clap::ArgAction::Count)
     ///     );
     ///
     /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
@@ -235,7 +235,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("special-help")
     ///             .short('?')
-    ///             .action(clap::builder::ArgAction::Help)
+    ///             .action(clap::ArgAction::Help)
     ///     );
     ///
     /// // Existing help still exists
@@ -261,7 +261,7 @@ pub enum ArgAction {
     ///     .arg(
     ///         Arg::new("special-version")
     ///             .long("special-version")
-    ///             .action(clap::builder::ArgAction::Version)
+    ///             .action(clap::ArgAction::Version)
     ///     );
     ///
     /// // Existing help still exists
diff --git a/src/builder/arg.rs b/src/builder/arg.rs
index 6f9fc767..6ac18271 100644
--- a/src/builder/arg.rs
+++ b/src/builder/arg.rs
@@ -18,9 +18,9 @@ use yaml_rust::Yaml;
 
 // Internal
 use crate::builder::usage_parser::UsageParser;
-use crate::builder::ArgAction;
 use crate::builder::ArgPredicate;
 use crate::util::{Id, Key};
+use crate::ArgAction;
 use crate::PossibleValue;
 use crate::ValueHint;
 use crate::INTERNAL_ERROR_MSG;
@@ -1014,7 +1014,7 @@ impl<'help> Arg<'help> {
     ///     .arg(
     ///         Arg::new("flag")
     ///             .long("flag")
-    ///             .action(clap::builder::ArgAction::StoreValue)
+    ///             .action(clap::ArgAction::StoreValue)
     ///     );
     ///
     /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap();
diff --git a/src/builder/value_parser.rs b/src/builder/value_parser.rs
index 01aeef57..30b3610d 100644
--- a/src/builder/value_parser.rs
+++ b/src/builder/value_parser.rs
@@ -1252,7 +1252,7 @@ impl BoolValueParser {
         ["true", "false"]
             .iter()
             .copied()
-            .map(|l| crate::PossibleValue::new(l).hide(true))
+            .map(|l| crate::PossibleValue::new(l))
     }
 }
 
@@ -1352,7 +1352,7 @@ impl FalseyValueParser {
             .iter()
             .chain(crate::util::FALSE_LITERALS.iter())
             .copied()
-            .map(|l| crate::PossibleValue::new(l).hide(true))
+            .map(|l| crate::PossibleValue::new(l).hide(l != "true" && l != "false"))
     }
 }
 
@@ -1449,7 +1449,7 @@ impl BoolishValueParser {
             .iter()
             .chain(crate::util::FALSE_LITERALS.iter())
             .copied()
-            .map(|l| crate::PossibleValue::new(l).hide(true))
+            .map(|l| crate::PossibleValue::new(l).hide(l != "true" && l != "false"))
     }
 }
 
diff --git a/src/lib.rs b/src/lib.rs
index 435539b1..f1f58a97 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -26,6 +26,7 @@
 #[cfg(not(feature = "std"))]
 compile_error!("`std` feature is currently required to build `clap`");
 
+pub use crate::builder::ArgAction;
 pub use crate::builder::Command;
 pub use crate::builder::{Arg, ArgGroup};
 pub use crate::error::Error;
diff --git a/src/output/help.rs b/src/output/help.rs
index 04e0a7a2..152955d2 100644
--- a/src/output/help.rs
+++ b/src/output/help.rs
@@ -601,7 +601,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> {
                 spec_vals.push(env_info);
             }
         }
-        if !a.is_hide_default_value_set() && !a.default_vals.is_empty() {
+        if a.is_takes_value_set() && !a.is_hide_default_value_set() && !a.default_vals.is_empty() {
             debug!(
                 "Help::spec_vals: Found default value...[{:?}]",
                 a.default_vals
diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs
index eab49991..690f504b 100644
--- a/src/parser/arg_matcher.rs
+++ b/src/parser/arg_matcher.rs
@@ -116,7 +116,7 @@ impl ArgMatcher {
         self.matches.args.contains_key(arg)
     }
 
-    pub(crate) fn arg_names(&self) -> indexmap::map::Keys<Id, MatchedArg> {
+    pub(crate) fn arg_ids(&self) -> indexmap::map::Keys<Id, MatchedArg> {
         self.matches.args.keys()
     }
 
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index 4a695c26..e6d99800 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -9,7 +9,6 @@ use clap_lex::RawOsStr;
 
 // Internal
 use crate::builder::AppSettings as AS;
-use crate::builder::ArgAction;
 use crate::builder::{Arg, Command};
 use crate::error::Error as ClapError;
 use crate::error::Result as ClapResult;
@@ -20,6 +19,7 @@ use crate::parser::features::suggestions;
 use crate::parser::{ArgMatcher, SubCommand};
 use crate::parser::{Validator, ValueSource};
 use crate::util::Id;
+use crate::ArgAction;
 use crate::{INTERNAL_ERROR_MSG, INVALID_UTF8};
 
 pub(crate) struct Parser<'help, 'cmd> {
@@ -771,7 +771,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
                     long_arg, rest
                 );
                 let used: Vec<Id> = matcher
-                    .arg_names()
+                    .arg_ids()
+                    .filter(|arg_id| {
+                        matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent)
+                    })
                     .filter(|&n| {
                         self.cmd
                             .find(n)
@@ -1312,7 +1315,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
 
         // Override anything that can override us
         let mut transitive = Vec::new();
-        for arg_id in matcher.arg_names() {
+        for arg_id in matcher.arg_ids() {
             if let Some(overrider) = self.cmd.find(arg_id) {
                 if overrider.overrides.contains(&arg.id) {
                     transitive.push(&overrider.id);
@@ -1612,7 +1615,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
 
         let required = self.cmd.required_graph();
         let used: Vec<Id> = matcher
-            .arg_names()
+            .arg_ids()
+            .filter(|arg_id| {
+                matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent)
+            })
             .filter(|n| self.cmd.find(n).map_or(true, |a| !a.is_hide_set()))
             .cloned()
             .collect();
diff --git a/src/parser/validator.rs b/src/parser/validator.rs
index dca9789e..dc4c3598 100644
--- a/src/parser/validator.rs
+++ b/src/parser/validator.rs
@@ -52,7 +52,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
 
         if !has_subcmd && self.cmd.is_arg_required_else_help_set() {
             let num_user_values = matcher
-                .arg_names()
+                .arg_ids()
                 .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
                 .count();
             if num_user_values == 0 {
@@ -179,7 +179,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
         self.validate_exclusive(matcher)?;
 
         for arg_id in matcher
-            .arg_names()
+            .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
             .filter(|arg_id| self.cmd.find(arg_id).is_some())
         {
@@ -193,15 +193,22 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
 
     fn validate_exclusive(&self, matcher: &ArgMatcher) -> ClapResult<()> {
         debug!("Validator::validate_exclusive");
-        // Not bothering to filter for `check_explicit` since defaults shouldn't play into this
-        let args_count = matcher.arg_names().count();
+        let args_count = matcher
+            .arg_ids()
+            .filter(|arg_id| {
+                matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent)
+            })
+            .count();
         if args_count <= 1 {
             // Nothing present to conflict with
             return Ok(());
         }
 
         matcher
-            .arg_names()
+            .arg_ids()
+            .filter(|arg_id| {
+                matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent)
+            })
             .filter_map(|name| {
                 debug!("Validator::validate_exclusive:iter:{:?}", name);
                 self.cmd
@@ -263,7 +270,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
 
     fn build_conflict_err_usage(&self, matcher: &ArgMatcher, conflicting_keys: &[Id]) -> String {
         let used_filtered: Vec<Id> = matcher
-            .arg_names()
+            .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
             .filter(|n| {
                 // Filter out the args we don't want to specify.
@@ -288,7 +295,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
     fn gather_requires(&mut self, matcher: &ArgMatcher) {
         debug!("Validator::gather_requires");
         for name in matcher
-            .arg_names()
+            .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
         {
             debug!("Validator::gather_requires:iter:{:?}", name);
@@ -455,12 +462,15 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
         debug!("Validator::validate_required: required={:?}", self.required);
         self.gather_requires(matcher);
 
-        let is_exclusive_present = matcher.arg_names().any(|name| {
-            self.cmd
-                .find(name)
-                .map(|arg| arg.is_exclusive_set())
-                .unwrap_or_default()
-        });
+        let is_exclusive_present = matcher
+            .arg_ids()
+            .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
+            .any(|id| {
+                self.cmd
+                    .find(id)
+                    .map(|arg| arg.is_exclusive_set())
+                    .unwrap_or_default()
+            });
         debug!(
             "Validator::validate_required: is_exclusive_present={}",
             is_exclusive_present
@@ -569,7 +579,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
         );
 
         let used: Vec<Id> = matcher
-            .arg_names()
+            .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
             .filter(|n| {
                 // Filter out the args we don't want to specify.
@@ -601,7 +611,7 @@ impl Conflicts {
         debug!("Conflicts::gather_conflicts: arg={:?}", arg_id);
         let mut conflicts = Vec::new();
         for other_arg_id in matcher
-            .arg_names()
+            .arg_ids()
             .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent))
         {
             if arg_id == other_arg_id {


Test patch:
diff --git a/tests/builder/action.rs b/tests/builder/action.rs
index 1697914d..a6f73d24 100644
--- a/tests/builder/action.rs
+++ b/tests/builder/action.rs
@@ -1,7 +1,7 @@
 #![allow(clippy::bool_assert_comparison)]
 
-use clap::builder::ArgAction;
 use clap::Arg;
+use clap::ArgAction;
 use clap::Command;
 
 #[test]
diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs
index 80d2d31f..345afa50 100644
--- a/tests/builder/conflicts.rs
+++ b/tests/builder/conflicts.rs
@@ -62,7 +62,7 @@ fn flag_conflict_with_all() {
 }
 
 #[test]
-fn flag_conflict_with_everything() {
+fn exclusive_flag() {
     let result = Command::new("flag_conflict")
         .arg(arg!(-f --flag "some flag").exclusive(true))
         .arg(arg!(-o --other "some flag"))
@@ -72,6 +72,56 @@ fn flag_conflict_with_everything() {
     assert_eq!(err.kind(), ErrorKind::ArgumentConflict);
 }
 
+#[test]
+fn exclusive_option() {
+    let result = Command::new("flag_conflict")
+        .arg(
+            arg!(-f --flag <VALUE> "some flag")
+                .required(false)
+                .exclusive(true),
+        )
+        .arg(arg!(-o --other <VALUE> "some flag").required(false))
+        .try_get_matches_from(vec!["myprog", "-o=val1", "-f=val2"]);
+    assert!(result.is_err());
+    let err = result.err().unwrap();
+    assert_eq!(err.kind(), ErrorKind::ArgumentConflict);
+}
+
+#[test]
+fn not_exclusive_with_defaults() {
+    let result = Command::new("flag_conflict")
+        .arg(
+            arg!(-f --flag <VALUE> "some flag")
+                .required(false)
+                .exclusive(true),
+        )
+        .arg(
+            arg!(-o --other <VALUE> "some flag")
+                .required(false)
+                .default_value("val1"),
+        )
+        .try_get_matches_from(vec!["myprog", "-f=val2"]);
+    assert!(result.is_ok(), "{}", result.unwrap_err());
+}
+
+#[test]
+fn default_doesnt_activate_exclusive() {
+    let result = Command::new("flag_conflict")
+        .arg(
+            arg!(-f --flag <VALUE> "some flag")
+                .required(false)
+                .exclusive(true)
+                .default_value("val2"),
+        )
+        .arg(
+            arg!(-o --other <VALUE> "some flag")
+                .required(false)
+                .default_value("val1"),
+        )
+        .try_get_matches_from(vec!["myprog"]);
+    assert!(result.is_ok(), "{}", result.unwrap_err());
+}
+
 #[test]
 fn arg_conflicts_with_group() {
     let mut cmd = Command::new("group_conflict")
diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs
index bacbe1ff..1d2913e4 100644
--- a/tests/builder/default_missing_vals.rs
+++ b/tests/builder/default_missing_vals.rs
@@ -24,6 +24,7 @@ fn opt_missing() {
         m.value_source("color").unwrap(),
         clap::ValueSource::DefaultValue
     );
+    assert_eq!(m.index_of("color"), Some(1));
 }
 
 #[test]
@@ -50,6 +51,7 @@ fn opt_present_with_missing_value() {
         m.value_source("color").unwrap(),
         clap::ValueSource::CommandLine
     );
+    assert_eq!(m.index_of("color"), Some(2));
 }
 
 #[test]
@@ -76,6 +78,7 @@ fn opt_present_with_value() {
         m.value_source("color").unwrap(),
         clap::ValueSource::CommandLine
     );
+    assert_eq!(m.index_of("color"), Some(2));
 }
 
 #[test]
@@ -101,6 +104,7 @@ fn opt_present_with_empty_value() {
         m.value_source("color").unwrap(),
         clap::ValueSource::CommandLine
     );
+    assert_eq!(m.index_of("color"), Some(2));
 }
 
 //## `default_value`/`default_missing_value` non-interaction checks
@@ -271,3 +275,32 @@ fn default_missing_values_are_valid() {
         )
         .try_get_matches();
 }
+
+#[test]
+fn valid_index() {
+    let m = Command::new("df")
+        .arg(
+            Arg::new("color")
+                .long("color")
+                .default_value("auto")
+                .min_values(0)
+                .require_equals(true)
+                .default_missing_value("always"),
+        )
+        .arg(Arg::new("sync").long("sync"))
+        .try_get_matches_from(vec!["df", "--color", "--sync"])
+        .unwrap();
+    assert!(m.is_present("color"));
+    assert_eq!(
+        m.get_one::<String>("color").map(|v| v.as_str()).unwrap(),
+        "always"
+    );
+    assert_eq!(m.occurrences_of("color"), 1);
+    assert_eq!(
+        m.value_source("color").unwrap(),
+        clap::ValueSource::CommandLine
+    );
+
+    // Make sure the index reflects `--color`s position and not something else
+    assert_eq!(m.index_of("color"), Some(2));
+}
diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs
index 501ffcd3..b6de1e52 100644
--- a/tests/builder/default_vals.rs
+++ b/tests/builder/default_vals.rs
@@ -19,6 +19,20 @@ fn opts() {
     );
 }
 
+#[test]
+fn default_has_index() {
+    let r = Command::new("df")
+        .arg(
+            arg!(o: -o <opt> "some opt")
+                .required(false)
+                .default_value("default"),
+        )
+        .try_get_matches_from(vec![""]);
+    assert!(r.is_ok(), "{}", r.unwrap_err());
+    let m = r.unwrap();
+    assert_eq!(m.index_of("o"), Some(1));
+}
+
 #[test]
 fn opt_without_value_fail() {
     let r = Command::new("df")
diff --git a/tests/derive/help.rs b/tests/derive/help.rs
index 1edf42d9..4254dda5 100644
--- a/tests/derive/help.rs
+++ b/tests/derive/help.rs
@@ -363,7 +363,7 @@ OPTIONS:
     let mut buffer: Vec<u8> = Default::default();
     cmd.write_help(&mut buffer).unwrap();
     let help = String::from_utf8(buffer).unwrap();
-    assert_eq!(help, HELP);
+    snapbox::assert_eq(HELP, help);
 }
 
 #[test]
@@ -420,7 +420,7 @@ OPTIONS:
     let mut buffer: Vec<u8> = Default::default();
     cmd.write_help(&mut buffer).unwrap();
     let help = String::from_utf8(buffer).unwrap();
-    assert_eq!(help, HELP);
+    snapbox::assert_eq(HELP, help);
 }
 
 #[test]
@@ -476,5 +476,52 @@ OPTIONS:
     let mut buffer: Vec<u8> = Default::default();
     cmd.write_help(&mut buffer).unwrap();
     let help = String::from_utf8(buffer).unwrap();
-    assert_eq!(help, HELP);
+    snapbox::assert_eq(HELP, help);
+}
+
+#[test]
+#[cfg(feature = "unstable-v4")]
+fn derive_possible_value_help() {
+    static HELP: &str = "clap 
+Application help
+
+USAGE:
+    clap <ARG>
+
+ARGS:
+    <ARG>
+            Argument help
+
+            Possible values:
+              - foo: Foo help
+              - bar: Bar help
+
+OPTIONS:
+    -h, --help
+            Print help information
+";
+
+    /// Application help
+    #[derive(Parser, PartialEq, Debug)]
+    struct Args {
+        /// Argument help
+        #[clap(arg_enum, value_parser)]
+        arg: ArgChoice,
+    }
+
+    #[derive(clap::ArgEnum, PartialEq, Debug, Clone)]
+    enum ArgChoice {
+        /// Foo help
+        Foo,
+        /// Bar help
+        Bar,
+    }
+
+    use clap::CommandFactory;
+    let mut cmd = Args::command();
+
+    let mut buffer: Vec<u8> = Default::default();
+    cmd.write_long_help(&mut buffer).unwrap();
+    let help = String::from_utf8(buffer).unwrap();
+    snapbox::assert_eq(HELP, help);
 }
diff --git a/tests/derive_ui/next/tuple_struct.stderr b/tests/derive_ui/next/tuple_struct.stderr
index 3055c31a..bf2bac06 100644
--- a/tests/derive_ui/next/tuple_struct.stderr
+++ b/tests/derive_ui/next/tuple_struct.stderr
@@ -17,6 +17,6 @@ error[E0599]: no function or associated item named `parse` found for struct `Opt
    |
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following traits define an item `parse`, perhaps you need to implement one of them:
-           candidate #1: `Parser`
+           candidate #1: `StructOpt`
            candidate #2: `AnyValueParser`
            candidate #3: `TypedValueParser`