perf: Avoid retrieving possible_values unless used

In some sophisticated situations, these may be expensive to calculate.
One example might be a '--branch' option accepting any single Git
branch that exists on the remote -- in such a case, the remote would
need to be queried for all possible_values. The cost is ultimately
unavoidable at runtime since this validation has to happen eventually,
but there's no need to pay it when generating help text if
`is_hide_possible_values_set`.

To keep '-h' fast, avoid collecting `possible_values` during '-h'
unless we're actually going to use the values in display.

This optimization is repeated for the manpage renderer.

This is trivially based on the short-circuiting logic at [1], which at
least supports the idea that actually consuming the iterator is not
generally-guaranteed behavior when `hide_possible_values` is set.

Note on the 'expensive' mod: This keeps all the possible_values tests
in one file but allows the entire set of tests to be controlled by the
'strings' feature (which is required to be able to use String rather
than str for each possible value).

[1]: clap_builder/src/builder/command.rs:long_help_exists_

Author: vermiculus

clap_builder/src/output/help_template.rs

@@ -684,57 +684,56 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
         let help_is_empty = help.is_empty();
         self.writer.push_styled(&help);
         if let Some(arg) = arg {
-            const DASH_SPACE: usize = "- ".len();
-            let possible_vals = arg.get_possible_values();
-            if !possible_vals.is_empty()
-                && !arg.is_hide_possible_values_set()
-                && self.use_long_pv(arg)
-            {
-                debug!("HelpTemplate::help: Found possible vals...{possible_vals:?}");
-                let longest = possible_vals
-                    .iter()
-                    .filter(|f| !f.is_hide_set())
-                    .map(|f| display_width(f.get_name()))
-                    .max()
-                    .expect("Only called with possible value");
-
-                let spaces = spaces + TAB_WIDTH - DASH_SPACE;
-                let trailing_indent = spaces + DASH_SPACE;
-                let trailing_indent = self.get_spaces(trailing_indent);
+            if !arg.is_hide_possible_values_set() && self.use_long_pv(arg) {
+                const DASH_SPACE: usize = "- ".len();
+                let possible_vals = arg.get_possible_values();
+                if !possible_vals.is_empty() {
+                    debug!("HelpTemplate::help: Found possible vals...{possible_vals:?}");
+                    let longest = possible_vals
+                        .iter()
+                        .filter(|f| !f.is_hide_set())
+                        .map(|f| display_width(f.get_name()))
+                        .max()
+                        .expect("Only called with possible value");
+
+                    let spaces = spaces + TAB_WIDTH - DASH_SPACE;
+                    let trailing_indent = spaces + DASH_SPACE;
+                    let trailing_indent = self.get_spaces(trailing_indent);
+
+                    if !help_is_empty {
+                        let _ = write!(self.writer, "\n\n{:spaces$}", "");
+                    }
+                    self.writer.push_str("Possible values:");
+                    for pv in possible_vals.iter().filter(|pv| !pv.is_hide_set()) {
+                        let name = pv.get_name();
 
-                if !help_is_empty {
-                    let _ = write!(self.writer, "\n\n{:spaces$}", "");
-                }
-                self.writer.push_str("Possible values:");
-                for pv in possible_vals.iter().filter(|pv| !pv.is_hide_set()) {
-                    let name = pv.get_name();
+                        let mut descr = StyledStr::new();
+                        let _ = write!(
+                            &mut descr,
+                            "{}{name}{}",
+                            literal.render(),
+                            literal.render_reset()
+                        );
+                        if let Some(help) = pv.get_help() {
+                            debug!("HelpTemplate::help: Possible Value help");
+                            // To align help messages
+                            let padding = longest - display_width(name);
+                            let _ = write!(&mut descr, ": {:padding$}", "");
+                            descr.push_styled(help);
+                        }
 
-                    let mut descr = StyledStr::new();
-                    let _ = write!(
-                        &mut descr,
-                        "{}{name}{}",
-                        literal.render(),
-                        literal.render_reset()
-                    );
-                    if let Some(help) = pv.get_help() {
-                        debug!("HelpTemplate::help: Possible Value help");
-                        // To align help messages
-                        let padding = longest - display_width(name);
-                        let _ = write!(&mut descr, ": {:padding$}", "");
-                        descr.push_styled(help);
+                        let avail_chars = if self.term_w > trailing_indent.len() {
+                            self.term_w - trailing_indent.len()
+                        } else {
+                            usize::MAX
+                        };
+                        descr.replace_newline_var();
+                        descr.wrap(avail_chars);
+                        descr.indent("", &trailing_indent);
+
+                        let _ = write!(self.writer, "\n{:spaces$}- ", "",);
+                        self.writer.push_styled(&descr);
                     }
-
-                    let avail_chars = if self.term_w > trailing_indent.len() {
-                        self.term_w - trailing_indent.len()
-                    } else {
-                        usize::MAX
-                    };
-                    descr.replace_newline_var();
-                    descr.wrap(avail_chars);
-                    descr.indent("", &trailing_indent);
-
-                    let _ = write!(self.writer, "\n{:spaces$}- ", "",);
-                    self.writer.push_styled(&descr);
                 }
             }
         }
@@ -844,17 +843,19 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
             spec_vals.push(format!("[short aliases: {als}]"));
         }
 
-        let possible_vals = a.get_possible_values();
-        if !possible_vals.is_empty() && !a.is_hide_possible_values_set() && !self.use_long_pv(a) {
-            debug!("HelpTemplate::spec_vals: Found possible vals...{possible_vals:?}");
+        if !a.is_hide_possible_values_set() && !self.use_long_pv(a) {
+            let possible_vals = a.get_possible_values();
+            if !possible_vals.is_empty() {
+                debug!("HelpTemplate::spec_vals: Found possible vals...{possible_vals:?}");
 
-            let pvs = possible_vals
-                .iter()
-                .filter_map(PossibleValue::get_visible_quoted_name)
-                .collect::<Vec<_>>()
-                .join(", ");
+                let pvs = possible_vals
+                    .iter()
+                    .filter_map(PossibleValue::get_visible_quoted_name)
+                    .collect::<Vec<_>>()
+                    .join(", ");
 
-            spec_vals.push(format!("[possible values: {pvs}]"));
+                spec_vals.push(format!("[possible values: {pvs}]"));
+            }
         }
         let connector = if self.use_long { "\n" } else { " " };
         spec_vals.join(connector)

clap_mangen/src/render.rs

@@ -305,11 +305,15 @@ fn option_default_values(opt: &clap::Arg) -> Option<String> {
 }
 
 fn get_possible_values(arg: &clap::Arg) -> Option<(Vec<String>, bool)> {
+    if arg.is_hide_possible_values_set() {
+        return None;
+    }
+
     let possibles = &arg.get_possible_values();
     let possibles: Vec<&clap::builder::PossibleValue> =
         possibles.iter().filter(|pos| !pos.is_hide_set()).collect();
 
-    if !(possibles.is_empty() || arg.is_hide_possible_values_set()) {
+    if !possibles.is_empty() {
         return Some(format_possible_values(&possibles));
     }
     None

tests/builder/possible_values.rs

@@ -472,3 +472,145 @@ fn ignore_case_multiple_fail() {
     assert!(m.is_err());
     assert_eq!(m.unwrap_err().kind(), ErrorKind::InvalidValue);
 }
+
+#[cfg(feature = "string")]
+mod expensive {
+    use std::sync::{Arc, Mutex};
+
+    use clap::{Arg, Command};
+    use clap_builder::builder::{PossibleValue, PossibleValuesParser, TypedValueParser};
+
+    #[cfg(feature = "error-context")]
+    use super::utils;
+
+    #[derive(Clone)]
+    struct ExpensiveValues {
+        iterated: Arc<Mutex<bool>>,
+    }
+
+    impl ExpensiveValues {
+        pub fn new() -> Self {
+            ExpensiveValues {
+                iterated: Arc::new(Mutex::new(false)),
+            }
+        }
+    }
+
+    impl IntoIterator for ExpensiveValues {
+        type Item = String;
+
+        type IntoIter = ExpensiveValuesIntoIterator;
+
+        fn into_iter(self) -> Self::IntoIter {
+            ExpensiveValuesIntoIterator { me: self, index: 0 }
+        }
+    }
+
+    struct ExpensiveValuesIntoIterator {
+        me: ExpensiveValues,
+        index: usize,
+    }
+
+    impl Iterator for ExpensiveValuesIntoIterator {
+        type Item = String;
+        fn next(&mut self) -> Option<String> {
+            let mut guard = self
+                .me
+                .iterated
+                .lock()
+                .expect("not working across multiple threads");
+
+            *guard = true;
+            self.index += 1;
+
+            if self.index < 3 {
+                Some(format!("expensive-value-{}", self.index))
+            } else {
+                None
+            }
+        }
+    }
+
+    impl TypedValueParser for ExpensiveValues {
+        type Value = String;
+
+        fn parse_ref(
+            &self,
+            _cmd: &clap_builder::Command,
+            _arg: Option<&clap_builder::Arg>,
+            _value: &std::ffi::OsStr,
+        ) -> Result<Self::Value, clap_builder::Error> {
+            todo!()
+        }
+
+        fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
+            Some(Box::new(self.clone().into_iter().map(PossibleValue::from)))
+        }
+    }
+
+    #[test]
+    fn no_iterate_when_hidden() {
+        static PV_EXPECTED: &str = "\
+Usage: clap-test [some-cheap-option] [some-expensive-option]
+
+Arguments:
+  [some-cheap-option]      cheap [possible values: some, cheap, values]
+  [some-expensive-option]  expensive
+
+Options:
+  -h, --help  Print help
+";
+        let expensive = ExpensiveValues::new();
+        utils::assert_output(
+            Command::new("test")
+                .arg(
+                    Arg::new("some-cheap-option")
+                        .help("cheap")
+                        .value_parser(PossibleValuesParser::new(["some", "cheap", "values"])),
+                )
+                .arg(
+                    Arg::new("some-expensive-option")
+                        .help("expensive")
+                        .hide_possible_values(true)
+                        .value_parser(expensive.clone()),
+                ),
+            "clap-test -h",
+            PV_EXPECTED,
+            false,
+        );
+        assert_eq!(*expensive.iterated.lock().unwrap(), false);
+    }
+
+    #[test]
+    fn iterate_when_displayed() {
+        static PV_EXPECTED: &str = "\
+Usage: clap-test [some-cheap-option] [some-expensive-option]
+
+Arguments:
+  [some-cheap-option]      cheap [possible values: some, cheap, values]
+  [some-expensive-option]  expensive [possible values: expensive-value-1, expensive-value-2]
+
+Options:
+  -h, --help  Print help
+";
+        let expensive = ExpensiveValues::new();
+        utils::assert_output(
+            Command::new("test")
+                .arg(
+                    Arg::new("some-cheap-option")
+                        .help("cheap")
+                        .value_parser(PossibleValuesParser::new(["some", "cheap", "values"])),
+                )
+                .arg(
+                    Arg::new("some-expensive-option")
+                        .help("expensive")
+                        .hide_possible_values(false)
+                        .value_parser(expensive.clone()),
+                ),
+            "clap-test -h",
+            PV_EXPECTED,
+            false,
+        );
+        assert_eq!(*expensive.iterated.lock().unwrap(), true);
+    }
+}