From 05cd057978db743a65fb5fde33213af752d064e7 Mon Sep 17 00:00:00 2001 From: Sean Allred Date: Sun, 24 Dec 2023 04:54:54 -0600 Subject: [PATCH] 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_ --- clap_builder/src/output/help_template.rs | 115 +++++++++--------- clap_mangen/src/render.rs | 6 +- tests/builder/possible_values.rs | 142 +++++++++++++++++++++++ 3 files changed, 205 insertions(+), 58 deletions(-) diff --git a/clap_builder/src/output/help_template.rs b/clap_builder/src/output/help_template.rs index 9fe7211b306..dff15ff1279 100644 --- a/clap_builder/src/output/help_template.rs +++ b/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::>() - .join(", "); + let pvs = possible_vals + .iter() + .filter_map(PossibleValue::get_visible_quoted_name) + .collect::>() + .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) diff --git a/clap_mangen/src/render.rs b/clap_mangen/src/render.rs index 95f2740938f..ef4981d80d6 100644 --- a/clap_mangen/src/render.rs +++ b/clap_mangen/src/render.rs @@ -305,11 +305,15 @@ fn option_default_values(opt: &clap::Arg) -> Option { } fn get_possible_values(arg: &clap::Arg) -> Option<(Vec, 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 diff --git a/tests/builder/possible_values.rs b/tests/builder/possible_values.rs index b39517279be..de66cf8650f 100644 --- a/tests/builder/possible_values.rs +++ b/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>, + } + + 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 { + 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 { + todo!() + } + + fn possible_values(&self) -> Option + '_>> { + 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); + } +}