Repository: pacak/bpaf
Pull request #210 (2023-05-14 16:48:54)

Improve error messages a bit more
Pick the first mismatch in case of mutually exclusive branches instead of first parsed element.


Patch:
diff --git a/src/args.rs b/src/args.rs
index 32fee2e..f5fe46f 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -90,9 +90,6 @@ mod inner {
         /// "deeper" parser should win in or_else branches
         pub depth: usize,
 
-        /// used to pick the parser that consumes the left most item
-        pub(crate) head: usize,
-
         /// don't try to suggest any more positional items after there's a positional item failure
         /// or parsing in progress
         #[cfg(feature = "autocomplete")]
@@ -173,7 +170,6 @@ mod inner {
                 scope: 0..vec.len(),
                 items: Rc::from(vec),
                 current: None,
-                head: usize::MAX,
                 depth: 0,
                 #[cfg(feature = "autocomplete")]
                 comp: None,
@@ -241,12 +237,26 @@ mod inner {
             if self.scope.contains(&index) && self.item_state[index].present() {
                 self.current = Some(index);
                 self.remaining -= 1;
-                self.head = self.head.min(index);
                 self.item_state[index] = ItemState::Parsed;
             }
         }
 
-        pub(crate) fn conflict(self: &Args) -> Option<(usize, usize)> {
+        pub(crate) fn pick_winner(&self, other: &Self) -> (bool, Option<usize>) {
+            for (ix, (me, other)) in self
+                .item_state
+                .iter()
+                .zip(other.item_state.iter())
+                .enumerate()
+            {
+                if me.parsed() ^ other.parsed() {
+                    return (me.parsed(), Some(ix));
+                }
+            }
+            (true, None)
+        }
+
+        /// find first saved conflict
+        pub(crate) fn conflict(&self) -> Option<(usize, usize)> {
             let (ix, _item) = self.items_iter().next()?;
             if let ItemState::Conflict(other) = self.item_state.get(ix)? {
                 Some((ix, *other))
@@ -255,10 +265,10 @@ mod inner {
             }
         }
 
-        pub(crate) fn save_conflicts(&mut self, loser: &Args) {
+        pub(crate) fn save_conflicts(&mut self, loser: &Args, win: usize) {
             for (winner, loser) in self.item_state.iter_mut().zip(loser.item_state.iter()) {
                 if winner.present() && loser.parsed() {
-                    *winner = ItemState::Conflict(self.head);
+                    *winner = ItemState::Conflict(win);
                 }
             }
         }
diff --git a/src/params.rs b/src/params.rs
index 52b39ec..507c6ad 100644
--- a/src/params.rs
+++ b/src/params.rs
@@ -595,7 +595,6 @@ impl<T> Parser<T> for ParseCommand<T> {
                 args.set_scope(cur..args.scope().end);
             }
 
-            args.head = usize::MAX;
             args.depth += 1;
             if !self.adjacent {
                 self.subparser
diff --git a/src/structs.rs b/src/structs.rs
index 40935e7..1e70d85 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -200,9 +200,7 @@ impl<T> Parser<T> for ParseOrElse<T> {
         // if they both succed - pick the one that consumes left, remember the second one
         // if one succeeds - pick that, forget the remaining one unless we are doing completion
         let mut args_a = args.clone();
-        args_a.head = usize::MAX;
         let mut args_b = args.clone();
-        args_b.head = usize::MAX;
 
         // run both parsers, expand Result<T, Error> into Option<T> + Option<Error>
         // so that code that does a bunch of comparing logic can be shared across
@@ -283,33 +281,31 @@ fn this_or_that_picks_first(
         comp_stash.extend(b.drain_comps());
     }
 
-    let ok_a = err_a.is_none();
-    let ok_b = err_b.is_none();
     // otherwise pick based on the left most or successful one
     #[allow(clippy::let_and_return)] // <- it is without autocomplete only
     let res = match (err_a, err_b) {
         (None, None) => {
-            if args_a.head <= args_b.head {
-                Ok(true)
+            if args.len() == args_a.len() && args.len() == args_b.len() {
+                Ok((true, None))
             } else {
-                Ok(false)
+                Ok(args_a.pick_winner(args_b))
             }
         }
         (Some(e1), Some(e2)) => Err(e1.combine_with(e2)),
         // otherwise either a or b are success, true means a is success
-        (a_ok, _) => Ok(a_ok.is_none()),
+        (a_ok, _) => Ok((a_ok.is_none(), None)),
     };
 
     match res {
-        Ok(true) => {
-            if ok_b {
-                args_a.save_conflicts(args_b);
+        Ok((true, ix)) => {
+            if let Some(win) = ix {
+                args_a.save_conflicts(args_b, win);
             }
             std::mem::swap(args, args_a);
         }
-        Ok(false) => {
-            if ok_a {
-                args_b.save_conflicts(args_a);
+        Ok((false, ix)) => {
+            if let Some(win) = ix {
+                args_b.save_conflicts(args_a, win);
             }
             std::mem::swap(args, args_b);
         }
@@ -322,7 +318,7 @@ fn this_or_that_picks_first(
         comp.extend_comps(comp_stash);
     }
 
-    res
+    Ok(res?.0)
 }
 
 /// Parser that transforms parsed value with a failing function, created with


Test patch:
diff --git a/tests/errors.rs b/tests/errors.rs
index e22185e..f25ab2b 100644
--- a/tests/errors.rs
+++ b/tests/errors.rs
@@ -1,5 +1,42 @@
 use bpaf::*;
 
+#[test]
+fn this_or_that_odd() {
+    let a = short('a').req_flag(());
+    let b = short('b').req_flag(());
+    let ab = construct!(a, b);
+    let a = short('a').req_flag(());
+    let c = short('c').req_flag(());
+    let cd = construct!(a, c);
+    let parser = construct!([ab, cd]).to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["-a", "-b", "-c"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "\"-c\" cannot be used at the same time as \"-b\"");
+}
+
+#[test]
+fn cannot_be_used_partial_arg() {
+    let a = short('a').req_flag(10);
+    let b = short('b').argument::<usize>("ARG");
+    let parser = construct!([a, b]).to_options();
+
+    // TODO - error message can be improved...
+    let res = parser
+        .run_inner(Args::from(&["-b", "-a"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "-b is not expected in this context");
+
+    let res = parser
+        .run_inner(Args::from(&["-a", "-b"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "-b is not expected in this context");
+}
+
 #[test]
 fn better_error_with_enum() {
     #[derive(Debug, Clone, Bpaf)]