Repository: pacak/bpaf (rust)

Issue:
Bad error message for a command that expects a positional argument
Exapmple code:
```rust
use bpaf::Parser;

#[derive(Debug, Clone)]
enum Opts {
    Opt { arg: String },
    Noopt,
}

fn main() {
    let command = bpaf::positional("NAME")
        .to_options()
        .descr("descr for command")
        .command("command")
        .help("help for command")
        .map(|arg| Opts::Opt{ arg });

    let noopt = bpaf::pure(Opts::Noopt);
    let run = bpaf::construct!([noopt]);

    let opts = bpaf::construct!([command, run])
        .to_options()
        .descr("top level descr")
        .run();

    match opts {
        Opts::Opt {arg} => println!("Opt {}", arg),
        Opts::Noopt => println!("Noopt"),
    }
}
```

Example invokation:
```
> ./target/debug/bpaf-test 
Noopt
> ./target/debug/bpaf-test command foo
Opt foo
> ./target/debug/bpaf-test command
command is not expected in this context
>
```

"not expected in this context" is a confusing error message, since that command kind of is expected in this context. I think it should rather say something like "eof not expected in this context" or in the best case "expecting a positional argument NAME following 'command'"

Pull request:
Improve error handling UX
Better error messages when using commands and alternatives
Suggestions for typos in code

Fixes #30

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 74e7d74..e094b85 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "bpaf"
-version = "0.5.0"
+version = "0.5.1"
 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 49c7ff2..43eb731 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,10 @@
 # Change Log
 
+## bpaf [0.5.1] - 2022-08-22
+improve error messages if argument parsing fails:
+  - matcher no longer escapes inner command if it gets there
+  - detect and try to suggest for possible typos
+
 ## bpaf [0.5.0] - 2022-08-21
 A big rewrite, performance should stay mostly unchanged, binary overhead
 should be down by a third or so. Some minor cosmetic and correctness changes.
diff --git a/src/_derive_tutorial.rs b/src/_derive_tutorial.rs
index 0980e04..93e5dbe 100644
--- a/src/_derive_tutorial.rs
+++ b/src/_derive_tutorial.rs
@@ -60,7 +60,7 @@
 //!
 //!    - ### Generated function visibility: `private`
 //!
-//!      By default bpaf uses the same visibility as the datatype,
+//!      By default `bpaf` uses the same visibility as the datatype,
 //!      `private` makes it module private:
 //!
 //!      ```rust
diff --git a/src/args.rs b/src/args.rs
index 0370138..f928bfa 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -41,13 +41,17 @@ mod inner {
     #[derive(Clone, Debug)]
     pub struct Args {
         /// list of remaining arguments, for cheap cloning
-        items: Rc<[Arg]>,
+        pub(crate) items: Rc<[Arg]>,
         /// removed items, false - present, true - removed
         removed: Vec<bool>,
         remaining: usize,
 
+        #[doc(hidden)]
         /// Used to render an error message for [`parse`][crate::Parser::parse]
-        pub(crate) current: Option<usize>,
+        pub current: Option<usize>,
+        #[doc(hidden)]
+        /// "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,
@@ -100,6 +104,7 @@ mod inner {
                 items: Rc::from(vec),
                 current: None,
                 head: usize::MAX,
+                depth: 0,
             }
         }
     }
diff --git a/src/info.rs b/src/info.rs
index ce4d12f..f4b80a6 100644
--- a/src/info.rs
+++ b/src/info.rs
@@ -212,7 +212,11 @@ impl<T> OptionParser<T> {
                 if let Err(err) = check_unexpected(&reg_args) {
                     err
                 } else {
-                    *args = reg_args;
+                    std::mem::swap(args, &mut reg_args);
+                    // <-
+                    // complain about any unused items - beats "not expected in this context"
+                    crate::meta_youmean::suggest(args, &self.inner.meta())?;
+                    // <-
                     return Ok(r);
                 }
             }
@@ -242,9 +246,14 @@ impl<T> OptionParser<T> {
             }
             Err(_) => {}
         }
-        Err(err)
 
-        //self.inner.run(args)
+        // <-
+        if crate::meta_youmean::should_suggest(&err) {
+            crate::meta_youmean::suggest(args, &self.inner.meta())?;
+        }
+        // <-
+
+        Err(err)
     }
     /// Get first line of description if Available
     ///
@@ -257,7 +266,7 @@ impl<T> OptionParser<T> {
 
     /// Set the version field.
     ///
-    /// By default bpaf won't include any version info and won't accept `--version` switch.
+    /// By default `bpaf` won't include any version info and won't accept `--version` switch.
     ///
     /// # Combinatoric usage
     ///
diff --git a/src/lib.rs b/src/lib.rs
index 9df2c01..0e0eb50 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -269,6 +269,7 @@ mod item;
 mod meta;
 mod meta_help;
 mod meta_usage;
+mod meta_youmean;
 mod params;
 mod structs;
 
@@ -467,6 +468,7 @@ macro_rules! construct {
         let meta = $crate::Meta::And(vec![ $($fields.meta()),* ]);
         let inner = move |args: &mut $crate::Args| {
             $(let $fields = $fields.eval(args)?;)*
+            args.current = None;
             ::std::result::Result::Ok::<_, $crate::Error>
                 ($crate::construct!(@make $ty [$($fields)*]))
         };
@@ -486,7 +488,7 @@ macro_rules! construct {
 /// inside that are composable with other `Parser` containers using [`construct!`] and the only
 /// way to extract this value is by transforming it to [`OptionParser`] with
 /// [`to_options`](Parser::to_options) and running it with [`run`](OptionParser::run). At which
-/// point you either get your value out or bpaf would generate a message describing a problem
+/// point you either get your value out or `bpaf` would generate a message describing a problem
 /// (missing argument, validation failure, user requested help, etc) and the program would
 /// exit.
 ///
@@ -600,7 +602,7 @@ pub trait Parser<T> {
     /// ```
     ///
     /// # Derive usage:
-    /// Bpaf would insert implicit `many` when resulting type is a vector
+    /// `bpaf` would insert implicit `many` when resulting type is a vector
     /// ```rust
     /// # use bpaf::*;
     /// #[derive(Debug, Clone, Bpaf)]
@@ -712,7 +714,7 @@ pub trait Parser<T> {
     ///
     /// # Derive usage
     ///
-    /// By default bpaf would automatically use optional for fields of type `Option<T>`,
+    /// By default `bpaf_derive` would automatically use optional for fields of type `Option<T>`,
     /// for as long as it's not prevented from doing so by present postprocessing options
     /// ```rust
     /// # use bpaf::*;
@@ -1105,7 +1107,7 @@ pub trait Parser<T> {
     // {{{ or_else
     /// If first parser fails - try the second one
     ///
-    /// For parser to succeed eiter of the components needs to succeed. If both succeed - bpaf
+    /// For parser to succeed eiter of the components needs to succeed. If both succeed - `bpaf`
     /// would use output from one that consumed the left most value. The second flag on the command
     /// line remains unconsumed by `or_else`.
     ///
diff --git a/src/meta_help.rs b/src/meta_help.rs
index 870be37..d2125ee 100644
--- a/src/meta_help.rs
+++ b/src/meta_help.rs
@@ -3,7 +3,7 @@
 use crate::{info::Info, item::ShortLong, Meta};
 
 #[derive(Debug)]
-enum HelpItem<'a> {
+pub(crate) enum HelpItem<'a> {
     Decor {
         help: &'a str,
     },
@@ -30,10 +30,10 @@ enum HelpItem<'a> {
 }
 
 #[derive(Default, Debug)]
-struct HelpItems<'a> {
+pub(crate) struct HelpItems<'a> {
     cmds: Vec<HelpItem<'a>>,
-    psns: Vec<HelpItem<'a>>,
-    flgs: Vec<HelpItem<'a>>,
+    pub(crate) psns: Vec<HelpItem<'a>>,
+    pub(crate) flgs: Vec<HelpItem<'a>>,
 }
 
 impl HelpItem<'_> {
@@ -46,7 +46,7 @@ impl HelpItem<'_> {
 }
 
 impl<'a> HelpItems<'a> {
-    fn classify(&mut self, meta: &'a Meta) {
+    pub(crate) fn classify(&mut self, meta: &'a Meta) {
         match meta {
             Meta::And(xs) | Meta::Or(xs) => {
                 for x in xs {
@@ -116,7 +116,7 @@ impl<'a> HelpItems<'a> {
 }
 
 #[derive(Debug)]
-struct ShortLongHelp(ShortLong);
+pub(crate) struct ShortLongHelp(ShortLong);
 
 impl ShortLongHelp {
     #[inline]
diff --git a/src/meta_usage.rs b/src/meta_usage.rs
index d5444a7..b5a39c9 100644
--- a/src/meta_usage.rs
+++ b/src/meta_usage.rs
@@ -21,9 +21,9 @@ pub(crate) enum UsageMeta {
 /// Transforms `Meta` to [`UsageMeta`]
 ///
 /// parameter `required` defines the value's context: optional or required.
-/// bpaf shows Optional values in required context in []
+/// `bpaf` shows Optional values in required context in []
 ///
-/// bpaf uses parameter `had_commands` for command deduplication, initialize it with false
+/// `bpaf` uses parameter `had_commands` for command deduplication, initialize it with false
 pub(crate) fn collect_usage_meta(
     meta: &Meta,
     required: bool,
diff --git a/src/meta_youmean.rs b/src/meta_youmean.rs
new file mode 100644
index 0000000..d12105c
--- /dev/null
+++ b/src/meta_youmean.rs
@@ -0,0 +1,166 @@
+use crate::{
+    args::Arg,
+    item::{Item, ShortLong},
+    Args, Error, Meta,
+};
+
+pub(crate) fn should_suggest(err: &Error) -> bool {
+    match err {
+        Error::Stdout(_) => false,
+        Error::Stderr(_) => true,
+        Error::Missing(xs) => {
+            let mut hi = crate::meta_help::HelpItems::default();
+            for x in xs.iter() {
+                hi.classify(x);
+            }
+            hi.flgs.is_empty() && hi.psns.is_empty()
+        }
+    }
+}
+
+/// Looks for potential typos
+pub(crate) fn suggest(args: &Args, meta: &Meta) -> Result<(), Error> {
+    let arg = match args.peek() {
+        Some(arg) => arg,
+        None => return Ok(()),
+    };
+    if args.items.iter().filter(|&a| a == arg).count() > 1 {
+        // We have more than one copy of unexpected item. Either user specified
+        // several of those or parser accepts only limited number of them. Or
+        // they are accepted in a different branch. Give up and produce a default
+        // "not expected in this context" error
+        return Ok(());
+    }
+
+    let mut variants = Vec::new();
+    inner(arg, meta, &mut variants);
+
+    variants.sort_by(|a, b| b.0.cmp(&a.0));
+
+    if let Some((l, best)) = variants.pop() {
+        if l > 0 {
+            return Err(Error::Stderr(best));
+        }
+    }
+    Ok(())
+}
+
+#[derive(Copy, Clone)]
+enum I<'a> {
+    ShortFlag(char),
+    LongFlag(&'a str),
+    ShortCmd(char),
+    LongCmd(&'a str),
+}
+
+// human readable
+impl std::fmt::Debug for I<'_> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::ShortFlag(s) => write!(f, "flag: `-{}`", s),
+            Self::LongFlag(s) => write!(f, "flag: `--{}`", s),
+            Self::ShortCmd(s) => write!(f, "command alias: `{}`", s),
+            Self::LongCmd(s) => write!(f, "command: `{}`", s),
+        }
+    }
+}
+
+// used for levenshtein distance calculation
+impl std::fmt::Display for I<'_> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use std::fmt::Write;
+        match self {
+            Self::ShortFlag(s) => write!(f, "-{}", s),
+            Self::LongFlag(s) => write!(f, "--{}", s),
+            Self::ShortCmd(s) => f.write_char(*s),
+            Self::LongCmd(s) => f.write_str(s),
+        }
+    }
+}
+
+fn ins(expected: I, actual: I, variants: &mut Vec<(usize, String)>) {
+    variants.push((
+        levenshtein(&expected.to_string(), &actual.to_string()),
+        format!("No such {:?}, did you mean `{}`?", actual, expected),
+    ));
+}
+
+fn inner_item(arg: &Arg, item: &Item, variants: &mut Vec<(usize, String)>) {
+    let actual: I = match arg {
+        Arg::Short(s) => I::ShortFlag(*s),
+        Arg::Long(s) => I::LongFlag(s.as_str()),
+        Arg::Word(w) => match &w.utf8 {
+            Some(s) => I::LongCmd(s.as_str()),
+            None => return,
+        },
+    };
+    match item {
+        Item::Positional { .. } => {}
+        Item::Command { name, short, .. } => {
+            ins(I::LongCmd(name), actual, variants);
+            if let Some(s) = short {
+                ins(I::ShortCmd(*s), actual, variants);
+            }
+        }
+        Item::Flag { name, .. } | Item::Argument { name, .. } => match name {
+            ShortLong::Short(s) => ins(I::ShortFlag(*s), actual, variants),
+            ShortLong::Long(l) => ins(I::LongFlag(l), actual, variants),
+            ShortLong::ShortLong(s, l) => {
+                ins(I::ShortFlag(*s), actual, variants);
+                ins(I::LongFlag(l), actual, variants);
+            }
+        },
+    }
+}
+
+fn inner(arg: &Arg, meta: &Meta, variants: &mut Vec<(usize, String)>) {
+    match meta {
+        Meta::And(xs) | Meta::Or(xs) => {
+            for x in xs {
+                inner(arg, x, variants);
+            }
+        }
+        Meta::Item(item) => inner_item(arg, item, variants),
+        Meta::Optional(meta) | Meta::Many(meta) | Meta::Decorated(meta, _) => {
+            inner(arg, meta, variants);
+        }
+        Meta::Skip => {}
+    }
+}
+
+fn levenshtein(a: &str, b: &str) -> usize {
+    let mut result = 0;
+    let mut cache = a.chars().enumerate().map(|i| i.0 + 1).collect::<Vec<_>>();
+    let mut distance_a;
+    let mut distance_b;
+
+    for (index_b, code_b) in b.chars().enumerate() {
+        result = index_b;
+        distance_a = index_b;
+
+        for (index_a, code_a) in a.chars().enumerate() {
+            distance_b = if code_a == code_b {
+                distance_a
+            } else {
+                distance_a + 1
+            };
+
+            distance_a = cache[index_a];
+
+            result = if distance_a > result {
+                if distance_b > result {
+                    result + 1
+                } else {
+                    distance_b
+                }
+            } else if distance_b > distance_a {
+                distance_a + 1
+            } else {
+                distance_b
+            };
+
+            cache[index_a] = result;
+        }
+    }
+    result
+}
diff --git a/src/params.rs b/src/params.rs
index d70816d..fcb39ed 100644
--- a/src/params.rs
+++ b/src/params.rs
@@ -67,7 +67,7 @@ use crate::{
 /// Create it with [`short`] or [`long`].
 ///
 /// # Ways to consume data
-/// bpaf supports several different ways user can specify values on a command line:
+/// `bpaf` supports several different ways user can specify values on a command line:
 ///
 /// - [`flag`](Named::flag) - a string that consists of two dashes (`--flag`) and a name and a single
 /// dash and a single character (`-f`) - [`long`](Named::long) and [`short`](Named::short) name respectively.
@@ -115,7 +115,7 @@ use crate::{
 /// // fails since --out-dir isn't a valid argument for command "check"
 /// ```
 ///
-/// As most of the other parsers bpaf treats anything to the right of `--` symbol as positional
+/// As most of the other parsers `bpaf` treats anything to the right of `--` symbol as positional
 /// arguments regardless their names:
 ///
 /// ```console
@@ -827,7 +827,7 @@ impl<P> Command<P> {
     ///
     /// `bpaf` uses this description along with the command name
     /// in help output so it shouldn't exceed one or two lines. If `help` isn't specified
-    /// bpaf falls back to [`descr`](OptionParser::descr) from the inner parser.
+    /// `bpaf` falls back to [`descr`](OptionParser::descr) from the inner parser.
     ///
     /// # Combinatoric usage
     ///
@@ -848,7 +848,7 @@ impl<P> Command<P> {
     /// ```
     ///
     /// # Derive usage
-    /// bpaf uses doc comments for inner parser, no specific options are available.
+    /// `bpaf_derive` uses doc comments for inner parser, no specific options are available.
     /// See [`descr`](OptionParser::descr) for more details
     /// ```rust
     /// # use bpaf::*;
@@ -909,6 +909,8 @@ impl<T> Parser<T> for Command<T> {
             })
         {
             args.head = usize::MAX;
+            args.depth += 1;
+            // failure past this point should be preserved in or_else parser
             self.subparser.run_subparser(args)
         } else {
             Err(Error::Missing(vec![self.meta()]))
diff --git a/src/structs.rs b/src/structs.rs
index d3834c7..2ebf62d 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -123,16 +123,27 @@ where
     fn eval(&self, args: &mut Args) -> Result<T, Error> {
         let mut args_a = args.clone();
         let mut args_b = args.clone();
-        match (self.this.eval(&mut args_a), self.that.eval(&mut args_b)) {
-            // side channel (--help) reporting takes priority
-            (e @ Err(Error::Stdout(_)), _) | (_, e @ Err(Error::Stdout(_))) => e,
+        let res_a = self.this.eval(&mut args_a);
+        let res_b = self.that.eval(&mut args_b);
 
+        match Ord::cmp(&args_a.depth, &args_b.depth) {
+            std::cmp::Ordering::Less => {
+                std::mem::swap(args, &mut args_b);
+                return res_b;
+            }
+            std::cmp::Ordering::Equal => {}
+            std::cmp::Ordering::Greater => {
+                std::mem::swap(args, &mut args_a);
+                return res_a;
+            }
+        }
+        match (res_a, res_b) {
             (Ok(a), Ok(b)) => {
                 if args_a.head <= args_b.head {
-                    *args = args_a;
+                    std::mem::swap(args, &mut args_a);
                     Ok(a)
                 } else {
-                    *args = args_b;
+                    std::mem::swap(args, &mut args_b);
                     Ok(b)
                 }
             }


Test patch:
diff --git a/src/tests.rs b/src/tests.rs
index 5afd9f2..5462170 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -1303,7 +1303,7 @@ fn many_error_handling() {
 }
 
 #[test]
-fn failure_is_not_stupid() {
+fn failure_is_not_stupid_1() {
     let a = short('a').argument("A").from_str::<u32>();
     let b = pure(()).parse::<_, _, String>(|_| Err("nope".to_string()));
     let parser = construct!(a, b).to_options();
@@ -1314,3 +1314,112 @@ fn failure_is_not_stupid() {
         .unwrap_stderr();
     assert_eq!(res, "Couldn't parse: nope");
 }
+
+#[test]
+fn failure_is_not_stupid_2() {
+    let a = short('a').argument("A").from_str::<u32>();
+    let b = short('b').argument("B").from_str::<u32>();
+    let parser = construct!(a, b)
+        .parse::<_, (), String>(|_| Err("nope".to_string()))
+        .to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["-a", "42", "-b", "42"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "Couldn't parse: nope");
+}
+
+#[test]
+fn no_fallback_out_of_command_parser() {
+    let alt1 = positional("NAME").to_options().command("cmd");
+    let alt2 = pure(String::new());
+    let parser = construct!([alt1, alt2]).to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["cmd"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "Expected <NAME>, pass --help for usage information");
+
+    let res = parser.run_inner(Args::from(&["cmd", "a"])).unwrap();
+    assert_eq!(res, "a");
+
+    let res = parser.run_inner(Args::from(&[])).unwrap();
+    assert_eq!(res, "");
+}
+
+#[test]
+fn did_you_mean_switch() {
+    let a = short('f').long("flag").switch();
+    let b = short('p').long("plag").switch();
+    let parser = construct!([a, b]).to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["--fla"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such flag: `--fla`, did you mean `--flag`?");
+
+    let res = parser
+        .run_inner(Args::from(&["flag"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such command: `flag`, did you mean `--flag`?");
+
+    let res = parser
+        .run_inner(Args::from(&["--pla"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such flag: `--pla`, did you mean `--plag`?");
+
+    let res = parser
+        .run_inner(Args::from(&["--p"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such flag: `--p`, did you mean `-p`?");
+}
+
+#[test]
+fn did_you_mean_req_flag() {
+    let parser = long("flag").req_flag(()).to_options();
+    let res = parser
+        .run_inner(Args::from(&["--fla"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "Expected --flag, pass --help for usage information");
+}
+
+#[test]
+fn did_you_mean_argument() {
+    let parser = long("flag").argument("VAL").to_options();
+    let res = parser
+        .run_inner(Args::from(&["--fla"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(
+        res,
+        "Expected --flag VAL, pass --help for usage information"
+    );
+}
+
+#[test]
+fn did_you_mean_command() {
+    let parser = pure(())
+        .to_options()
+        .command("command")
+        .short('c')
+        .to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["comman"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such command: `comman`, did you mean `command`?");
+
+    let res = parser
+        .run_inner(Args::from(&["--comman"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "No such flag: `--comman`, did you mean `command`?");
+}