Merge #1998

1998: Relax some restrictions on validators r=pksunkara a=CreepySkeleton



Co-authored-by: CreepySkeleton <creepy-skeleton@yandex.ru>

src/build/app/debug_asserts.rs

@@ -0,0 +1,282 @@
+use crate::{App, AppSettings, ArgSettings, ValueHint};
+use std::cmp::Ordering;
+
+#[derive(Eq)]
+enum Flag<'a> {
+    App(String, &'a str),
+    Arg(String, &'a str),
+}
+
+impl PartialEq for Flag<'_> {
+    fn eq(&self, other: &Flag) -> bool {
+        self.cmp(other) == Ordering::Equal
+    }
+}
+
+impl PartialOrd for Flag<'_> {
+    fn partial_cmp(&self, other: &Flag) -> Option<Ordering> {
+        use Flag::*;
+
+        match (self, other) {
+            (App(s1, _), App(s2, _))
+            | (Arg(s1, _), Arg(s2, _))
+            | (App(s1, _), Arg(s2, _))
+            | (Arg(s1, _), App(s2, _)) => {
+                if s1 == s2 {
+                    Some(Ordering::Equal)
+                } else {
+                    s1.partial_cmp(s2)
+                }
+            }
+        }
+    }
+}
+
+impl Ord for Flag<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.partial_cmp(other).unwrap()
+    }
+}
+
+pub(crate) fn assert_app(app: &App) {
+    debug!("App::_debug_asserts");
+
+    let mut short_flags = vec![];
+    let mut long_flags = vec![];
+
+    for sc in &app.subcommands {
+        if let Some(s) = sc.short_flag.as_ref() {
+            short_flags.push(Flag::App(format!("-{}", s), &sc.name));
+        }
+
+        for (short_alias, _) in &sc.short_flag_aliases {
+            short_flags.push(Flag::App(format!("-{}", short_alias), &sc.name));
+        }
+
+        if let Some(l) = sc.long_flag.as_ref() {
+            long_flags.push(Flag::App(format!("--{}", l), &sc.name));
+        }
+
+        for (long_alias, _) in &sc.long_flag_aliases {
+            long_flags.push(Flag::App(format!("--{}", long_alias), &sc.name));
+        }
+    }
+
+    for arg in &app.args.args {
+        arg._debug_asserts();
+
+        if let Some(s) = arg.short.as_ref() {
+            short_flags.push(Flag::Arg(format!("-{}", s), &*arg.name));
+        }
+
+        for (short_alias, _) in &arg.short_aliases {
+            short_flags.push(Flag::Arg(format!("-{}", short_alias), &arg.name));
+        }
+
+        if let Some(l) = arg.long.as_ref() {
+            long_flags.push(Flag::Arg(format!("--{}", l), &*arg.name));
+        }
+
+        for (long_alias, _) in &arg.aliases {
+            long_flags.push(Flag::Arg(format!("--{}", long_alias), &arg.name));
+        }
+
+        // Name conflicts
+        assert!(
+            app.two_args_of(|x| x.id == arg.id).is_none(),
+            "Argument names must be unique, but '{}' is in use by more than one argument or group",
+            arg.name,
+        );
+
+        // Long conflicts
+        if let Some(l) = arg.long {
+            if let Some((first, second)) = app.two_args_of(|x| x.long == Some(l)) {
+                panic!(
+                    "Long option names must be unique for each argument, \
+                        but '--{}' is in use by both '{}' and '{}'",
+                    l, first.name, second.name
+                )
+            }
+        }
+
+        // Short conflicts
+        if let Some(s) = arg.short {
+            if let Some((first, second)) = app.two_args_of(|x| x.short == Some(s)) {
+                panic!(
+                    "Short option names must be unique for each argument, \
+                        but '-{}' is in use by both '{}' and '{}'",
+                    s, first.name, second.name
+                )
+            }
+        }
+
+        // Index conflicts
+        if let Some(idx) = arg.index {
+            if let Some((first, second)) =
+                app.two_args_of(|x| x.is_positional() && x.index == Some(idx))
+            {
+                panic!(
+                    "Argument '{}' has the same index as '{}' \
+                    and they are both positional arguments\n\n\t \
+                    Use Arg::multiple_values(true) to allow one \
+                    positional argument to take multiple values",
+                    first.name, second.name
+                )
+            }
+        }
+
+        // requires, r_if, r_unless
+        for req in &arg.requires {
+            assert!(
+                app.id_exists(&req.1),
+                "Argument or group '{:?}' specified in 'requires*' for '{}' does not exist",
+                req.1,
+                arg.name,
+            );
+        }
+
+        for req in &arg.r_ifs {
+            assert!(
+                app.id_exists(&req.0),
+                "Argument or group '{:?}' specified in 'required_if*' for '{}' does not exist",
+                req.0,
+                arg.name
+            );
+        }
+
+        for req in &arg.r_unless {
+            assert!(
+                app.id_exists(req),
+                "Argument or group '{:?}' specified in 'required_unless*' for '{}' does not exist",
+                req,
+                arg.name,
+            );
+        }
+
+        // blacklist
+        for req in &arg.blacklist {
+            assert!(
+                app.id_exists(req),
+                "Argument or group '{:?}' specified in 'conflicts_with*' for '{}' does not exist",
+                req,
+                arg.name,
+            );
+        }
+
+        if arg.is_set(ArgSettings::Last) {
+            assert!(
+                arg.long.is_none(),
+                "Flags or Options cannot have last(true) set. '{}' has both a long and last(true) set.",
+                arg.name
+            );
+            assert!(
+                arg.short.is_none(),
+                "Flags or Options cannot have last(true) set. '{}' has both a short and last(true) set.",
+                arg.name
+            );
+        }
+
+        assert!(
+            !(arg.is_set(ArgSettings::Required) && arg.global),
+            "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required",
+            arg.name
+        );
+
+        // validators
+        assert!(
+            arg.validator.is_none() || arg.validator_os.is_none(),
+            "Argument '{}' has both `validator` and `validator_os` set which is not allowed",
+            arg.name
+        );
+
+        if arg.value_hint == ValueHint::CommandWithArguments {
+            assert!(
+                arg.short.is_none() && arg.long.is_none(),
+                "Argument '{}' has hint CommandWithArguments and must be positional.",
+                arg.name
+            );
+
+            assert!(
+                app.is_set(AppSettings::TrailingVarArg),
+                "Positional argument '{}' has hint CommandWithArguments, so App must have TrailingVarArg set.",
+                arg.name
+            );
+        }
+    }
+
+    for group in &app.groups {
+        // Name conflicts
+        assert!(
+            app.groups.iter().filter(|x| x.id == group.id).count() < 2,
+            "Argument group name must be unique\n\n\t'{}' is already in use",
+            group.name,
+        );
+
+        // Groups should not have naming conflicts with Args
+        assert!(
+            !app.args.args.iter().any(|x| x.id == group.id),
+            "Argument group name '{}' must not conflict with argument name",
+            group.name,
+        );
+
+        // Args listed inside groups should exist
+        for arg in &group.args {
+            assert!(
+                app.args.args.iter().any(|x| x.id == *arg),
+                "Argument group '{}' contains non-existent argument '{:?}'",
+                group.name,
+                arg
+            )
+        }
+    }
+
+    // Conflicts between flags and subcommands
+
+    long_flags.sort_unstable();
+    short_flags.sort_unstable();
+
+    detect_duplicate_flags(&long_flags, "long");
+    detect_duplicate_flags(&short_flags, "short");
+
+    app._panic_on_missing_help(app.g_settings.is_set(AppSettings::HelpRequired));
+}
+
+fn detect_duplicate_flags(flags: &[Flag], short_or_long: &str) {
+    use Flag::*;
+
+    for (one, two) in find_duplicates(flags) {
+        match (one, two) {
+            (App(flag, one), App(_, another)) if one != another => panic!(
+                "the '{}' {} flag is specified for both '{}' and '{}' subcommands",
+                flag, short_or_long, one, another
+            ),
+
+            (Arg(flag, one), Arg(_, another)) if one != another => panic!(
+                "{} option names must be unique, but '{}' is in use by both '{}' and '{}'",
+                short_or_long, flag, one, another
+            ),
+
+            (Arg(flag, arg), App(_, sub)) | (App(flag, sub), Arg(_, arg)) => panic!(
+                "the '{}' {} flag for the '{}' argument conflicts with the short flag \
+                     for '{}' subcommand",
+                flag, short_or_long, arg, sub
+            ),
+
+            _ => {}
+        }
+    }
+}
+
+/// Find duplicates in a sorted array.
+///
+/// The algorithm is simple: the array is sorted, duplicates
+/// must be placed next to each other, we can check only adjacent elements.
+fn find_duplicates<T: PartialEq>(slice: &[T]) -> impl Iterator<Item = (&T, &T)> {
+    slice.windows(2).filter_map(|w| {
+        if w[0] == w[1] {
+            Some((&w[0], &w[1]))
+        } else {
+            None
+        }
+    })
+}

src/build/app/mod.rs

@@ -1,3 +1,5 @@
+#[cfg(debug_assertions)]
+mod debug_asserts;
 mod settings;
 #[cfg(test)]
 mod tests;
@@ -26,7 +28,7 @@ use crate::{
     output::{fmt::Colorizer, Help, HelpWriter, Usage},
     parse::{ArgMatcher, ArgMatches, Input, Parser},
     util::{safe_exit, termcolor::ColorChoice, ArgStr, Id, Key},
-    Result as ClapResult, ValueHint, INTERNAL_ERROR_MSG,
+    Result as ClapResult, INTERNAL_ERROR_MSG,
 };
 
 // FIXME (@CreepySkeleton): some of these variants are never constructed
@@ -1888,41 +1890,6 @@ impl<'help> App<'help> {
     }
 }
 
-// Allows checking for conflicts between `Args` and `Apps` with subcommand flags
-#[cfg(debug_assertions)]
-#[derive(Debug)]
-enum Flag<'r, 'help> {
-    App(&'r App<'help>, String),
-    Arg(&'r Arg<'help>, String),
-}
-
-#[cfg(debug_assertions)]
-impl Flag<'_, '_> {
-    pub fn value(&self) -> &str {
-        match self {
-            Self::App(_, value) => value,
-            Self::Arg(_, value) => value,
-        }
-    }
-
-    pub fn id(&self) -> &Id {
-        match self {
-            Self::App(app, _) => &app.id,
-            Self::Arg(arg, _) => &arg.id,
-        }
-    }
-}
-
-#[cfg(debug_assertions)]
-impl fmt::Display for Flag<'_, '_> {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            Self::App(app, _) => write!(f, "App named '{}'", app.name),
-            Self::Arg(arg, _) => write!(f, "Arg named '{}'", arg.name),
-        }
-    }
-}
-
 // Internally used only
 impl<'help> App<'help> {
     fn _do_parse(&mut self, it: &mut Input) -> ClapResult<ArgMatches> {
@@ -1997,7 +1964,7 @@ impl<'help> App<'help> {
         self.settings.set(AppSettings::Built);
 
         #[cfg(debug_assertions)]
-        self._debug_asserts();
+        self::debug_asserts::assert_app(self);
     }
 
     fn _panic_on_missing_help(&self, help_required_globally: bool) {
@@ -2025,73 +1992,11 @@ impl<'help> App<'help> {
     }
 
     #[cfg(debug_assertions)]
-    fn two_long_flags_of<F>(&self, condition: F) -> Option<(Flag, Flag)>
-    where
-        F: Fn(&Flag) -> bool,
-    {
-        let mut flags: Vec<Flag> = Vec::new();
-        for sc in &self.subcommands {
-            if let Some(long) = sc.long_flag {
-                flags.push(Flag::App(&sc, long.to_string()));
-            }
-            flags.extend(
-                sc.get_all_long_flag_aliases()
-                    .map(|alias| Flag::App(&sc, alias.to_string())),
-            );
-            self.args.args.iter().for_each(|arg| {
-                flags.extend(
-                    arg.aliases
-                        .iter()
-                        .map(|(alias, _)| Flag::Arg(arg, alias.to_string())),
-                )
-            });
-            flags.extend(
-                self.args
-                    .args
-                    .iter()
-                    .filter_map(|arg| arg.long.map(|l| Flag::Arg(arg, l.to_string()))),
-            );
-        }
-        two_elements_of(flags.into_iter().filter(|f| condition(f)))
-    }
-
-    #[cfg(debug_assertions)]
-    fn two_short_flags_of<F>(&self, condition: F) -> Option<(Flag, Flag)>
-    where
-        F: Fn(&Flag) -> bool,
-    {
-        let mut flags: Vec<Flag> = Vec::new();
-        for sc in &self.subcommands {
-            if let Some(short) = sc.short_flag {
-                flags.push(Flag::App(&sc, short.to_string()));
-            }
-            flags.extend(
-                sc.get_all_short_flag_aliases()
-                    .map(|alias| Flag::App(&sc, alias.to_string())),
-            );
-            self.args.args.iter().for_each(|arg| {
-                flags.extend(
-                    arg.short_aliases
-                        .iter()
-                        .map(|(alias, _)| Flag::Arg(arg, alias.to_string())),
-                )
-            });
-            flags.extend(
-                self.args
-                    .args
-                    .iter()
-                    .filter_map(|arg| arg.short.map(|l| Flag::Arg(arg, l.to_string()))),
-            );
-        }
-        two_elements_of(flags.into_iter().filter(|f| condition(f)))
-    }
-
-    #[cfg(debug_assertions)]
-    fn two_args_of<F>(&self, condition: F) -> Option<(&Arg, &Arg)>
+    fn two_args_of<F>(&self, condition: F) -> Option<(&Arg<'help>, &Arg<'help>)>
     where
         F: Fn(&Arg) -> bool,
     {
-        two_elements_of(self.args.args.iter().filter(|a| condition(a)))
+        two_elements_of(self.args.args.iter().filter(|a: &&Arg| condition(a)))
     }
 
     // just in case
@@ -2103,197 +2008,6 @@ impl<'help> App<'help> {
         two_elements_of(self.groups.iter().filter(|a| condition(a)))
     }
 
-    // Perform some expensive assertions on the Parser itself
-    #[allow(clippy::cognitive_complexity)]
-    #[cfg(debug_assertions)]
-    fn _debug_asserts(&self) {
-        debug!("App::_debug_asserts");
-
-        for sc in &self.subcommands {
-            // Conflicts between flag subcommands and long args
-            if let Some(l) = sc.long_flag {
-                if let Some((first, second)) = self.two_long_flags_of(|f| f.value() == l) {
-                    // Prevent conflicts with itself
-                    if first.id() != second.id() {
-                        panic!(
-                            "Long option names must be unique for each argument, \
-                        but '--{}' is used by both an {} and an {}",
-                            l, first, second
-                        );
-                    }
-                }
-            }
-
-            // Conflicts between flag subcommands and long args
-            if let Some(s) = sc.short_flag {
-                if let Some((first, second)) =
-                    self.two_short_flags_of(|f| f.value() == s.to_string())
-                {
-                    // Prevent conflicts with itself
-                    if first.id() != second.id() {
-                        panic!(
-                            "Short option names must be unique for each argument, \
-                        but '-{}' is used by both an {} and an {}",
-                            s, first, second
-                        );
-                    }
-                }
-            }
-        }
-
-        for arg in &self.args.args {
-            arg._debug_asserts();
-
-            // Name conflicts
-            assert!(
-                self.two_args_of(|x| x.id == arg.id).is_none(),
-                "Argument names must be unique, but '{}' is in use by more than one argument or group",
-                arg.name,
-            );
-
-            // Long conflicts
-            if let Some(l) = arg.long {
-                if let Some((first, second)) = self.two_args_of(|x| x.long == Some(l)) {
-                    panic!(
-                        "Long option names must be unique for each argument, \
-                            but '--{}' is in use by both '{}' and '{}'",
-                        l, first.name, second.name
-                    )
-                }
-            }
-
-            // Short conflicts
-            if let Some(s) = arg.short {
-                if let Some((first, second)) = self.two_args_of(|x| x.short == Some(s)) {
-                    panic!(
-                        "Short option names must be unique for each argument, \
-                            but '-{}' is in use by both '{}' and '{}'",
-                        s, first.name, second.name
-                    )
-                }
-            }
-
-            // Index conflicts
-            if let Some(idx) = arg.index {
-                if let Some((first, second)) =
-                    self.two_args_of(|x| x.is_positional() && x.index == Some(idx))
-                {
-                    panic!(
-                        "Argument '{}' has the same index as '{}' \
-                        and they are both positional arguments\n\n\t \
-                        Use Arg::multiple_values(true) to allow one \
-                        positional argument to take multiple values",
-                        first.name, second.name
-                    )
-                }
-            }
-
-            // requires, r_if, r_unless
-            for req in &arg.requires {
-                assert!(
-                    self.id_exists(&req.1),
-                    "Argument or group '{:?}' specified in 'requires*' for '{}' does not exist",
-                    req.1,
-                    arg.name,
-                );
-            }
-
-            for req in &arg.r_ifs {
-                assert!(
-                    self.id_exists(&req.0),
-                    "Argument or group '{:?}' specified in 'required_if*' for '{}' does not exist",
-                    req.0,
-                    arg.name
-                );
-            }
-
-            for req in &arg.r_unless {
-                assert!(
-                    self.id_exists(req),
-                    "Argument or group '{:?}' specified in 'required_unless*' for '{}' does not exist",
-                    req, arg.name,
-                );
-            }
-
-            // blacklist
-            for req in &arg.blacklist {
-                assert!(
-                    self.id_exists(req),
-                    "Argument or group '{:?}' specified in 'conflicts_with*' for '{}' does not exist",
-                    req, arg.name,
-                );
-            }
-
-            if arg.is_set(ArgSettings::Last) {
-                assert!(
-                    arg.long.is_none(),
-                    "Flags or Options cannot have last(true) set. '{}' has both a long and last(true) set.",
-                    arg.name
-                );
-                assert!(
-                    arg.short.is_none(),
-                    "Flags or Options cannot have last(true) set. '{}' has both a short and last(true) set.",
-                    arg.name
-                );
-            }
-
-            assert!(
-                !(arg.is_set(ArgSettings::Required) && arg.global),
-                "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required",
-                arg.name
-            );
-
-            // validators
-            assert!(
-                arg.validator.is_none() || arg.validator_os.is_none(),
-                "Argument '{}' has both `validator` and `validator_os` set which is not allowed",
-                arg.name
-            );
-
-            if arg.value_hint == ValueHint::CommandWithArguments {
-                assert!(
-                    arg.short.is_none() && arg.long.is_none(),
-                    "Argument '{}' has hint CommandWithArguments and must be positional.",
-                    arg.name
-                );
-
-                assert!(
-                    self.is_set(AppSettings::TrailingVarArg),
-                    "Positional argument '{}' has hint CommandWithArguments, so App must have TrailingVarArg set.",
-                    arg.name
-                );
-            }
-        }
-
-        for group in &self.groups {
-            // Name conflicts
-            assert!(
-                self.groups.iter().filter(|x| x.id == group.id).count() < 2,
-                "Argument group name must be unique\n\n\t'{}' is already in use",
-                group.name,
-            );
-
-            // Groups should not have naming conflicts with Args
-            assert!(
-                !self.args.args.iter().any(|x| x.id == group.id),
-                "Argument group name '{}' must not conflict with argument name",
-                group.name,
-            );
-
-            // Args listed inside groups should exist
-            for arg in &group.args {
-                assert!(
-                    self.args.args.iter().any(|x| x.id == *arg),
-                    "Argument group '{}' contains non-existent argument '{:?}'",
-                    group.name,
-                    arg
-                )
-            }
-        }
-
-        self._panic_on_missing_help(self.g_settings.is_set(AppSettings::HelpRequired));
-    }
-
     pub(crate) fn _propagate(&mut self, prop: Propagation) {
         macro_rules! propagate_subcmd {
             ($_self:expr, $sc:expr) => {{
@@ -2616,11 +2330,28 @@ impl<'help> App<'help> {
 
     /// Iterate through all the names of all subcommands (not recursively), including aliases.
     /// Used for suggestions.
-    pub(crate) fn all_subcommand_names(&self) -> impl Iterator<Item = &str> {
-        self.get_subcommands().map(|s| s.get_name()).chain(
-            self.get_subcommands()
-                .flat_map(|s| s.aliases.iter().map(|&(n, _)| n)),
-        )
+    pub(crate) fn all_subcommand_names<'a>(&'a self) -> impl Iterator<Item = &'a str>
+    where
+        'help: 'a,
+    {
+        let a: Vec<_> = self
+            .get_subcommands()
+            .flat_map(|sc| {
+                let name = sc.get_name();
+                let aliases = sc.get_all_aliases();
+                std::iter::once(name).chain(aliases)
+            })
+            .collect();
+
+        // Strictly speaking, we don't need this trip through the Vec.
+        // We should have been able to return FlatMap iter above directly.
+        //
+        // Unfortunately, that would trigger
+        // https://github.com/rust-lang/rust/issues/34511#issuecomment-373423999
+        //
+        // I think this "collect to vec" solution is better then the linked one
+        // because it's simpler and it doesn't really matter performance-wise.
+        a.into_iter()
     }
 
     pub(crate) fn unroll_args_in_group(&self, group: &Id) -> Vec<Id> {

src/build/app/tests.rs

@@ -44,3 +44,10 @@ fn global_settings() {
         .unwrap()
         .is_set(AppSettings::TrailingVarArg));
 }
+
+// This test will *fail to compile* if App is not Send + Sync
+#[test]
+fn app_send_sync() {
+    fn foo<T: Send + Sync>(_: T) {}
+    foo(App::new("test"))
+}

src/build/arg/mod.rs

@@ -13,8 +13,8 @@ use std::{
     env,
     ffi::{OsStr, OsString},
     fmt::{self, Display, Formatter},
-    rc::Rc,
     str,
+    sync::{Arc, Mutex},
 };
 
 // Third Party
@@ -30,8 +30,8 @@ use crate::{
 #[cfg(feature = "yaml")]
 use yaml_rust::Yaml;
 
-type Validator = Rc<dyn Fn(&str) -> Result<(), String>>;
-type ValidatorOs = Rc<dyn Fn(&OsStr) -> Result<(), String>>;
+type Validator<'a> = dyn FnMut(&str) -> Result<(), String> + Send + 'a;
+type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), String> + Send + 'a;
 
 /// The abstract representation of a command line argument. Used to set all the options and
 /// relationships that define a valid argument for the program.
@@ -80,8 +80,8 @@ pub struct Arg<'help> {
     pub(crate) num_vals: Option<u64>,
     pub(crate) max_vals: Option<u64>,
     pub(crate) min_vals: Option<u64>,
-    pub(crate) validator: Option<Validator>,
-    pub(crate) validator_os: Option<ValidatorOs>,
+    pub(crate) validator: Option<Arc<Mutex<Validator<'help>>>>,
+    pub(crate) validator_os: Option<Arc<Mutex<ValidatorOs<'help>>>>,
     pub(crate) val_delim: Option<char>,
     pub(crate) default_vals: Vec<&'help OsStr>,
     pub(crate) default_vals_ifs: VecMap<(Id, Option<&'help OsStr>, &'help OsStr)>,
@@ -1844,7 +1844,7 @@ impl<'help> Arg<'help> {
     /// arg, and `<YOUR MESSAGE>` is the `String` you return as the error.
     ///
     /// **NOTE:** There is a small performance hit for using validators, as they are implemented
-    /// with [`Rc`] pointers. And the value to be checked will be allocated an extra time in order
+    /// with [`Arc`] pointers. And the value to be checked will be allocated an extra time in order
     /// to be passed to the closure. This performance hit is extremely minimal in the grand
     /// scheme of things.
     ///
@@ -1869,15 +1869,15 @@ impl<'help> Arg<'help> {
     /// [`String`]: https://doc.rust-lang.org/std/string/struct.String.html
     /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html
     /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err
-    /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html
-    pub fn validator<F, O, E>(mut self, f: F) -> Self
+    /// [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html
+    pub fn validator<F, O, E>(mut self, mut f: F) -> Self
     where
-        F: Fn(&str) -> Result<O, E> + 'static,
+        F: FnMut(&str) -> Result<O, E> + Send + 'help,
         E: ToString,
     {
-        self.validator = Some(Rc::new(move |s| {
+        self.validator = Some(Arc::new(Mutex::new(move |s: &str| {
             f(s).map(|_| ()).map_err(|e| e.to_string())
-        }));
+        })));
         self
     }
 
@@ -1911,11 +1911,11 @@ impl<'help> Arg<'help> {
     /// [`Result`]: https://doc.rust-lang.org/std/result/enum.Result.html
     /// [`Err(String)`]: https://doc.rust-lang.org/std/result/enum.Result.html#variant.Err
     /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html
-    pub fn validator_os<F, O>(mut self, f: F) -> Self
+    pub fn validator_os<F, O>(mut self, mut f: F) -> Self
     where
-        F: Fn(&OsStr) -> Result<O, String> + 'static,
+        F: FnMut(&OsStr) -> Result<O, String> + Send + 'help,
     {
-        self.validator_os = Some(Rc::new(move |s| f(s).map(|_| ())));
+        self.validator_os = Some(Arc::new(Mutex::new(move |s: &OsStr| f(s).map(|_| ()))));
         self
     }
 
@@ -4494,11 +4494,11 @@ impl<'help> fmt::Debug for Arg<'help> {
             .field("min_vals", &self.min_vals)
             .field(
                 "validator",
-                &self.validator.as_ref().map_or("None", |_| "Some(Fn)"),
+                &self.validator.as_ref().map_or("None", |_| "Some(FnMut)"),
             )
             .field(
                 "validator_os",
-                &self.validator_os.as_ref().map_or("None", |_| "Some(Fn)"),
+                &self.validator_os.as_ref().map_or("None", |_| "Some(FnMut)"),
             )
             .field("val_delim", &self.val_delim)
             .field("default_vals", &self.default_vals)

src/build/arg/tests.rs

@@ -23,3 +23,10 @@ fn short_flag_name_missing() {
     assert!(a.val_names.is_empty());
     assert!(a.num_vals.is_none());
 }
+
+// This test will *fail to compile* if Arg is not Send + Sync
+#[test]
+fn arg_send_sync() {
+    fn foo<T: Send + Sync>(_: T) {}
+    foo(Arg::new("test"))
+}

src/build/arg_group.rs

@@ -550,6 +550,13 @@ requires:
         assert_eq!(g.requires, reqs);
         assert_eq!(g.conflicts, confs);
     }
+
+    // This test will *fail to compile* if ArgGroup is not Send + Sync
+    #[test]
+    fn arg_group_send_sync() {
+        fn foo<T: Send + Sync>(_: T) {}
+        foo(ArgGroup::new("test"))
+    }
 }
 
 impl Clone for ArgGroup<'_> {

src/output/help.rs

@@ -222,7 +222,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> {
     }
 
     /// Writes help for an argument to the wrapped stream.
-    fn write_arg(&mut self, arg: &Arg, prevent_nlh: bool) -> io::Result<()> {
+    fn write_arg(&mut self, arg: &Arg<'help>, prevent_nlh: bool) -> io::Result<()> {
         debug!("Help::write_arg");
         self.short(arg)?;
         self.long(arg)?;
@@ -232,7 +232,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> {
     }
 
     /// Writes argument's short command to the wrapped stream.
-    fn short(&mut self, arg: &Arg) -> io::Result<()> {
+    fn short(&mut self, arg: &Arg<'help>) -> io::Result<()> {
         debug!("Help::short");
 
         self.none(TAB)?;
@@ -247,7 +247,7 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> {
     }
 
     /// Writes argument's long command to the wrapped stream.
-    fn long(&mut self, arg: &Arg) -> io::Result<()> {
+    fn long(&mut self, arg: &Arg<'help>) -> io::Result<()> {
         debug!("Help::long");
         if !arg.has_switch() {
             return Ok(());

src/parse/matches/arg_matches.rs

@@ -214,7 +214,7 @@ impl ArgMatches {
     /// ```
     /// [`Values`]: ./struct.Values.html
     /// [`Iterator`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html
-    pub fn values_of<T: Key>(&self, id: T) -> Option<Values<'_>> {
+    pub fn values_of<T: Key>(&self, id: T) -> Option<Values> {
         self.args.get(&Id::from(id)).map(|arg| {
             fn to_str_slice(o: &OsString) -> &str {
                 o.to_str().expect(INVALID_UTF8)

src/parse/validator.rs

@@ -136,9 +136,14 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                     self.p.app.color(),
                 )?);
             }
+
+            // FIXME: `(&mut *vtor)(args...)` can be simplified to `vtor(args...)`
+            //      once https://github.com/rust-lang/rust/pull/72280 is landed on stable
+            //      (about 10 weeks from now)
             if let Some(ref vtor) = arg.validator {
                 debug!("Validator::validate_arg_values: checking validator...");
-                if let Err(e) = vtor(&*val.to_string_lossy()) {
+                let mut vtor = vtor.lock().unwrap();
+                if let Err(e) = (&mut *vtor)(&*val.to_string_lossy()) {
                     debug!("error");
                     return Err(Error::value_validation(Some(arg), &e, self.p.app.color())?);
                 } else {
@@ -147,7 +152,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
             }
             if let Some(ref vtor) = arg.validator_os {
                 debug!("Validator::validate_arg_values: checking validator_os...");
-                if let Err(e) = vtor(val) {
+                let mut vtor = vtor.lock().unwrap();
+                if let Err(e) = (&mut *vtor)(val) {
                     debug!("error");
                     return Err(Error::value_validation(
                         Some(arg),

tests/arg_aliases.rs

@@ -167,7 +167,7 @@ fn invisible_arg_aliases_help_output() {
                     .takes_value(true)
                     .aliases(&["invisible", "als1", "more"]),
             )
-            .arg(Arg::from("-f, --flag").aliases(&["invisible", "flg1", "anyway"])),
+            .arg(Arg::from("-f, --flag").aliases(&["unseeable", "flg1", "anyway"])),
     );
     assert!(utils::compare_output(
         app,

tests/arg_aliases_short.rs

@@ -163,7 +163,7 @@ fn invisible_short_arg_aliases_help_output() {
                     .takes_value(true)
                     .short_aliases(&['a', 'b', 'c']),
             )
-            .arg(Arg::from("-f, --flag").short_aliases(&['a', 'b', 'c'])),
+            .arg(Arg::from("-f, --flag").short_aliases(&['x', 'y', 'z'])),
     );
     assert!(utils::compare_output(
         app,

tests/fixtures/app.yaml

@@ -92,7 +92,7 @@ args:
     - multshortaliases:
         long: multshortaliases
         about: Tests multiple short aliases
-        short_aliases: [a, b, c]
+        short_aliases: [b, c]
     - minvals2:
         long: minvals2
         multiple: true

tests/flag_subcommands.rs

@@ -281,7 +281,7 @@ fn flag_subcommand_multiple() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an Arg named \'test\'"]
+#[should_panic = "the \'-f\' short flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"]
 fn flag_subcommand_short_conflict_with_arg() {
     let _ = App::new("test")
         .subcommand(App::new("some").short_flag('f').long_flag("some"))
@@ -291,7 +291,7 @@ fn flag_subcommand_short_conflict_with_arg() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an App named \'result\'"]
+#[should_panic = "the \'-f\' short flag is specified for both \'some\' and \'result\' subcommands"]
 fn flag_subcommand_short_conflict_with_alias() {
     let _ = App::new("test")
         .subcommand(App::new("some").short_flag('f').long_flag("some"))
@@ -301,7 +301,7 @@ fn flag_subcommand_short_conflict_with_alias() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Long option names must be unique for each argument, but \'--flag\' is used by both an App named \'some\' and an App named \'result\'"]
+#[should_panic = "the \'--flag\' long flag is specified for both \'some\' and \'result\' subcommands"]
 fn flag_subcommand_long_conflict_with_alias() {
     let _ = App::new("test")
         .subcommand(App::new("some").long_flag("flag"))
@@ -311,7 +311,7 @@ fn flag_subcommand_long_conflict_with_alias() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Short option names must be unique for each argument, but \'-f\' is used by both an App named \'some\' and an Arg named \'test\'"]
+#[should_panic = "the \'-f\' short flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"]
 fn flag_subcommand_short_conflict_with_arg_alias() {
     let _ = App::new("test")
         .subcommand(App::new("some").short_flag('f').long_flag("some"))
@@ -321,7 +321,7 @@ fn flag_subcommand_short_conflict_with_arg_alias() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Long option names must be unique for each argument, but \'--some\' is used by both an App named \'some\' and an Arg named \'test\'"]
+#[should_panic = "the \'--some\' long flag for the \'test\' argument conflicts with the short flag for \'some\' subcommand"]
 fn flag_subcommand_long_conflict_with_arg_alias() {
     let _ = App::new("test")
         .subcommand(App::new("some").short_flag('f').long_flag("some"))
@@ -331,7 +331,7 @@ fn flag_subcommand_long_conflict_with_arg_alias() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Long option names must be unique for each argument, but \'--flag\' is used by both an App named \'some\' and an Arg named \'flag\'"]
+#[should_panic = "the \'--flag\' long flag for the \'flag\' argument conflicts with the short flag for \'some\' subcommand"]
 fn flag_subcommand_long_conflict_with_arg() {
     let _ = App::new("test")
         .subcommand(App::new("some").short_flag('a').long_flag("flag"))

tests/validators.rs

@@ -48,3 +48,17 @@ fn test_validator_msg_newline() {
     let msg = format!("{}", err);
     assert!(msg.ends_with('\n'));
 }
+
+#[test]
+fn stateful_validator() {
+    let mut state = false;
+    App::new("test")
+        .arg(Arg::new("test").validator(|val| {
+            state = true;
+            val.parse::<u32>().map_err(|e| e.to_string())
+        }))
+        .try_get_matches_from(&["app", "10"])
+        .unwrap();
+
+    assert!(state);
+}