From b7e1a91c3ee92e2b0bd5cfef0429eb0ee5ae1f2a Mon Sep 17 00:00:00 2001
From: Michael Baikov <manpacket@gmail.com>
Date: Fri, 29 Sep 2023 20:08:49 -0400
Subject: [PATCH 1/2] Update generated docs

---
 src/docs2/compose_basic_to_options.md | 2 +-
 src/docs2/custom_help_version.md      | 4 ++--
 src/docs2/to_options.md               | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/docs2/compose_basic_to_options.md b/src/docs2/compose_basic_to_options.md
index cbd2f18f..97a5f472 100644
--- a/src/docs2/compose_basic_to_options.md
+++ b/src/docs2/compose_basic_to_options.md
@@ -57,7 +57,7 @@ The other one is `--version` - passing a string literal or something like
 
 <div class='bpaf-doc'>
 $ app --version<br>
-Version: 3.1415
+<p>Version: 3.1415</p>
 <style>
 div.bpaf-doc {
     padding: 14px;
diff --git a/src/docs2/custom_help_version.md b/src/docs2/custom_help_version.md
index d7868e53..43f7cc23 100644
--- a/src/docs2/custom_help_version.md
+++ b/src/docs2/custom_help_version.md
@@ -115,7 +115,7 @@ both working
 
 <div class='bpaf-doc'>
 $ app --version<br>
-Version: 0.42
+<p>Version: 0.42</p>
 <style>
 div.bpaf-doc {
     padding: 14px;
@@ -134,7 +134,7 @@ div.bpaf-doc  { padding-left: 1em; }
 
 <div class='bpaf-doc'>
 $ app -v<br>
-Version: 0.42
+<p>Version: 0.42</p>
 <style>
 div.bpaf-doc {
     padding: 14px;
diff --git a/src/docs2/to_options.md b/src/docs2/to_options.md
index 5f6245d7..0698fefc 100644
--- a/src/docs2/to_options.md
+++ b/src/docs2/to_options.md
@@ -82,7 +82,7 @@ The other one is `--version` - passing a string literal or something like
 
 <div class='bpaf-doc'>
 $ app --version<br>
-Version: 3.1415
+<p>Version: 3.1415</p>
 <style>
 div.bpaf-doc {
     padding: 14px;

From 54805b230857c23962fa25dc2c674db66e7ec442 Mon Sep 17 00:00:00 2001
From: Michael Baikov <manpacket@gmail.com>
Date: Sat, 30 Sep 2023 09:05:49 -0400
Subject: [PATCH 2/2] change take_positional_word to do a scan

instead of picking the next unused one.

`positional` should be used either after all the named items or as an
item adjacent to a named item in some way, still after all the named
items within the scope.

In case of valid user input scanning for or picking up the front item
gives the same results. When user specifies an invalid item - it will
not be consumed during normal scanning so picking the front item fails.

Error from positional item failing with "not present" will take the
priority over "not expected" from invalid item resulting in a bad error
message.
---
 Changelog.md                              |  1 +
 src/args.rs                               | 37 ++++++++----------
 src/docs2/adjacent_struct_0.md            |  2 +-
 src/docs2/derive_basic_custom_consumer.md |  2 +-
 src/params.rs                             | 47 ++++++++++-------------
 tests/errors.rs                           | 32 +++++++++++++++
 6 files changed, 72 insertions(+), 49 deletions(-)

diff --git a/Changelog.md b/Changelog.md
index 79301979..25891873 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -3,6 +3,7 @@
 ## bpaf [0.9.6], bpaf_derive [0.5.6] - Unreleased
 - make sure env-only arguments and flags are working
 - support raw identifiers in derive macro (#282)
+- better error messages for unexpected values that prevent positional parses
 
 ## bpaf [0.9.5], bpaf_derive [0.5.5] - 2023-08-24
 - fancier squashing: parse `-abfoo` as `-a -b=foo` if b is a short argument
diff --git a/src/args.rs b/src/args.rs
index 91924e12..963c0ab9 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -676,33 +676,30 @@ impl State {
     pub(crate) fn take_positional_word(
         &mut self,
         metavar: Metavar,
-    ) -> Result<Option<(usize, bool, OsString)>, Error> {
-        #[allow(clippy::range_plus_one)] // gives wrong type otherwise
-        match self.items_iter().next() {
-            Some((ix, Arg::PosWord(w))) => {
+    ) -> Result<(usize, bool, OsString), Error> {
+        match self.items_iter().find_map(|(ix, arg)| match arg {
+            Arg::Word(w) => Some((ix, false, w)),
+            Arg::PosWord(w) => Some((ix, true, w)),
+            _ => None,
+        }) {
+            Some((ix, strict, w)) => {
                 let w = w.clone();
                 self.current = Some(ix);
                 self.remove(ix);
-                Ok(Some((ix, true, w)))
+                Ok((ix, strict, w))
             }
-            Some((ix, Arg::Word(w))) => {
-                let w = w.clone();
-                self.current = Some(ix);
-                self.remove(ix);
-                Ok(Some((ix, false, w)))
-            }
-            Some((position, _arg)) => {
+            None => {
+                let scope = self.scope();
                 let missing = MissingItem {
                     item: Item::Positional {
                         help: None,
                         metavar,
                     },
-                    position,
-                    scope: position..position + 1,
+                    position: scope.start,
+                    scope,
                 };
                 Err(Error(Message::Missing(vec![missing])))
             }
-            None => Ok(None),
         }
     }
 
@@ -760,7 +757,7 @@ mod tests {
         let flag = a.take_flag(&long("speed"));
         assert!(flag);
         assert!(!a.is_empty());
-        let s = a.take_positional_word(M).unwrap().unwrap();
+        let s = a.take_positional_word(M).unwrap();
         assert_eq!(s.2, "12");
         assert!(a.is_empty());
     }
@@ -856,7 +853,7 @@ mod tests {
     fn command_and_positional() {
         let mut a = State::from(&["cmd", "pos"]);
         assert!(a.take_cmd("cmd"));
-        let w = a.take_positional_word(M).unwrap().unwrap();
+        let w = a.take_positional_word(M).unwrap();
         assert_eq!(w.2, "pos");
         assert!(a.is_empty());
     }
@@ -865,7 +862,7 @@ mod tests {
     fn positionals_after_double_dash1() {
         let mut a = State::from(&["-v", "--", "-x"]);
         assert!(a.take_flag(&short('v')));
-        let w = a.take_positional_word(M).unwrap().unwrap();
+        let w = a.take_positional_word(M).unwrap();
         assert_eq!(w.2, "-x");
         assert!(a.is_empty());
     }
@@ -874,7 +871,7 @@ mod tests {
     fn positionals_after_double_dash2() {
         let mut a = State::from(&["-v", "--", "-x"]);
         assert!(a.take_flag(&short('v')));
-        let w = a.take_positional_word(M).unwrap().unwrap();
+        let w = a.take_positional_word(M).unwrap();
         assert_eq!(w.2, "-x");
         assert!(a.is_empty());
     }
@@ -884,7 +881,7 @@ mod tests {
         let mut a = State::from(&["-v", "12", "--", "-x"]);
         let w = a.take_arg(&short('v'), false, M).unwrap().unwrap();
         assert_eq!(w, "12");
-        let w = a.take_positional_word(M).unwrap().unwrap();
+        let w = a.take_positional_word(M).unwrap();
         assert_eq!(w.2, "-x");
         assert!(a.is_empty());
     }
diff --git a/src/docs2/adjacent_struct_0.md b/src/docs2/adjacent_struct_0.md
index db727933..986b92a5 100644
--- a/src/docs2/adjacent_struct_0.md
+++ b/src/docs2/adjacent_struct_0.md
@@ -139,7 +139,7 @@ Options { point: [Point { point: (), x: 10, y: 20, z: 3.1415 }, Point { point: (
 
 <div class='bpaf-doc'>
 $ app --point 10 20 --rotate 3.1415<br>
-<b>Error:</b> expected <tt><i>Z</i></tt>, got <b>--rotate</b>. Pass <tt><b>--help</b></tt> for usage information
+<b>Error:</b> expected <tt><i>Z</i></tt>, pass <tt><b>--help</b></tt> for usage information
 <style>
 div.bpaf-doc {
     padding: 14px;
diff --git a/src/docs2/derive_basic_custom_consumer.md b/src/docs2/derive_basic_custom_consumer.md
index 0a6d6b63..85fcf867 100644
--- a/src/docs2/derive_basic_custom_consumer.md
+++ b/src/docs2/derive_basic_custom_consumer.md
@@ -64,7 +64,7 @@ long name is missing
 
 <div class='bpaf-doc'>
 $ app --switch 42<br>
-<b>Error:</b> expected <tt><i>NUM</i></tt>, got <b>--switch</b>. Pass <tt><b>--help</b></tt> for usage information
+<b>Error:</b> <b>--switch</b> is not expected in this context
 <style>
 div.bpaf-doc {
     padding: 14px;
diff --git a/src/params.rs b/src/params.rs
index 6c881513..4a33f75a 100644
--- a/src/params.rs
+++ b/src/params.rs
@@ -794,36 +794,29 @@ fn parse_pos_word(
     help: &Option<Doc>,
 ) -> Result<OsString, Error> {
     let metavar = Metavar(metavar);
-    if let Some((ix, is_strict, word)) = args.take_positional_word(metavar)? {
-        if strict && !is_strict {
-            #[cfg(feature = "autocomplete")]
-            args.push_pos_sep();
+    match args.take_positional_word(metavar) {
+        Ok((ix, is_strict, word)) => {
+            if strict && !is_strict {
+                #[cfg(feature = "autocomplete")]
+                args.push_pos_sep();
 
-            return Err(Error(Message::StrictPos(ix, metavar)));
-        }
-        #[cfg(feature = "autocomplete")]
-        if args.touching_last_remove() && !args.check_no_pos_ahead() {
-            args.push_metavar(metavar.0, help, false);
-            args.set_no_pos_ahead();
+                return Err(Error(Message::StrictPos(ix, metavar)));
+            }
+            #[cfg(feature = "autocomplete")]
+            if args.touching_last_remove() && !args.check_no_pos_ahead() {
+                args.push_metavar(metavar.0, help, false);
+                args.set_no_pos_ahead();
+            }
+            Ok(word)
         }
-        Ok(word)
-    } else {
-        #[cfg(feature = "autocomplete")]
-        if !args.check_no_pos_ahead() {
-            args.push_metavar(metavar.0, help, false);
-            args.set_no_pos_ahead();
+        Err(err) => {
+            #[cfg(feature = "autocomplete")]
+            if !args.check_no_pos_ahead() {
+                args.push_metavar(metavar.0, help, false);
+                args.set_no_pos_ahead();
+            }
+            Err(err)
         }
-
-        let position = args.items_iter().next().map_or(args.scope().end, |x| x.0);
-        let missing = MissingItem {
-            item: Item::Positional {
-                metavar,
-                help: help.clone(),
-            },
-            position,
-            scope: args.scope(),
-        };
-        Err(Error(Message::Missing(vec![missing])))
     }
 }
 
diff --git a/tests/errors.rs b/tests/errors.rs
index d433f04f..f55eff5d 100644
--- a/tests/errors.rs
+++ b/tests/errors.rs
@@ -199,3 +199,35 @@ fn adjacent_option_complains_to() {
     // TODO - this should point to the whole "-ayam" thing
     assert_eq!(r, "couldn't parse `yam`: invalid digit found in string");
 }
+
+#[test]
+fn some_pos_with_invalid_flag() {
+    let a = short('a').switch();
+    let b = positional::<usize>("B").some("Want B");
+    let parser = construct!(a, b).to_options();
+
+    let r = parser.run_inner(&["-c", "12"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "`-c` is not expected in this context");
+
+    let r = parser.run_inner(&["12", "-c"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "`-c` is not expected in this context");
+}
+
+#[test]
+fn pos_with_invalid_arg() {
+    let a = short('a').argument::<usize>("A").optional();
+    let b = positional::<usize>("B");
+    let parser = construct!(a, b).to_options();
+
+    let r = parser.run_inner(&["-c", "12"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "`-c` is not expected in this context");
+
+    let r = parser.run_inner(&["12", "-c"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "`-c` is not expected in this context");
+
+    let r = parser.run_inner(&["-c", "t"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "couldn't parse `t`: invalid digit found in string");
+
+    let r = parser.run_inner(&["t", "-c"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "couldn't parse `t`: invalid digit found in string");
+}