Skip to content

Commit

Permalink
perf: Avoid retrieving possible_values unless used
Browse files Browse the repository at this point in the history 
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_
  • Loading branch information
@vermiculus
vermiculus committed Dec 28, 2023
1 parent d092896 commit 05cd057
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 58 deletions.
115 changes: 58 additions & 57 deletions clap_builder/src/output/help_template.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion clap_mangen/src/render.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
142 changes: 142 additions & 0 deletions tests/builder/possible_values.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

0 comments on commit 05cd057

Please sign in to comment.