Merge #1862

1862: Improve panic messages r=pksunkara a=CreepySkeleton



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

src/build/app/mod.rs

@@ -1579,6 +1579,22 @@ impl<'b> App<'b> {
         }
     }
 
+    fn two_args_of<F>(&self, condition: F) -> Option<(&Arg, &Arg)>
+    where
+        F: Fn(&Arg<'_>) -> bool,
+    {
+        two_elements_of(self.args.args.iter().filter(|a| condition(a)))
+    }
+
+    // just in case
+    #[allow(unused)]
+    fn two_groups_of<F>(&self, condition: F) -> Option<(&ArgGroup, &ArgGroup)>
+    where
+        F: Fn(&ArgGroup<'_>) -> bool,
+    {
+        two_elements_of(self.groups.iter().filter(|a| condition(a)))
+    }
+
     // Perform some expensive assertions on the Parser itself
     #[allow(clippy::cognitive_complexity)]
     fn _debug_asserts(&self) {
@@ -1589,51 +1605,55 @@ impl<'b> App<'b> {
 
             // Name conflicts
             assert!(
-                self.args.args.iter().filter(|x| x.id == arg.id).count() < 2,
-                "Argument name must be unique\n\n\t'{}' is already in use",
+                self.two_args_of(|x| x.name == arg.name).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 {
-                assert!(
-                    self.args.args.iter().filter(|x| x.long == Some(l)).count() < 2,
-                    "Argument long must be unique\n\n\t'--{}' is already in use",
-                    l
-                );
+                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 {
-                assert!(
-                    self.args.args.iter().filter(|x| x.short == Some(s)).count() < 2,
-                    "Argument short must be unique\n\n\t'-{}' is already in use",
-                    s
-                );
+                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 {
-                assert!(
-                    positionals!(self).fold(0, |acc, p| if p.index == Some(idx) {
-                        acc + 1
-                    } else {
-                        acc
-                    }) < 2,
-                    "Argument '{}' has the same index as another positional argument\n\n\t \
-                    Use Arg::setting(ArgSettings::MultipleValues) to allow one \
-                    positional argument to take multiple values",
-                    arg.name
-                );
+                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
             if let Some(reqs) = &arg.requires {
                 for req in reqs {
                     assert!(
-                        self.args.args.iter().any(|x| x.id == req.1)
-                            || self.groups.iter().any(|x| x.id == req.1),
-                        "Argument or group specified in 'requires*' for '{}' does not exist",
+                        self.id_exists(&req.1),
+                        "Argument or group '{:?}' specified in 'requires*' for '{}' does not exist",
+                        req.1,
                         arg.name,
                     );
                 }
@@ -1642,10 +1662,9 @@ impl<'b> App<'b> {
             if let Some(reqs) = &arg.r_ifs {
                 for req in reqs {
                     assert!(
-                        self.args.args.iter().any(|x| x.id == req.0)
-                            || self.groups.iter().any(|x| x.id == req.0),
-                        "Argument or group specified in 'required_if*' for '{}' does not exist",
-                        arg.name,
+                        self.id_exists(&req.0),
+                        "Argument or group '{:?}' specified in 'required_if*' for '{}' does not exist",
+                        req.0, arg.name
                     );
                 }
             }
@@ -1653,10 +1672,9 @@ impl<'b> App<'b> {
             if let Some(reqs) = &arg.r_unless {
                 for req in reqs {
                     assert!(
-                        self.args.args.iter().any(|x| x.id == *req)
-                            || self.groups.iter().any(|x| x.id == *req),
-                        "Argument or group specified in 'required_unless*' for '{}' does not exist",
-                        arg.name,
+                        self.id_exists(req),
+                        "Argument or group '{:?}' specified in 'required_unless*' for '{}' does not exist",
+                        req, arg.name,
                     );
                 }
             }
@@ -1665,10 +1683,9 @@ impl<'b> App<'b> {
             if let Some(reqs) = &arg.blacklist {
                 for req in reqs {
                     assert!(
-                        self.args.args.iter().any(|x| x.id == *req)
-                            || self.groups.iter().any(|x| x.id == *req),
-                        "Argument or group specified in 'conflicts_with*' for '{}' does not exist",
-                        arg.name,
+                        self.id_exists(req),
+                        "Argument or group '{:?}' specified in 'conflicts_with*' for '{}' does not exist",
+                        req, arg.name,
                     );
                 }
             }
@@ -1676,19 +1693,19 @@ impl<'b> App<'b> {
             if arg.is_set(ArgSettings::Last) {
                 assert!(
                     arg.long.is_none(),
-                    "Flags or Options may not have last(true) set. '{}' has both a long and last(true) set.",
+                    "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 may not have last(true) set. '{}' has both a short and last(true) set.",
+                    "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 global and required",
+                "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required",
                 arg.name
             );
         }
@@ -1712,8 +1729,9 @@ impl<'b> App<'b> {
             for arg in &group.args {
                 assert!(
                     self.args.args.iter().any(|x| x.id == *arg),
-                    "Argument group '{}' contains non-existent argument",
+                    "Argument group '{}' contains non-existent argument '{:?}'",
                     group.name,
+                    arg
                 )
             }
         }
@@ -1978,6 +1996,10 @@ impl<'b> App<'b> {
             .any(|sc| !sc.is_set(AppSettings::Hidden))
     }
 
+    pub(crate) fn id_exists(&self, id: &Id) -> bool {
+        self.args.args.iter().any(|x| x.id == *id) || self.groups.iter().any(|x| x.id == *id)
+    }
+
     pub(crate) fn unroll_args_in_group(&self, group: &Id) -> Vec<Id> {
         debug!("App::unroll_args_in_group: group={:?}", group);
         let mut g_vec = vec![group];
@@ -2198,3 +2220,16 @@ impl<'e> fmt::Display for App<'e> {
         write!(f, "{}", self.name)
     }
 }
+
+fn two_elements_of<I, T>(mut iter: I) -> Option<(T, T)>
+where
+    I: Iterator<Item = T>,
+{
+    let first = iter.next();
+    let second = iter.next();
+
+    match (first, second) {
+        (Some(first), Some(second)) => Some((first, second)),
+        _ => None,
+    }
+}

src/util/id.rs

@@ -47,8 +47,11 @@ impl Id {
 impl Debug for Id {
     fn fmt(&self, f: &mut Formatter) -> Result {
         #[cfg(debug_assertions)]
-        write!(f, "{:?} ", self.name)?;
-        write!(f, "[hash: {}]", self.id)
+        write!(f, "{}", self.name)?;
+        #[cfg(not(debug_assertions))]
+        write!(f, "[hash: {}]", self.id)?;
+
+        Ok(())
     }
 }
 

src/util/mod.rs

@@ -4,7 +4,6 @@ mod argstr;
 mod fnv;
 mod graph;
 mod id;
-mod strext;
 
 pub use self::fnv::Key;
 

src/util/strext.rs

@@ -1,16 +0,0 @@
-pub(crate) trait _StrExt {
-    fn _is_char_boundary(&self, index: usize) -> bool;
-}
-
-impl _StrExt for str {
-    #[inline]
-    fn _is_char_boundary(&self, index: usize) -> bool {
-        if index == self.len() {
-            return true;
-        }
-
-        self.as_bytes()
-            .get(index)
-            .map_or(false, |&b| b < 128 || b >= 192)
-    }
-}

tests/conflicts.rs

@@ -194,7 +194,7 @@ fn self_conflicting_arg() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument or group specified in 'conflicts_with*' for 'config' does not exist"]
+#[should_panic = "Argument or group 'extra' specified in 'conflicts_with*' for 'config' does not exist"]
 fn conflicts_with_invalid_arg() {
     let _ = App::new("prog")
         .arg(

tests/help.rs

@@ -1199,7 +1199,7 @@ fn arg_short_conflict_with_help() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument short must be unique\n\n\t'-h' is already in use"]
+#[should_panic = "Short option names must be unique for each argument, but '-h' is in use by both 'home' and 'help'"]
 fn arg_short_conflict_with_help_mut_arg() {
     let _ = App::new("conflict")
         .arg(Arg::with_name("home").short('h'))

tests/require.rs

@@ -731,7 +731,7 @@ fn issue_1643_args_mutually_require_each_other() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument or group specified in 'requires*' for 'config' does not exist"]
+#[should_panic = "Argument or group 'extra' specified in 'requires*' for 'config' does not exist"]
 fn requires_invalid_arg() {
     let _ = App::new("prog")
         .arg(Arg::with_name("config").requires("extra").long("config"))
@@ -740,7 +740,7 @@ fn requires_invalid_arg() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument or group specified in 'requires*' for 'config' does not exist"]
+#[should_panic = "Argument or group 'extra' specified in 'requires*' for 'config' does not exist"]
 fn requires_if_invalid_arg() {
     let _ = App::new("prog")
         .arg(
@@ -753,7 +753,7 @@ fn requires_if_invalid_arg() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument or group specified in 'required_if*' for 'config' does not exist"]
+#[should_panic = "Argument or group 'extra' specified in 'required_if*' for 'config' does not exist"]
 fn required_if_invalid_arg() {
     let _ = App::new("prog")
         .arg(
@@ -766,7 +766,7 @@ fn required_if_invalid_arg() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument or group specified in 'required_unless*' for 'config' does not exist"]
+#[should_panic = "Argument or group 'extra' specified in 'required_unless*' for 'config' does not exist"]
 fn required_unless_invalid_arg() {
     let _ = App::new("prog")
         .arg(

tests/unique_args.rs

@@ -2,7 +2,7 @@ use clap::{App, Arg};
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument name must be unique\n\n\t'arg1' is already in use"]
+#[should_panic = "Argument names must be unique, but 'arg1' is in use by more than one argument or group"]
 fn unique_arg_names() {
     let _ = App::new("some")
         .args(&[
@@ -14,7 +14,7 @@ fn unique_arg_names() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument short must be unique\n\n\t'-a' is already in use"]
+#[should_panic = "Short option names must be unique for each argument, but '-a' is in use by both 'arg1' and 'arg2'"]
 fn unique_arg_shorts() {
     let _ = App::new("some")
         .args(&[
@@ -26,7 +26,7 @@ fn unique_arg_shorts() {
 
 #[cfg(debug_assertions)]
 #[test]
-#[should_panic = "Argument long must be unique\n\n\t'--long' is already in use"]
+#[should_panic = "Long option names must be unique for each argument, but '--long' is in use by both 'arg1' and 'arg2'"]
 fn unique_arg_longs() {
     let _ = App::new("some")
         .args(&[