Repository: pacak/bpaf (rust)

Issue:
Incorrect error message "no such flag" with valid option
Hi @pacak, thanks again for bpaf! I'm migrating some projects at work to it and ran into an incorrect error message which I've reduced to this case:

```rust
struct Args {
    a: u64,
    b: u64,
}

fn parser() -> OptionParser<Args> {
    let a = long("a").argument("a");
    let b = long("b").argument("b");
    construct!(Args { a, b }).to_options()
}
```

When run with arguments `--b 1` bpaf outputs ``no such flag: `--b`, did you mean `--a`?``

Pull request description:
Release 0.9.11
- Improve error messages in case where parser that occurs earlier in a block fails (Fixes #350)

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 79ac397..444b633 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "bpaf"
-version = "0.9.10"
+version = "0.9.11"
 edition = "2021"
 categories = ["command-line-interface"]
 description = "A simple Command Line Argument Parser with parser combinators"
diff --git a/Changelog.md b/Changelog.md
index 66b8a7e..9464409 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,6 +1,7 @@
 # Change Log
 
-
+## bpaf [0.9.11] - 2024-03-24
+- better error messages
 
 ## bpaf [0.9.10], bpaf_derive [0.5.10] - 2024-03-19
 - due to dependency change colored output for legacy version is no longer supported
diff --git a/README.md b/README.md
index cf194f5..eeca430 100644
--- a/README.md
+++ b/README.md
@@ -384,12 +384,12 @@ Usage --user=ARG
 
 
  [__cargo_doc2readme_dependencies_info]: ggGkYW0AYXSEG52uRQSwBdezG6GWW8ODAbr5G6KRmT_WpUB5G9hPmBcUiIp6YXKEG67Vn_d8EgCjGwx1q1eGjg8OG62q7Al6912tG9tijfvWADwPYWSBgmRicGFmZTAuOC4w
- [__link10]: https://docs.rs/bpaf/0.9.9/bpaf/?search=bpaf::Parser::fallback
- [__link11]: https://docs.rs/bpaf/0.9.9/bpaf/?search=parsers::NamedArg::switch
- [__link12]: https://docs.rs/bpaf/0.9.9/bpaf/?search=parsers::NamedArg::argument
- [__link13]: https://docs.rs/bpaf/0.9.9/bpaf/?search=params::positional
- [__link14]: https://docs.rs/bpaf/0.9.9/bpaf/?search=parsers::NamedArg::argument
- [__link15]: https://docs.rs/bpaf/0.9.9/bpaf/?search=params::positional
- [__link16]: https://docs.rs/bpaf/0.9.9/bpaf/?search=bpaf::Parser::complete
- [__link18]: https://docs.rs/bpaf/0.9.9/bpaf/?search=info::OptionParser::run_inner
- [__link21]: https://docs.rs/bpaf/0.9.9/bpaf/?search=info::OptionParser::render_markdown
+ [__link10]: https://docs.rs/bpaf/0.9.11/bpaf/?search=bpaf::Parser::fallback
+ [__link11]: https://docs.rs/bpaf/0.9.11/bpaf/?search=parsers::NamedArg::switch
+ [__link12]: https://docs.rs/bpaf/0.9.11/bpaf/?search=parsers::NamedArg::argument
+ [__link13]: https://docs.rs/bpaf/0.9.11/bpaf/?search=params::positional
+ [__link14]: https://docs.rs/bpaf/0.9.11/bpaf/?search=parsers::NamedArg::argument
+ [__link15]: https://docs.rs/bpaf/0.9.11/bpaf/?search=params::positional
+ [__link16]: https://docs.rs/bpaf/0.9.11/bpaf/?search=bpaf::Parser::complete
+ [__link18]: https://docs.rs/bpaf/0.9.11/bpaf/?search=info::OptionParser::run_inner
+ [__link21]: https://docs.rs/bpaf/0.9.11/bpaf/?search=info::OptionParser::render_markdown
diff --git a/src/args.rs b/src/args.rs
index 9b3bd96..78ed79e 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -245,7 +245,7 @@ pub use inner::State;
 mod inner {
     use std::{ops::Range, rc::Rc};
 
-    use crate::{error::Message, Args};
+    use crate::{error::Message, item::Item, Args};
 
     use super::{split_os_argument, Arg, ArgType, ItemState};
     #[derive(Clone, Debug)]
@@ -531,8 +531,19 @@ mod inner {
             start..start + span_size
         }
 
-        pub(crate) fn ranges(&self) -> ArgRangesIter {
-            ArgRangesIter { args: self, cur: 0 }
+        pub(crate) fn ranges(&'a self, item: &'a Item) -> ArgRangesIter<'a> {
+            let width = match item {
+                Item::Any { .. }
+                | Item::Positional { .. }
+                | Item::Command { .. }
+                | Item::Flag { .. } => 1,
+                Item::Argument { .. } => 2,
+            };
+            ArgRangesIter {
+                args: self,
+                cur: 0,
+                width,
+            }
         }
 
         pub(crate) fn scope(&self) -> Range<usize> {
@@ -573,10 +584,15 @@ mod inner {
 
     pub(crate) struct ArgRangesIter<'a> {
         args: &'a State,
+        width: usize,
         cur: usize,
     }
     impl<'a> Iterator for ArgRangesIter<'a> {
-        type Item = (usize, State);
+        type Item = (
+            /* start offset */ usize,
+            /* width of the first item */ usize,
+            State,
+        );
 
         fn next(&mut self) -> Option<Self::Item> {
             loop {
@@ -585,14 +601,17 @@ mod inner {
                     return None;
                 }
                 self.cur += 1;
-
                 if !self.args.present(cur)? {
                     continue;
                 }
-
+                if cur + self.width > self.args.items.len() {
+                    return None;
+                }
+                // It should be possible to optimize this code a bit
+                // by checking if first item can possibly
                 let mut args = self.args.clone();
                 args.set_scope(cur..self.args.items.len());
-                return Some((cur, args));
+                return Some((cur, self.width, args));
             }
         }
     }
diff --git a/src/buffer.rs b/src/buffer.rs
index 8f64f9b..bc44998 100644
--- a/src/buffer.rs
+++ b/src/buffer.rs
@@ -138,7 +138,7 @@ impl Doc {
                 self.write_char('-', Style::Literal);
                 self.write_char(*s, Style::Literal);
             }
-            ShortLong::Long(l) | ShortLong::ShortLong(_, l) => {
+            ShortLong::Long(l) | ShortLong::Both(_, l) => {
                 self.write_str("--", Style::Literal);
                 self.write_str(l, Style::Literal);
             }
diff --git a/src/complete_gen.rs b/src/complete_gen.rs
index 7aa9c11..10ee608 100644
--- a/src/complete_gen.rs
+++ b/src/complete_gen.rs
@@ -422,7 +422,7 @@ impl State {
 fn preferred_name(name: ShortLong) -> String {
     match name {
         ShortLong::Short(s) => format!("-{}", s),
-        ShortLong::Long(l) | ShortLong::ShortLong(_, l) => format!("--{}", l),
+        ShortLong::Long(l) | ShortLong::Both(_, l) => format!("--{}", l),
     }
 }
 
@@ -439,7 +439,7 @@ fn arg_matches(arg: &str, name: ShortLong) -> Option<String> {
     // allocations and cloning
     match name {
         ShortLong::Long(_) => {}
-        ShortLong::Short(s) | ShortLong::ShortLong(s, _) => {
+        ShortLong::Short(s) | ShortLong::Both(s, _) => {
             can_match |= arg
                 .strip_prefix('-')
                 .and_then(|a| a.strip_prefix(s))
@@ -450,7 +450,7 @@ fn arg_matches(arg: &str, name: ShortLong) -> Option<String> {
     // and long string too
     match name {
         ShortLong::Short(_) => {}
-        ShortLong::Long(l) | ShortLong::ShortLong(_, l) => {
+        ShortLong::Long(l) | ShortLong::Both(_, l) => {
             can_match |= arg.strip_prefix("--").map_or(false, |s| l.starts_with(s));
         }
     }
diff --git a/src/error.rs b/src/error.rs
index f6cacc3..df6e5f7 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -460,7 +460,7 @@ impl Message {
 
                         match v {
                             Variant::CommandLong(name) => doc.literal(name),
-                            Variant::Flag(ShortLong::Long(l) | ShortLong::ShortLong(_, l)) => {
+                            Variant::Flag(ShortLong::Long(l) | ShortLong::Both(_, l)) => {
                                 doc.literal("--");
                                 doc.literal(l);
                             }
diff --git a/src/item.rs b/src/item.rs
index f65e779..1efd554 100644
--- a/src/item.rs
+++ b/src/item.rs
@@ -64,19 +64,19 @@ impl Item {
 pub enum ShortLong {
     Short(char),
     Long(&'static str),
-    ShortLong(char, &'static str),
+    Both(char, &'static str),
 }
 
 impl ShortLong {
     pub(crate) fn as_long(&self) -> Option<&'static str> {
         match self {
-            ShortLong::Long(l) | ShortLong::ShortLong(_, l) => Some(l),
+            ShortLong::Long(l) | ShortLong::Both(_, l) => Some(l),
             ShortLong::Short(_) => None,
         }
     }
     pub(crate) fn as_short(&self) -> Option<char> {
         match self {
-            ShortLong::Short(s) | ShortLong::ShortLong(s, _) => Some(*s),
+            ShortLong::Short(s) | ShortLong::Both(s, _) => Some(*s),
             ShortLong::Long(_) => None,
         }
     }
@@ -94,7 +94,7 @@ impl PartialEq<&str> for ShortLong {
         match self {
             ShortLong::Short(s) => short_eq(*s, other),
             ShortLong::Long(l) => long_eq(l, other),
-            ShortLong::ShortLong(s, l) => short_eq(*s, other) || long_eq(l, other),
+            ShortLong::Both(s, l) => short_eq(*s, other) || long_eq(l, other),
         }
     }
 }
@@ -105,7 +105,7 @@ impl ShortLong {
     pub(crate) fn normalize(&mut self, short: bool) {
         match self {
             ShortLong::Short(_) | ShortLong::Long(_) => {}
-            ShortLong::ShortLong(s, l) => {
+            ShortLong::Both(s, l) => {
                 if short {
                     *self = Self::Short(*s);
                 } else {
@@ -124,7 +124,7 @@ impl TryFrom<&NamedArg> for ShortLong {
             (true, true) => Err(()),
             (true, false) => Ok(Self::Long(named.long[0])),
             (false, true) => Ok(Self::Short(named.short[0])),
-            (false, false) => Ok(Self::ShortLong(named.short[0], named.long[0])),
+            (false, false) => Ok(Self::Both(named.short[0], named.long[0])),
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 7f24c6e..60f0a40 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -432,69 +432,39 @@ macro_rules! construct {
     };
 
     (@prepare $ty:tt [$($fields:tt)*]) => {
-        $crate::__cons_prepare!($ty [ $($fields)* ])
+        $crate::construct!(@fin $ty [ $($fields)* ])
     };
 
     (@make [named [$($con:tt)+]] [$($fields:ident)*]) => { $($con)+ { $($fields),* } };
     (@make [pos   [$($con:tt)+]] [$($fields:ident)*]) => { $($con)+ ( $($fields),* ) };
     (@make [pos] [$($fields:ident)*]) => { ( $($fields),* ) };
-}
 
-#[macro_export]
-#[doc(hidden)]
-#[cfg(not(feature = "autocomplete"))]
-/// to avoid extra parsing when autocomplete feature is off
-macro_rules! __cons_prepare {
-    ([named [$($con:tt)+]] []) => { $crate::pure($($con)+ { })};
-    ([pos   [$($con:tt)+]] []) => { $crate::pure($($con)+ ( ))};
-
-    ([pos] [$field:ident]) => {{
-        use $crate::Parser;
-        $field.boxed()
-    }};
-
-    ($ty:tt [$($fields:ident)+]) => {{
-        use $crate::Parser;
-        let meta = $crate::Meta::And(vec![ $($fields.meta()),+ ]);
-        let inner = move |args: &mut $crate::State| {
-            $(let $fields = $fields.eval(args)?;)+
-            args.current = None;
-            ::std::result::Result::Ok::<_, $crate::Error>
-                ($crate::construct!(@make $ty [$($fields)+]))
-        };
-        $crate::ParseCon { inner, meta }
-    }};
-}
 
-#[macro_export]
-#[doc(hidden)]
-#[cfg(feature = "autocomplete")]
-/// for completion bpaf needs to observe all the failures in a branch
-macro_rules! __cons_prepare {
-    ([named [$($con:tt)+]] []) => { $crate::pure($($con)+ { })};
-    ([pos   [$($con:tt)+]] []) => { $crate::pure($($con)+ ( ))};
+    (@fin [named [$($con:tt)+]] []) => { $crate::pure($($con)+ { })};
+    (@fin [pos   [$($con:tt)+]] []) => { $crate::pure($($con)+ ( ))};
 
-    ([pos] [$field:ident]) => {{
+    (@fin [pos] [$field:ident]) => {{
         use $crate::Parser;
         $field.boxed()
     }};
 
-    ($ty:tt [$($fields:ident)+]) => {{
+    (@fin $ty:tt [$front:ident $($fields:ident)*]) => {{
         use $crate::Parser;
-        let meta = $crate::Meta::And(vec![ $($fields.meta()),+ ]);
-        let inner = move |args: &mut $crate::State| {
-            $(let $fields = if args.is_comp() {
-                $fields.eval(args)
-            } else {
-                Ok($fields.eval(args)?)
-            };)+
-            $(let $fields = $fields?;)+
+        let meta = $crate::Meta::And(vec![ $front.meta(), $($fields.meta()),* ]);
+        let inner = move |failfast: bool, args: &mut $crate::State| {
+            let mut $front = $front.eval(args);
+            if failfast {
+                $front = Ok($front?);
+            }
+            $(let $fields = $fields.eval(args);)*
+            let $front = $front?;
+            $(let $fields = $fields?;)*
 
             args.current = None;
             ::std::result::Result::Ok::<_, $crate::Error>
-                ($crate::construct!(@make $ty [$($fields)+]))
+                ($crate::construct!(@make $ty [$front $($fields)*]))
         };
-        $crate::ParseCon { inner, meta }
+        $crate::ParseCon { inner, meta, failfast: false }
     }};
 }
 
diff --git a/src/meta.rs b/src/meta.rs
index 4956088..951259a 100644
--- a/src/meta.rs
+++ b/src/meta.rs
@@ -154,10 +154,10 @@ impl Meta {
     }
 
     /// Used by adjacent parsers since it inherits behavior of the front item
-    pub(crate) fn first_item(meta: &Meta) -> Option<Item> {
+    pub(crate) fn first_item(meta: &Meta) -> Option<&Item> {
         match meta {
             Meta::And(xs) => xs.first().and_then(Self::first_item),
-            Meta::Item(item) => Some(*item.clone()),
+            Meta::Item(item) => Some(item),
             Meta::Skip | Meta::Or(_) => None,
             Meta::Optional(x)
             | Meta::Strict(x)
diff --git a/src/meta_help.rs b/src/meta_help.rs
index f1e407a..d222afd 100644
--- a/src/meta_help.rs
+++ b/src/meta_help.rs
@@ -510,7 +510,7 @@ fn write_shortlong(buf: &mut Doc, name: ShortLong) {
             buf.write_str("    --", Style::Literal);
             buf.write_str(l, Style::Literal);
         }
-        ShortLong::ShortLong(s, l) => {
+        ShortLong::Both(s, l) => {
             buf.write_char('-', Style::Literal);
             buf.write_char(s, Style::Literal);
             buf.write_str(", ", Style::Text);
diff --git a/src/structs.rs b/src/structs.rs
index f3e7755..baf59fc 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -850,14 +850,21 @@ pub struct ParseCon<P> {
     pub inner: P,
     /// metas for inner parsers
     pub meta: Meta,
+    /// To produce a better error messages while parsing constructed values
+    /// we want to look at all the items so values that can be consumed are consumed
+    /// autocomplete relies on the same logic
+    ///
+    /// However when dealing with adjacent restriction detecting the first item relies on failing
+    /// fast
+    pub failfast: bool,
 }
 
 impl<T, P> Parser<T> for ParseCon<P>
 where
-    P: Fn(&mut State) -> Result<T, Error>,
+    P: Fn(bool, &mut State) -> Result<T, Error>,
 {
     fn eval(&self, args: &mut State) -> Result<T, Error> {
-        let res = (self.inner)(args);
+        let res = (self.inner)(self.failfast, args);
         args.current = None;
         res
     }
@@ -928,7 +935,8 @@ impl<T> ParseCon<T> {
     /// There's also similar method [`adjacent`](crate::parsers::ParseArgument) that allows to restrict argument
     /// parser to work only for arguments where both key and a value are in the same shell word:
     /// `-f=bar` or `-fbar`, but not `-f bar`.
-    pub fn adjacent(self) -> ParseAdjacent<Self> {
+    pub fn adjacent(mut self) -> ParseAdjacent<Self> {
+        self.failfast = true;
         ParseAdjacent { inner: self }
     }
 }
@@ -1048,9 +1056,12 @@ where
     fn eval(&self, args: &mut State) -> Result<T, Error> {
         let original_scope = args.scope();
 
-        let mut best_error = if let Some(item) = Meta::first_item(&self.inner.meta()) {
+        let first_item;
+        let inner_meta = self.inner.meta();
+        let mut best_error = if let Some(item) = Meta::first_item(&inner_meta) {
+            first_item = item;
             let missing_item = MissingItem {
-                item,
+                item: item.clone(),
                 position: original_scope.start,
                 scope: original_scope.clone(),
             };
@@ -1061,7 +1072,7 @@ where
         let mut best_args = args.clone();
         let mut best_consumed = 0;
 
-        for (start, mut this_arg) in args.ranges() {
+        for (start, width, mut this_arg) in args.ranges(first_item) {
             // since we only want to parse things to the right of the first item we perform
             // parsing in two passes:
             // - try to run the parser showing only single argument available at all the indices
@@ -1071,8 +1082,7 @@ where
             // consider examples "42 -n" and "-n 42"
             // without multi step approach first command line also parses into 42
             let mut scratch = this_arg.clone();
-            #[allow(clippy::range_plus_one)] // clippy suggests wrong type
-            scratch.set_scope(start..start + 1);
+            scratch.set_scope(start..start + width);
             let before = scratch.len();
 
             // nothing to consume, might as well skip this segment right now
@@ -1116,8 +1126,8 @@ where
                         if consumed > best_consumed {
                             best_consumed = consumed;
                             std::mem::swap(&mut best_args, &mut this_arg);
+                            best_error = err;
                         }
-                        best_error = err;
                         break;
                     }
                 }


Test patch:
diff --git a/tests/adjacent.rs b/tests/adjacent.rs
index 9a61382..6d4756f 100644
--- a/tests/adjacent.rs
+++ b/tests/adjacent.rs
@@ -188,3 +188,28 @@ Available commands:
 
     assert_eq!(r, expected);
 }
+
+#[test]
+fn two_adjacent_args() {
+    let x = short('x').argument::<usize>("X");
+    let y = short('y').argument::<usize>("Y");
+    let c = short('c').switch();
+    let point = construct!(x, y).adjacent();
+    let parser = construct!(point, c).to_options();
+
+    let r = parser.run_inner(&["-x", "3", "-y", "4", "-c"]).unwrap();
+    assert_eq!(r, ((3, 4), true));
+
+    // they are adjacent to each other, but the way it is coded currently - they must be adjacent
+    // to the first element.
+    // Proper fix is to split "adjacent" into "adjacent to" and "adjacent block"
+
+    // let r = parser.run_inner(&["-y", "3", "-x", "4", "-c"]).unwrap();
+    // assert_eq!(r, ((4, 3), true));
+
+    let r = parser
+        .run_inner(&["-y", "3", "-c", "-x", "4"])
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "expected `-y=Y`, pass `--help` for usage information");
+}
diff --git a/tests/anywhere.rs b/tests/anywhere.rs
index 93a54d2..464ad0d 100644
--- a/tests/anywhere.rs
+++ b/tests/anywhere.rs
@@ -57,10 +57,7 @@ fn parse_anywhere_no_catch() {
     assert_eq!(r, "couldn't parse `221b`: invalid digit found in string");
 
     let r = parser.run_inner(&["-a", "-c"]).unwrap_err().unwrap_stderr();
-    assert_eq!(
-        r,
-        "expected `X`, got `-c`. Pass `--help` for usage information"
-    );
+    assert_eq!(r, "expected `X`, pass `--help` for usage information");
 
     let r = parser
         .run_inner(&["-a", "221b", "-c"])
diff --git a/tests/errors.rs b/tests/errors.rs
index 2c72e41..36ae600 100644
--- a/tests/errors.rs
+++ b/tests/errors.rs
@@ -29,10 +29,10 @@ fn no_argument() {
     //    let r = parser.run_inner(&["-a", "-4"])).unwrap();
     //    assert_eq!(r, (-4, flse));
     let r = parser.run_inner(&["-a", "-2"]).unwrap_err().unwrap_stderr();
-    assert_eq!(
-        r,
-        "`-a` requires an argument `N`, got a flag `-2`, try `-a=-2` to use it as an argument"
-    );
+
+    // can we restore this?
+    // "`-a` requires an argument `N`, got a flag `-2`, try `-a=-2` to use it as an argument"
+    assert_eq!(r, "`-a` requires an argument `N`");
 }
 
 #[test]
@@ -262,3 +262,12 @@ fn guard_on_fallback() {
     let r = parser.run_inner(&[]).unwrap_err().unwrap_stderr();
     assert_eq!(r, "check failed: too big");
 }
+
+#[test]
+fn two_required_fields_first_missing() {
+    let a = long("a").argument::<u32>("A");
+    let b = long("b").argument::<u32>("B");
+    let parser = construct!(a, b).to_options();
+    let r = parser.run_inner(&["--b", "1"]).unwrap_err().unwrap_stderr();
+    assert_eq!(r, "expected `--a=A`, pass `--help` for usage information");
+}
diff --git a/tests/tests.rs b/tests/tests.rs
index 87702c6..15f8303 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -1760,8 +1760,7 @@ fn option_requires_other_option2() {
     let parser = construct!(b, a).optional().to_options();
 
     let r = parser.run_inner(&["-a"]).unwrap_err().unwrap_stderr();
-    // error message sucks...
-    assert_eq!(r, "`-a` is not expected in this context");
+    assert_eq!(r, "expected `-b=B`, pass `--help` for usage information");
 }
 
 #[test]