From 50f4018dcfb1f33cb092780e27f14bfed5b0ed2c Mon Sep 17 00:00:00 2001
From: Ed Page <eopage@gmail.com>
Date: Thu, 2 Jun 2022 13:45:18 -0500
Subject: [PATCH] fix(parser): Don't treat missing values as missing args

---
 src/parser/error.rs               |  8 ++++++++
 src/parser/matches/arg_matches.rs |  7 +++++--
 tests/builder/env.rs              |  5 ++++-
 tests/builder/groups.rs           |  5 ++++-
 tests/builder/ignore_errors.rs    | 15 ++++++++++++---
 5 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/parser/error.rs b/src/parser/error.rs
index bdafa9ae..e23ee5e3 100644
--- a/src/parser/error.rs
+++ b/src/parser/error.rs
@@ -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`)")
+            }
         }
     }
 }
diff --git a/src/parser/matches/arg_matches.rs b/src/parser/matches/arg_matches.rs
index 6b27174f..0df555f3 100644
--- a/src/parser/matches/arg_matches.rs
+++ b/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);
             }
diff --git a/tests/builder/env.rs b/tests/builder/env.rs
index 28129df3..14a37bbc 100644
--- a/tests/builder/env.rs
+++ b/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"));
 }
diff --git a/tests/builder/groups.rs b/tests/builder/groups.rs
index 2247b989..9ea39ada 100644
--- a/tests/builder/groups.rs
+++ b/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]
diff --git a/tests/builder/ignore_errors.rs b/tests/builder/ignore_errors.rs
index 547320c5..6b29bc0f 100644
--- a/tests/builder/ignore_errors.rs
+++ b/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")