fix: Detect when AllowInvalidUtf8 is needed

src/parse/arg_matcher.rs

@@ -126,11 +126,20 @@ impl ArgMatcher {
         self.0.args.iter()
     }
 
-    pub(crate) fn inc_occurrence_of(&mut self, arg: &Id, ci: bool) {
-        debug!("ArgMatcher::inc_occurrence_of: arg={:?}", arg);
-        let ma = self.entry(arg).or_insert(MatchedArg::new());
+    pub(crate) fn inc_occurrence_of_arg(&mut self, arg: &Arg) {
+        let id = &arg.id;
+        debug!("ArgMatcher::inc_occurrence_of_arg: id={:?}", id);
+        let ma = self.entry(id).or_insert(MatchedArg::new());
+        ma.set_ty(ValueType::CommandLine);
+        ma.set_ignore_case(arg.is_set(ArgSettings::IgnoreCase));
+        ma.invalid_utf8_allowed(arg.is_set(ArgSettings::AllowInvalidUtf8));
+        ma.occurs += 1;
+    }
+
+    pub(crate) fn inc_occurrence_of_group(&mut self, id: &Id) {
+        debug!("ArgMatcher::inc_occurrence_of_group: id={:?}", id);
+        let ma = self.entry(id).or_insert(MatchedArg::new());
         ma.set_ty(ValueType::CommandLine);
-        ma.set_case_insensitive(ci);
         ma.occurs += 1;
     }
 
@@ -142,7 +151,7 @@ impl ArgMatcher {
         }
     }
 
-    pub(crate) fn push_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
+    fn push_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
         // We will manually inc occurrences later(for flexibility under
         // specific circumstances, like only add one occurrence for flag
         // when we met: `--flag=one,two`).
@@ -151,15 +160,15 @@ impl ArgMatcher {
         ma.push_val(val);
     }
 
-    pub(crate) fn new_val_group(&mut self, arg: &Id) {
+    fn append_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
         let ma = self.entry(arg).or_default();
-        ma.new_val_group();
+        ma.set_ty(ty);
+        ma.append_val(val);
     }
 
-    pub(crate) fn append_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
+    pub(crate) fn new_val_group(&mut self, arg: &Id) {
         let ma = self.entry(arg).or_default();
-        ma.set_ty(ty);
-        ma.append_val(val);
+        ma.new_val_group();
     }
 
     pub(crate) fn add_index_to(&mut self, arg: &Id, idx: usize, ty: ValueType) {

src/parse/matches/arg_matches.rs

@@ -160,12 +160,10 @@ impl ArgMatches {
     /// [`default_value`]: crate::Arg::default_value()
     /// [`occurrences_of`]: crate::ArgMatches::occurrences_of()
     pub fn value_of<T: Key>(&self, id: T) -> Option<&str> {
-        if let Some(arg) = self.get_arg(&Id::from(id)) {
-            if let Some(v) = arg.first() {
-                return Some(v.to_str().expect(INVALID_UTF8));
-            }
-        }
-        None
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_utf8_validation(arg);
+        let v = arg.first()?;
+        Some(v.to_str().expect(INVALID_UTF8))
     }
 
     /// Gets the lossy value of a specific option or positional argument.
@@ -210,12 +208,10 @@ impl ArgMatches {
     /// [`occurrences_of`]: ArgMatches::occurrences_of()
     /// [`Arg::values_of_lossy`]: ArgMatches::values_of_lossy()
     pub fn value_of_lossy<T: Key>(&self, id: T) -> Option<Cow<'_, str>> {
-        if let Some(arg) = self.get_arg(&Id::from(id)) {
-            if let Some(v) = arg.first() {
-                return Some(v.to_string_lossy());
-            }
-        }
-        None
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_no_utf8_validation(arg);
+        let v = arg.first()?;
+        Some(v.to_string_lossy())
     }
 
     /// Get the `OsStr` value of a specific option or positional argument.
@@ -261,8 +257,10 @@ impl ArgMatches {
     /// [`occurrences_of`]: ArgMatches::occurrences_of()
     /// [`ArgMatches::values_of_os`]: ArgMatches::values_of_os()
     pub fn value_of_os<T: Key>(&self, id: T) -> Option<&OsStr> {
-        self.get_arg(&Id::from(id))
-            .and_then(|arg| arg.first().map(OsString::as_os_str))
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_no_utf8_validation(arg);
+        let v = arg.first()?;
+        Some(v.as_os_str())
     }
 
     /// Get an [`Iterator`] over [values] of a specific option or positional argument.
@@ -296,33 +294,28 @@ impl ArgMatches {
     /// [values]: Values
     /// [`Iterator`]: std::iter::Iterator
     pub fn values_of<T: Key>(&self, id: T) -> Option<Values> {
-        self.get_arg(&Id::from(id)).map(|arg| {
-            fn to_str_slice(o: &OsString) -> &str {
-                o.to_str().expect(INVALID_UTF8)
-            }
-
-            Values {
-                iter: arg.vals_flatten().map(to_str_slice),
-            }
-        })
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_utf8_validation(arg);
+        fn to_str_slice(o: &OsString) -> &str {
+            o.to_str().expect(INVALID_UTF8)
+        }
+        let v = Values {
+            iter: arg.vals_flatten().map(to_str_slice),
+        };
+        Some(v)
     }
 
     /// Placeholder documentation.
     #[cfg(feature = "unstable-grouped")]
     pub fn grouped_values_of<T: Key>(&self, id: T) -> Option<GroupedValues> {
-        #[allow(clippy::type_complexity)]
-        let arg_values: for<'a> fn(
-            &'a MatchedArg,
-        ) -> Map<
-            Iter<'a, Vec<OsString>>,
-            fn(&Vec<OsString>) -> Vec<&str>,
-        > = |arg| {
-            arg.vals()
-                .map(|g| g.iter().map(|x| x.to_str().expect(INVALID_UTF8)).collect())
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_utf8_validation(arg);
+        let v = GroupedValues {
+            iter: arg
+                .vals()
+                .map(|g| g.iter().map(|x| x.to_str().expect(INVALID_UTF8)).collect()),
         };
-        self.get_arg(&Id::from(id))
-            .map(arg_values)
-            .map(|iter| GroupedValues { iter })
+        Some(v)
     }
 
     /// Get the lossy values of a specific option or positional argument.
@@ -362,11 +355,13 @@ impl ArgMatches {
     /// assert_eq!(itr.next(), None);
     /// ```
     pub fn values_of_lossy<T: Key>(&self, id: T) -> Option<Vec<String>> {
-        self.get_arg(&Id::from(id)).map(|arg| {
-            arg.vals_flatten()
-                .map(|v| v.to_string_lossy().into_owned())
-                .collect()
-        })
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_no_utf8_validation(arg);
+        let v = arg
+            .vals_flatten()
+            .map(|v| v.to_string_lossy().into_owned())
+            .collect();
+        Some(v)
     }
 
     /// Get an [`Iterator`] over [`OsStr`] [values] of a specific option or positional argument.
@@ -412,13 +407,15 @@ impl ArgMatches {
     /// [values]: OsValues
     /// [`String`]: std::string::String
     pub fn values_of_os<T: Key>(&self, id: T) -> Option<OsValues> {
+        let arg = self.get_arg(&Id::from(id))?;
+        assert_no_utf8_validation(arg);
         fn to_str_slice(o: &OsString) -> &OsStr {
             o
         }
-
-        self.get_arg(&Id::from(id)).map(|arg| OsValues {
+        let v = OsValues {
             iter: arg.vals_flatten().map(to_str_slice),
-        })
+        };
+        Some(v)
     }
 
     /// Parse the value (with [`FromStr`]) of a specific option or positional argument.
@@ -463,18 +460,17 @@ impl ArgMatches {
         R: FromStr,
         <R as FromStr>::Err: Display,
     {
-        if let Some(v) = self.value_of(name) {
-            v.parse::<R>().map_err(|e| {
-                let message = format!(
-                    "The argument '{}' isn't a valid value for '{}': {}",
-                    v, name, e
-                );
-
-                Error::value_validation_without_app(name.to_string(), v.to_string(), message.into())
-            })
-        } else {
-            Err(Error::argument_not_found_auto(name.to_string()))
-        }
+        let v = self
+            .value_of(name)
+            .ok_or_else(|| Error::argument_not_found_auto(name.to_string()))?;
+        v.parse::<R>().map_err(|e| {
+            let message = format!(
+                "The argument '{}' isn't a valid value for '{}': {}",
+                v, name, e
+            );
+
+            Error::value_validation_without_app(name.to_string(), v.to_string(), message.into())
+        })
     }
 
     /// Parse the value (with [`FromStr`]) of a specific option or positional argument.
@@ -554,22 +550,17 @@ impl ArgMatches {
         R: FromStr,
         <R as FromStr>::Err: Display,
     {
-        if let Some(vals) = self.values_of(name) {
-            vals.map(|v| {
-                v.parse::<R>().map_err(|e| {
-                    let message = format!("The argument '{}' isn't a valid value: {}", v, e);
-
-                    Error::value_validation_without_app(
-                        name.to_string(),
-                        v.to_string(),
-                        message.into(),
-                    )
-                })
+        let v = self
+            .values_of(name)
+            .ok_or_else(|| Error::argument_not_found_auto(name.to_string()))?;
+        v.map(|v| {
+            v.parse::<R>().map_err(|e| {
+                let message = format!("The argument '{}' isn't a valid value: {}", v, e);
+
+                Error::value_validation_without_app(name.to_string(), v.to_string(), message.into())
             })
-            .collect()
-        } else {
-            Err(Error::argument_not_found_auto(name.to_string()))
-        }
+        })
+        .collect()
     }
 
     /// Parse the values (with [`FromStr`]) of a specific option or positional argument.
@@ -825,12 +816,9 @@ impl ArgMatches {
     /// ```
     /// [delimiter]: crate::Arg::value_delimiter()
     pub fn index_of<T: Key>(&self, id: T) -> Option<usize> {
-        if let Some(arg) = self.get_arg(&Id::from(id)) {
-            if let Some(i) = arg.get_index(0) {
-                return Some(i);
-            }
-        }
-        None
+        let arg = self.get_arg(&Id::from(id))?;
+        let i = arg.get_index(0)?;
+        Some(i)
     }
 
     /// All indices an argument appeared at when parsing.
@@ -910,9 +898,11 @@ impl ArgMatches {
     /// [`ArgMatches::index_of`]: ArgMatches::index_of()
     /// [delimiter]: Arg::value_delimiter()
     pub fn indices_of<T: Key>(&self, id: T) -> Option<Indices<'_>> {
-        self.get_arg(&Id::from(id)).map(|arg| Indices {
+        let arg = self.get_arg(&Id::from(id))?;
+        let i = Indices {
             iter: arg.indices(),
-        })
+        };
+        Some(i)
     }
 
     /// The `ArgMatches` for the current [subcommand].
@@ -1247,6 +1237,24 @@ impl<'a> Default for Indices<'a> {
     }
 }
 
+#[track_caller]
+#[inline]
+fn assert_utf8_validation(arg: &MatchedArg) {
+    debug_assert!(
+        matches!(arg.is_invalid_utf8_allowed(), None | Some(false)),
+        "Must use `_os` lookups with `Arg::allow_invalid_utf8`"
+    );
+}
+
+#[track_caller]
+#[inline]
+fn assert_no_utf8_validation(arg: &MatchedArg) {
+    debug_assert!(
+        matches!(arg.is_invalid_utf8_allowed(), None | Some(true)),
+        "Must use `Arg::allow_invalid_utf8` with `_os` lookups"
+    );
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/parse/matches/matched_arg.rs

@@ -24,7 +24,8 @@ pub(crate) struct MatchedArg {
     pub(crate) ty: ValueType,
     indices: Vec<usize>,
     vals: Vec<Vec<OsString>>,
-    case_insensitive: bool,
+    ignore_case: bool,
+    invalid_utf8_allowed: Option<bool>,
 }
 
 impl Default for MatchedArg {
@@ -40,7 +41,8 @@ impl MatchedArg {
             ty: ValueType::Unknown,
             indices: Vec::new(),
             vals: Vec::new(),
-            case_insensitive: false,
+            ignore_case: false,
+            invalid_utf8_allowed: None,
         }
     }
 
@@ -127,7 +129,7 @@ impl MatchedArg {
 
     pub(crate) fn contains_val(&self, val: &str) -> bool {
         self.vals_flatten().any(|v| {
-            if self.case_insensitive {
+            if self.ignore_case {
                 // If `v` isn't utf8, it can't match `val`, so `OsStr::to_str` should be fine
                 v.to_str().map_or(false, |v| eq_ignore_case(v, val))
             } else {
@@ -140,8 +142,16 @@ impl MatchedArg {
         self.ty = ty;
     }
 
-    pub(crate) fn set_case_insensitive(&mut self, ci: bool) {
-        self.case_insensitive = ci;
+    pub(crate) fn set_ignore_case(&mut self, yes: bool) {
+        self.ignore_case = yes;
+    }
+
+    pub(crate) fn invalid_utf8_allowed(&mut self, yes: bool) {
+        self.invalid_utf8_allowed = Some(yes);
+    }
+
+    pub(crate) fn is_invalid_utf8_allowed(&self) -> Option<bool> {
+        self.invalid_utf8_allowed
     }
 }
 

src/parse/parser.rs

@@ -706,9 +706,9 @@ impl<'help, 'app> Parser<'help, 'app> {
                 let mut sc_m = ArgMatcher::new(self.app);
 
                 while let Some((v, _)) = it.next() {
-                    if !self.is_set(AS::AllowInvalidUtf8ForExternalSubcommands)
-                        && v.to_str().is_none()
-                    {
+                    let allow_invalid_utf8 =
+                        self.is_set(AS::AllowInvalidUtf8ForExternalSubcommands);
+                    if !allow_invalid_utf8 && v.to_str().is_none() {
                         return Err(ClapError::invalid_utf8(
                             self.app,
                             Usage::new(self).create_usage_with_title(&[]),
@@ -720,6 +720,9 @@ impl<'help, 'app> Parser<'help, 'app> {
                         ValueType::CommandLine,
                         false,
                     );
+                    sc_m.get_mut(&Id::empty_hash())
+                        .expect("just inserted")
+                        .invalid_utf8_allowed(allow_invalid_utf8);
                 }
 
                 matcher.subcommand(SubCommand {
@@ -1803,10 +1806,10 @@ impl<'help, 'app> Parser<'help, 'app> {
 
     /// Increase occurrence of specific argument and the grouped arg it's in.
     fn inc_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) {
-        matcher.inc_occurrence_of(&arg.id, arg.is_set(ArgSettings::IgnoreCase));
+        matcher.inc_occurrence_of_arg(arg);
         // Increment or create the group "args"
         for group in self.app.groups_for_arg(&arg.id) {
-            matcher.inc_occurrence_of(&group, false);
+            matcher.inc_occurrence_of_group(&group);
         }
     }
 }