Merge pull request #3781 from epage/get_one

fix(parser): Don't treat missing values as missing args

src/parser/error.rs

@@ -1,7 +1,7 @@
 use crate::util::Id;
 
 /// Violation of [`ArgMatches`][crate::ArgMatches] assumptions
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug)]
 #[allow(missing_copy_implementations)] // We might add non-Copy types in the future
 #[non_exhaustive]
 pub enum MatchesError {
@@ -18,6 +18,11 @@ pub enum MatchesError {
     UnknownArgument {
         // Missing `id` but blocked on a public id type which will hopefully come with `unstable-v4`
     },
+    /// Present argument must have one value
+    #[non_exhaustive]
+    ExpectedOne {
+        // Missing `id` but blocked on a public id type which will hopefully come with `unstable-v4`
+    },
 }
 
 impl MatchesError {
@@ -51,6 +56,9 @@ impl std::fmt::Display for MatchesError {
             Self::UnknownArgument {} => {
                 writeln!(f, "Unknown argument or group id.  Make sure you are using the argument id and not the short or long flags")
             }
+            Self::ExpectedOne {} => {
+                writeln!(f, "Present argument must have one value.  Make sure you are using the correct lookup (`get_one` vs `get_many`)")
+            }
         }
     }
 }

src/parser/matches/arg_matches.rs

@@ -1063,8 +1063,11 @@ impl ArgMatches {
     ) -> Result<Option<&T>, MatchesError> {
         let id = Id::from(name);
         let arg = self.try_get_arg_t::<T>(&id)?;
-        let value = match arg.and_then(|a| a.first()) {
-            Some(value) => value,
+        let value = match arg.map(|a| a.first()) {
+            Some(Some(value)) => value,
+            Some(None) => {
+                return Err(MatchesError::ExpectedOne {});
+            }
             None => {
                 return Ok(None);
             }

tests/builder/env.rs

@@ -38,7 +38,10 @@ fn env_bool_literal() {
     let m = r.unwrap();
     assert!(m.is_present("present"));
     assert_eq!(m.occurrences_of("present"), 0);
-    assert_eq!(m.get_one::<String>("present").map(|v| v.as_str()), None);
+    assert!(matches!(
+        m.try_get_one::<String>("present").unwrap_err(),
+        clap::parser::MatchesError::ExpectedOne { .. }
+    ));
     assert!(!m.is_present("negated"));
     assert!(!m.is_present("absent"));
 }

tests/builder/groups.rs

@@ -111,7 +111,10 @@ fn group_single_flag() {
 
     let m = res.unwrap();
     assert!(m.is_present("grp"));
-    assert!(m.get_one::<String>("grp").map(|v| v.as_str()).is_none());
+    assert!(matches!(
+        m.try_get_one::<String>("grp").unwrap_err(),
+        clap::parser::MatchesError::ExpectedOne { .. }
+    ));
 }
 
 #[test]

tests/builder/ignore_errors.rs

@@ -49,7 +49,10 @@ fn multiple_args_and_final_arg_without_value() {
         Some("file")
     );
     assert!(m.is_present("f"));
-    assert_eq!(m.get_one::<String>("stuff").map(|v| v.as_str()), None);
+    assert!(matches!(
+        m.try_get_one::<String>("stuff").unwrap_err(),
+        clap::parser::MatchesError::ExpectedOne { .. }
+    ));
 }
 
 #[test]
@@ -76,7 +79,10 @@ fn multiple_args_and_intermittent_arg_without_value() {
         Some("file")
     );
     assert!(m.is_present("f"));
-    assert_eq!(m.get_one::<String>("stuff").map(|v| v.as_str()), None);
+    assert!(matches!(
+        m.try_get_one::<String>("stuff").unwrap_err(),
+        clap::parser::MatchesError::ExpectedOne { .. }
+    ));
 }
 
 #[test]
@@ -118,7 +124,10 @@ fn subcommand() {
         sub_m.is_present("test"),
         "expected subcommand to be present due to partial parsing"
     );
-    assert_eq!(sub_m.get_one::<String>("test").map(|v| v.as_str()), None);
+    assert!(matches!(
+        sub_m.try_get_one::<String>("test").unwrap_err(),
+        clap::parser::MatchesError::ExpectedOne { .. }
+    ));
     assert_eq!(
         sub_m.get_one::<String>("stuff").map(|v| v.as_str()),
         Some("some other val")