Repository: pacak/bpaf
Pull request #203 (2023-04-24 17:44:21)

Better errros
This should improve error messages 
- inside anywhere blocks
- when positional interacts with non positional items
- extra tests :)

Fixes https://github.com/pacak/bpaf/issues/196


Patch:
diff --git a/Changelog.md b/Changelog.md
index a9c34af..827569b 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -21,6 +21,7 @@
 - env fallback can now be fully hidden
 - meta description refactor - invididual parsers should be described more consistently
   in all sorts of messages
+- better error messages with positionals and inside anywhere blocks
 
 ## bpaf [0.7.10], bpaf_derive [0.3.5] - 2013-03-19
 - improve error messages for typos like `-llvm` instead of `--llvm`
diff --git a/docs/src/anywhere/combine.rs b/docs/src/anywhere/combine.rs
index 43bf1d1..1cd052c 100644
--- a/docs/src/anywhere/combine.rs
+++ b/docs/src/anywhere/combine.rs
@@ -27,6 +27,7 @@ fn toggle_options(name: &'static str, help: &'static str) -> impl Parser<bool> {
             }
         })
         .anywhere()
+        .catch()
 }
 
 pub fn options() -> OptionParser<Options> {
diff --git a/docs/src/anywhere_1/cases.txt b/docs/src/anywhere_1/cases.txt
index 01d5f02..6621917 100644
--- a/docs/src/anywhere_1/cases.txt
+++ b/docs/src/anywhere_1/cases.txt
@@ -12,4 +12,4 @@ Options { multi_arg: Some(MultiArg { set: (), name: "name", value: "Bob" }), tur
 ? But not in between
 > --set name --turbo Bob
 Stderr
-Expected an argument <ARG>, got --turbo
+Expected <ARG>, pass --help for usage information
diff --git a/examples/find.rs b/examples/find.rs
index e810b60..fc622e8 100644
--- a/examples/find.rs
+++ b/examples/find.rs
@@ -72,7 +72,9 @@ fn perm() -> impl Parser<Option<Perm>> {
         Ok(perms)
     }
 
-    let tag = any::<String>("-mode").help("-mode (perm | -perm | /perm)");
+    let tag = any::<String>("-mode")
+        .help("-mode (perm | -perm | /perm)")
+        .guard(|x| x == "-mode", "");
     let mode = any::<String>("mode")
         .parse::<_, _, String>(|s| {
             if let Some(m) = s.strip_prefix('-') {
diff --git a/src/args.rs b/src/args.rs
index 50ab700..77b4e81 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -1,7 +1,9 @@
 use std::ffi::OsString;
 
 pub(crate) use crate::arg::*;
-use crate::{info::Info, meta_help::Metavar, parsers::NamedArg, Error, Meta, ParseFailure};
+use crate::{
+    info::Info, item::Item, meta_help::Metavar, parsers::NamedArg, Error, Meta, ParseFailure,
+};
 
 /// Shows which branch of [`ParseOrElse`] parsed the argument
 #[derive(Debug, Clone)]
@@ -641,10 +643,11 @@ impl Args {
                 self.remove(ix);
                 Ok(Some((false, w)))
             }
-            Some((_, arg)) => Err(Error::Message(
-                format!("Expected an argument {}, got {}", metavar, arg),
-                false,
-            )),
+            Some((_, _arg)) => Err(Error::Missing(vec![Item::Positional {
+                help: None,
+                metavar,
+                strict: false,
+            }])),
             None => Ok(None),
         }
     }
diff --git a/src/docs/anywhere.md b/src/docs/anywhere.md
index a1f455b..7ae5910 100644
--- a/src/docs/anywhere.md
+++ b/src/docs/anywhere.md
@@ -29,6 +29,7 @@ fn toggle_options(name: &'static str, help: &'static str) -> impl Parser<bool> {
             }
         })
         .anywhere()
+        .catch()
 }
 
 pub fn options() -> OptionParser<Options> {
diff --git a/src/docs/anywhere_1.md b/src/docs/anywhere_1.md
index 8669333..a604043 100644
--- a/src/docs/anywhere_1.md
+++ b/src/docs/anywhere_1.md
@@ -80,7 +80,7 @@ Options { multi_arg: Some(MultiArg { set: (), name: "name", value: "Bob" }), tur
 But not in between
 ```console
 % app --set name --turbo Bob
-Expected an argument <ARG>, got --turbo
+Expected <ARG>, pass --help for usage information
 ```
 
 </details>
diff --git a/src/docs/find.md b/src/docs/find.md
index daf4372..75bd0d4 100644
--- a/src/docs/find.md
+++ b/src/docs/find.md
@@ -73,7 +73,9 @@ fn perm() -> impl Parser<Option<Perm>> {
         Ok(perms)
     }
 
-    let tag = any::<String>("-mode").help("-mode (perm | -perm | /perm)");
+    let tag = any::<String>("-mode")
+        .help("-mode (perm | -perm | /perm)")
+        .guard(|x| x == "-mode", "");
     let mode = any::<String>("mode")
         .parse::<_, _, String>(|s| {
             if let Some(m) = s.strip_prefix('-') {
diff --git a/src/help.rs b/src/help.rs
index 926dc88..0b7bc24 100644
--- a/src/help.rs
+++ b/src/help.rs
@@ -2,7 +2,9 @@
 //!
 //! covers `--help`, `--version` etc.
 
-use crate::{info::Info, meta_help::render_help, short, Args, Error, Meta, ParseFailure, Parser};
+use crate::{
+    info::Info, item::Item, meta_help::render_help, short, Args, Error, Meta, ParseFailure, Parser,
+};
 
 struct ParseExtraParams {
     version: Option<&'static str>,
@@ -108,22 +110,29 @@ pub(crate) fn improve_error(
         }
         Some(Error::ParseFailure(f)) => return f,
         Some(Error::Message(msg, _)) => msg,
-        Some(Error::Missing(xs)) => {
-            let meta = Meta::Or(xs.iter().map(|i| Meta::from(i.clone())).collect::<Vec<_>>())
-                .normalized(false);
+        Some(Error::Missing(xs)) => summarize_missing(&xs, inner, args),
+    })
+}
 
-            if let Some(x) = args.peek() {
-                if let Some(msg) = crate::meta_youmean::suggest(args, inner) {
-                    msg
-                } else {
-                    format!(
-                        "Expected {}, got \"{}\". Pass --help for usage information",
-                        meta, x
-                    )
-                }
-            } else {
-                format!("Expected {}, pass --help for usage information", meta)
-            }
+#[inline(never)]
+pub(crate) fn summarize_missing(items: &[Item], inner: &Meta, args: &Args) -> String {
+    let meta = Meta::Or(
+        items
+            .iter()
+            .map(|i| Meta::from(i.clone()))
+            .collect::<Vec<_>>(),
+    )
+    .normalized(false);
+    if let Some(x) = args.peek() {
+        if let Some(msg) = crate::meta_youmean::suggest(args, inner) {
+            msg
+        } else {
+            format!(
+                "Expected {}, got \"{}\". Pass --help for usage information",
+                meta, x
+            )
         }
-    })
+    } else {
+        format!("Expected {}, pass --help for usage information", meta)
+    }
 }
diff --git a/src/meta_youmean.rs b/src/meta_youmean.rs
index 55107d4..9200cae 100644
--- a/src/meta_youmean.rs
+++ b/src/meta_youmean.rs
@@ -6,6 +6,7 @@ use crate::{
 };
 
 /// Looks for potential typos
+#[inline(never)]
 pub(crate) fn suggest(args: &Args, meta: &Meta) -> Option<String> {
     let arg = args.peek()?;
 
diff --git a/src/structs.rs b/src/structs.rs
index 96b4091..7f8c196 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -534,7 +534,7 @@ where
             }
 
             match err {
-                Error::Message(_, _) if catch => Ok(None),
+                Error::Message(_, can_catch) if can_catch || catch => Ok(None),
                 Error::Message(_, _) | Error::ParseFailure(_) => Err(err),
                 Error::Missing(_) => {
                     if args.len() == orig_args.len() {
@@ -841,38 +841,20 @@ where
     P: Parser<T> + Sized,
 {
     fn eval(&self, args: &mut Args) -> Result<T, Error> {
-        fn meta_items(meta: &Meta) -> Vec<Item> {
-            match meta {
-                Meta::And(xs) => {
-                    if xs.is_empty() {
-                        Vec::new()
-                    } else {
-                        meta_items(&xs[0])
-                    }
-                }
-                Meta::Or(xs) => {
-                    let mut res = Vec::new();
-                    for x in xs {
-                        res.extend(meta_items(x));
-                    }
-                    res
-                }
-                Meta::Item(i) => vec![*i.clone()],
-                Meta::Optional(m)
-                | Meta::Required(m)
-                | Meta::Many(m)
-                | Meta::Anywhere(m) // TODO?
-                | Meta::Decorated(m, _, _) => meta_items(m),
-                Meta::Skip | Meta::HideUsage(_) => Vec::new(),
-            }
-        }
-
         #[cfg(feature = "autocomplete")]
         let mut comp_items = Vec::new();
 
-        let mut best_missing = meta_items(&self.meta());
+        // the idea of "anywhere" parsing is to try to parse at all the "tails"
+        // of the arguments picking either first succesful parse or the "best" error
+        //
+        // consider a parser --set <NAME> <VAL>
+        // --set name val               <- good parser
+        // --set name --bogus val       <- want to complain on bogus here
+        // --set name                   <- want to demand <val>
 
-        let mut best_consumed = 0;
+        let mut best_args = args.clone();
+        let mut best_consumed = usize::MAX;
+        let mut best_error = Error::Missing(Vec::new());
 
         for (start, mut this_arg) in args.ranges() {
             // since we only want to parse things to the right of the first item we perform
@@ -905,31 +887,19 @@ where
                 //
                 // ParseFailure covers failures or --help/--version for the nested parsers
                 // anywhere shouldn't consume that
-                good @ (Ok(_) | Err(Error::ParseFailure(_))) => {
+                done @ (Ok(_) | Err(Error::ParseFailure(_))) => {
                     this_arg.copy_usage_from(args, 0..start);
                     std::mem::swap(&mut this_arg, args);
-                    return good;
+                    return done;
                 }
 
                 // failed to find something, try to improve previous error message and resume
-                Err(Error::Missing(items)) => {
-                    let consumed = args.len() - this_arg.len();
-                    if consumed >= best_consumed {
-                        best_missing = items;
-                        best_consumed = consumed;
-                        #[cfg(feature = "autocomplete")]
-                        this_arg.swap_comps_with(&mut comp_items);
-                    }
-                }
+                Err(err) => {
+                    if this_arg.len() < best_args.len() {
+                        best_error = err;
+                        best_consumed = this_arg.len();
 
-                // otherwise return the error message we get
-                otherwise @ Err(Error::Message(_, _)) => {
-                    let consumed = args.len() - this_arg.len();
-                    if consumed > 0 && !self.catch {
-                        this_arg.copy_usage_from(args, 0..start);
-                        #[cfg(feature = "autocomplete")]
-                        this_arg.swap_comps_with(&mut comp_items);
-                        return otherwise;
+                        best_args = this_arg;
                     }
                 }
             }
@@ -938,11 +908,37 @@ where
         #[cfg(feature = "autocomplete")]
         args.swap_comps_with(&mut comp_items);
 
-        if let Ok(val) = self.inner.eval(&mut Args::from(&[])) {
-            return Ok(val);
+        // also try to run the parser with no input
+        match self.inner.eval(&mut Args::from(&[])) {
+            done @ (Ok(_) | Err(Error::ParseFailure(_))) => {
+                return done;
+            }
+            Err(err) => {
+                if best_consumed == usize::MAX {
+                    best_error = err;
+                }
+            }
         }
 
-        Err(Error::Missing(best_missing))
+        // parser "anywhere" runs in a restricted context, using restricted
+        // context makes sense for error message reporting as well. Without this
+        // it would complain about unexpected flags that are unknown to self.inner, but known
+        // to the global parser.
+        //
+        // Removing unexpected things from the context limits error to "expected xxx", but
+        // breaks "perhaps you meant ..." part. So we run this logic here as well
+        let meta = self.inner.meta();
+        best_args.restrict_to_range(&(0..0));
+
+        Err(match best_error {
+            Error::Message(msg, catch) => Error::Message(msg, catch || self.catch),
+            err @ Error::ParseFailure(_) => err,
+            Error::Missing(missing) => {
+                let msg = crate::meta_youmean::suggest(&best_args, &meta)
+                    .unwrap_or_else(|| crate::help::summarize_missing(&missing, &meta, &best_args));
+                Error::Message(msg, self.catch)
+            }
+        })
     }
 
     fn meta(&self) -> Meta {


Test patch:
diff --git a/tests/anywhere.rs b/tests/anywhere.rs
index d3df13e..2080fee 100644
--- a/tests/anywhere.rs
+++ b/tests/anywhere.rs
@@ -30,14 +30,19 @@ fn parse_anywhere_no_catch() {
     let c = short('c').switch();
     let parser = construct!(ab, c).to_options();
 
+    // Usage: -a <x> [-c],
+
+    let r = parser
+        .run_inner(Args::from(&["3", "-a"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
+
     let r = parser
         .run_inner(Args::from(&["-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected <x>, got \"-a\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 
     let r = parser
         .run_inner(Args::from(&["-a", "221b"]))
@@ -49,10 +54,7 @@ fn parse_anywhere_no_catch() {
         .run_inner(Args::from(&["-c", "-a"]))
         .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(Args::from(&["-c", "-a", "221b"]))
@@ -64,22 +66,67 @@ fn parse_anywhere_no_catch() {
         .run_inner(Args::from(&["-a", "-c"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(r, "Expected an argument <x>, got -c");
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 
     let r = parser
         .run_inner(Args::from(&["-a", "221b", "-c"]))
         .unwrap_err()
         .unwrap_stderr();
     assert_eq!(r, "Couldn't parse \"221b\": invalid digit found in string");
+}
 
-    let r = parser
-        .run_inner(Args::from(&["3", "-a"]))
-        .unwrap_err()
-        .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected <x>, got \"3\". Pass --help for usage information"
-    );
+#[test]
+fn anywhere_catch_optional() {
+    let a = short('a').req_flag(());
+    let b = positional::<usize>("x");
+    let ab = construct!(a, b).anywhere().catch().optional();
+    let bc = short('a').switch();
+    let parser = construct!(ab, bc).to_options();
+
+    let r = parser.run_inner(Args::from(&["-a", "10"])).unwrap();
+    assert_eq!(r, (Some(((), 10)), false));
+
+    let r = parser.run_inner(Args::from(&["-a"])).unwrap();
+    assert_eq!(r, (None, true));
+
+    let r = parser.run_inner(Args::from(&[])).unwrap();
+    assert_eq!(r, (None, false));
+}
+
+#[test]
+fn anywhere_catch_many() {
+    let a = short('a').req_flag(());
+    let b = positional::<usize>("x");
+    let ab = construct!(a, b).anywhere().catch().many();
+    let bc = short('a').switch();
+    let parser = construct!(ab, bc).to_options();
+
+    let r = parser.run_inner(Args::from(&["-a", "10"])).unwrap();
+    assert_eq!(r, (vec![((), 10)], false));
+
+    let r = parser.run_inner(Args::from(&["-a"])).unwrap();
+    assert_eq!(r, (Vec::new(), true));
+
+    let r = parser.run_inner(Args::from(&[])).unwrap();
+    assert_eq!(r, (Vec::new(), false));
+}
+
+#[test]
+fn anywhere_catch_fallback() {
+    let a = short('a').req_flag(());
+    let b = positional::<usize>("x");
+    let ab = construct!(a, b).anywhere().catch().fallback(((), 10));
+    let bc = short('a').switch();
+    let parser = construct!(ab, bc).to_options();
+
+    let r = parser.run_inner(Args::from(&["-a", "12"])).unwrap();
+    assert_eq!(r, (((), 12), false));
+
+    let r = parser.run_inner(Args::from(&["-a"])).unwrap();
+    assert_eq!(r, (((), 10), true));
+
+    let r = parser.run_inner(Args::from(&[])).unwrap();
+    assert_eq!(r, (((), 10), false));
 }
 
 #[test]
@@ -91,67 +138,47 @@ fn parse_anywhere_catch_required() {
     let parser = construct!(ab, c).to_options();
 
     let r = parser
-        .run_inner(Args::from(&["-a"]))
+        .run_inner(Args::from(&["-c", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected <x>, got \"-a\". Pass --help for usage information"
-    );
+    // this should complain about unexpected -a
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 
     let r = parser
-        .run_inner(Args::from(&["-a", "221b"]))
+        .run_inner(Args::from(&["-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected -a <x>, got \"-a\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 
     let r = parser
-        .run_inner(Args::from(&["-c", "-a"]))
+        .run_inner(Args::from(&["-a", "221b"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected <x>, got \"-c\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Couldn't parse \"221b\": invalid digit found in string");
 
     let r = parser
         .run_inner(Args::from(&["-c", "-a", "221b"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected -a <x>, got \"-c\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Couldn't parse \"221b\": invalid digit found in string");
 
     let r = parser
         .run_inner(Args::from(&["-a", "-c"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected -a <x>, got \"-a\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 
     let r = parser
         .run_inner(Args::from(&["-a", "221b", "-c"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected -a <x>, got \"-a\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Couldn't parse \"221b\": invalid digit found in string");
 
     let r = parser
         .run_inner(Args::from(&["3", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(
-        r,
-        "Expected <x>, got \"3\". Pass --help for usage information"
-    );
+    assert_eq!(r, "Expected <x>, pass --help for usage information");
 }
 
 #[test]
@@ -162,6 +189,12 @@ fn parse_anywhere_catch_optional() {
     let c = short('c').switch();
     let parser = construct!(ab, c).to_options();
 
+    let r = parser
+        .run_inner(Args::from(&["3", "-a"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "3 is not expected in this context");
+
     let r = parser
         .run_inner(Args::from(&["-a"]))
         .unwrap_err()
@@ -197,10 +230,26 @@ fn parse_anywhere_catch_optional() {
         .unwrap_err()
         .unwrap_stderr();
     assert_eq!(r, "-a is not expected in this context");
+}
+
+#[test]
+fn anywhere_literal() {
+    let tag = any::<String>("-mode").guard(|x| x == "-mode", "not mode");
+    let mode = positional::<usize>("value");
+    let a = construct!(tag, mode).anywhere().catch().many();
+    let b = short('b').switch();
+    let parser = construct!(a, b).to_options();
 
     let r = parser
-        .run_inner(Args::from(&["3", "-a"]))
-        .unwrap_err()
-        .unwrap_stderr();
-    assert_eq!(r, "3 is not expected in this context");
+        .run_inner(Args::from(&["-b", "-mode", "12"]))
+        .unwrap();
+    assert_eq!(r, (vec![("-mode".to_owned(), 12)], true));
+
+    let r = parser
+        .run_inner(Args::from(&["-mode", "12", "-b"]))
+        .unwrap();
+    assert_eq!(r, (vec![("-mode".to_owned(), 12)], true));
+
+    let r = parser.run_inner(Args::from(&["-mode", "12"])).unwrap();
+    assert_eq!(r, (vec![("-mode".to_owned(), 12)], false));
 }
diff --git a/tests/meta_youmean.rs b/tests/meta_youmean.rs
index 9a7e117..a4e17bc 100644
--- a/tests/meta_youmean.rs
+++ b/tests/meta_youmean.rs
@@ -221,3 +221,73 @@ fn suggest_typo_fix() {
         .unwrap_stderr();
     assert_eq!(r, "--flag is not expected in this context");
 }
+
+#[test]
+fn better_error_message_with_typos() {
+    #[derive(Bpaf, Clone, Debug)]
+    #[bpaf(options)]
+    enum Commands {
+        /// Multi
+        ///  Line
+        ///  Comment
+        #[bpaf(command)]
+        Lines {},
+
+        #[bpaf(command)]
+        Arguments(#[bpaf(external(arguments))] Arguments),
+    }
+
+    #[derive(Bpaf, Clone, Debug)]
+    struct Arguments {
+        #[bpaf(short('e'), argument("Arg"))]
+        env: Vec<String>,
+
+        #[bpaf(positional("POS"))]
+        args: Vec<String>,
+    }
+
+    let r = arguments()
+        .to_options()
+        .run_inner(Args::from(&["-a", "erg"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "No such flag: `-a`, did you mean `-e`?");
+
+    let r = commands()
+        .run_inner(Args::from(&["arguments", "-a", "erg"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "No such flag: `-a`, did you mean `-e`?");
+
+    let r = arguments()
+        .to_options()
+        .run_inner(Args::from(&["--help"]))
+        .unwrap_err()
+        .unwrap_stdout();
+    let expected = "\
+Usage: [-e Arg]... [<POS>]...
+
+Available options:
+    -e <Arg>
+    -h, --help  Prints help information
+";
+    assert_eq!(r, expected);
+
+    let r = commands()
+        .run_inner(Args::from(&["--help"]))
+        .unwrap_err()
+        .unwrap_stdout();
+    let expected = "\
+Usage: COMMAND ...
+
+Available options:
+    -h, --help  Prints help information
+
+Available commands:
+    lines      Multi
+               Line
+               Comment
+    arguments
+";
+    assert_eq!(r, expected);
+}