Repository: pacak/bpaf
Pull request #114 (2022-10-27 13:26:18)

Release 0.7.2
- drop tainting logic, should be redundant
- improve error messages for guard and conflicting branches


Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 6f0c7e5..acbb2b9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "bpaf"
-version = "0.7.1"
+version = "0.7.2"
 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 e210c20..14cd5d5 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,9 @@
 # Change Log
 
+## bpaf [0.7.2] - 2022-10-27
+- drop tainting logic, should be redundant
+- improve error messages for guard and conflicting branches
+
 ## bpaf [0.7.1] - 2022-10-15
 - colors similar to cargo'some
    thanks to @kramer425
diff --git a/README.md b/README.md
index 5c78513..45fc898 100644
--- a/README.md
+++ b/README.md
@@ -372,21 +372,21 @@ Usage --user <ARG>
 	
 
 
- [__cargo_doc2readme_dependencies_info]: ggGkYW0AYXSEG52uRQSwBdezG6GWW8ODAbr5G6KRmT_WpUB5G9hPmBcUiIp6YXKEG-IPQtuM4VUFG47iETJwkSENG_GD7ukhn-KMG38J41C6yQn3YWSBgmRicGFmZTAuNy4x
- [__link0]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_derive_tutorial
- [__link1]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_combinatoric_tutorial
- [__link10]: https://docs.rs/bpaf/0.7.1/bpaf/?search=parsers::NamedArg::switch
- [__link11]: https://docs.rs/bpaf/0.7.1/bpaf/?search=parsers::NamedArg::argument
- [__link12]: https://docs.rs/bpaf/0.7.1/bpaf/?search=params::positional
- [__link13]: https://docs.rs/bpaf/0.7.1/bpaf/?search=parsers::NamedArg::argument
- [__link14]: https://docs.rs/bpaf/0.7.1/bpaf/?search=params::positional
- [__link15]: https://docs.rs/bpaf/0.7.1/bpaf/?search=bpaf::Parser::complete
- [__link17]: https://docs.rs/bpaf/0.7.1/bpaf/?search=info::OptionParser::run_inner
- [__link2]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_unusual
- [__link3]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_applicative
- [__link4]: https://docs.rs/bpaf/0.7.1/bpaf/?search=batteries
+ [__cargo_doc2readme_dependencies_info]: ggGkYW0AYXSEG52uRQSwBdezG6GWW8ODAbr5G6KRmT_WpUB5G9hPmBcUiIp6YXKEG-IPQtuM4VUFG47iETJwkSENG_GD7ukhn-KMG38J41C6yQn3YWSBgmRicGFmZTAuNy4y
+ [__link0]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_derive_tutorial
+ [__link1]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_combinatoric_tutorial
+ [__link10]: https://docs.rs/bpaf/0.7.2/bpaf/?search=parsers::NamedArg::switch
+ [__link11]: https://docs.rs/bpaf/0.7.2/bpaf/?search=parsers::NamedArg::argument
+ [__link12]: https://docs.rs/bpaf/0.7.2/bpaf/?search=params::positional
+ [__link13]: https://docs.rs/bpaf/0.7.2/bpaf/?search=parsers::NamedArg::argument
+ [__link14]: https://docs.rs/bpaf/0.7.2/bpaf/?search=params::positional
+ [__link15]: https://docs.rs/bpaf/0.7.2/bpaf/?search=bpaf::Parser::complete
+ [__link17]: https://docs.rs/bpaf/0.7.2/bpaf/?search=info::OptionParser::run_inner
+ [__link2]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_unusual
+ [__link3]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_applicative
+ [__link4]: https://docs.rs/bpaf/0.7.2/bpaf/?search=batteries
  [__link5]: https://github.com/pacak/bpaf/discussions/categories/q-a
- [__link6]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_derive_tutorial
- [__link7]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_combinatoric_tutorial
- [__link8]: https://docs.rs/bpaf/0.7.1/bpaf/?search=_applicative
- [__link9]: https://docs.rs/bpaf/0.7.1/bpaf/?search=bpaf::Parser::fallback
+ [__link6]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_derive_tutorial
+ [__link7]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_combinatoric_tutorial
+ [__link8]: https://docs.rs/bpaf/0.7.2/bpaf/?search=_applicative
+ [__link9]: https://docs.rs/bpaf/0.7.2/bpaf/?search=bpaf::Parser::fallback
diff --git a/bpaf_cauwugo/Cargo.toml b/bpaf_cauwugo/Cargo.toml
index cebf885..f6deea0 100644
--- a/bpaf_cauwugo/Cargo.toml
+++ b/bpaf_cauwugo/Cargo.toml
@@ -12,7 +12,7 @@ description = "A cargo frontend with dynamic completion"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-bpaf = { version = "0.7.1", path = "../", features = ["derive", "autocomplete"] }
+bpaf = { version = "0.7.2", path = "../", features = ["derive", "autocomplete"] }
 cargo_metadata = "0.15.0"
 once_cell = "1.13.1"
 
diff --git a/docs/src/guard/cases.txt b/docs/src/guard/cases.txt
index d0774c7..68bbae2 100644
--- a/docs/src/guard/cases.txt
+++ b/docs/src/guard/cases.txt
@@ -7,7 +7,7 @@ Options { number: 5 }
 ? But fails with the error message on higher values:
 > --number 11
 Stderr
-Couldn't parse "11": Values greater than 10 are only available in the DLC pack!
+"11": Values greater than 10 are only available in the DLC pack!
 
 
 ? But if function inside the parser fails - user will get the error back unless it's handled
diff --git a/docs/src/req_flag/cases.txt b/docs/src/req_flag/cases.txt
index 89b1e89..620026e 100644
--- a/docs/src/req_flag/cases.txt
+++ b/docs/src/req_flag/cases.txt
@@ -14,7 +14,7 @@ Options { decision: Undecided }
 ? `--on` and `--off` are mutually exclusive:
 > --on --off
 Stderr
---off is not expected in this context
+--off is not expected in this context: --off cannot be used at the same time as --on
 
 ? help
 > --help
diff --git a/src/args.rs b/src/args.rs
index e2cbc1d..a0e4a3e 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -6,12 +6,13 @@ use crate::{parsers::NamedArg, Error};
 /// Hides [`Args`] internal implementation
 mod inner {
     use std::{
+        collections::BTreeMap,
         ffi::{OsStr, OsString},
         ops::Range,
         rc::Rc,
     };
 
-    use crate::ParseFailure;
+    use crate::{Meta, ParseFailure};
 
     use super::{push_vec, Arg};
     /// All currently present command line parameters, use it for unit tests and manual parsing
@@ -47,22 +48,6 @@ mod inner {
         /// used to pick the parser that consumes the left most item
         pub(crate) head: usize,
 
-        /// setting it to true prevents suggester from replacing the results
-        ///
-        /// assume a parser consumes this:
-        /// ["asm"] -t <NUM>
-        /// and user passes ["asm", "-t", "x"] to it.
-        ///
-        /// problematic steps look something like this:
-        /// - bpaf parses "asm" expected
-        /// - then it consumes "-t x"
-        /// - then fails to parse "x"
-        /// - ParseWith rollbacks the arguments state - "asm" is back
-        /// - suggestion looks for something it can complain at and finds "asm"
-        ///
-        /// parse/guard failures should "taint" the arguments and turn off the suggestion logic
-        pub(crate) tainted: bool,
-
         /// don't try to suggest any more positional items after there's a positional item failure
         /// or parsing in progress
         #[cfg(feature = "autocomplete")]
@@ -73,6 +58,10 @@ mod inner {
 
         /// how many Ambiguities are there
         pub(crate) ambig: usize,
+
+        /// set of conflicts - usize contains the offset to the rejected item,
+        /// first Meta contains accepted item, second meta contains rejected item
+        pub(crate) conflicts: BTreeMap<usize, (Meta, Meta)>,
     }
 
     impl<const N: usize> From<&[&str; N]> for Args {
@@ -131,12 +120,12 @@ mod inner {
                 current: None,
                 head: usize::MAX,
                 depth: 0,
-                tainted: false,
                 #[cfg(feature = "autocomplete")]
                 comp: None,
                 #[cfg(feature = "autocomplete")]
                 no_pos_ahead: false,
                 ambig,
+                conflicts: BTreeMap::new(),
             }
         }
     }
@@ -438,7 +427,6 @@ impl Args {
     }
 
     pub(crate) fn word_parse_error(&mut self, error: &str) -> Error {
-        self.tainted = true;
         Error::Stderr(if let Some(os) = self.current_word() {
             format!("Couldn't parse {:?}: {}", os.to_string_lossy(), error)
         } else {
@@ -446,6 +434,14 @@ impl Args {
         })
     }
 
+    pub(crate) fn word_validate_error(&mut self, error: &str) -> Error {
+        Error::Stderr(if let Some(os) = self.current_word() {
+            format!("{:?}: {}", os.to_string_lossy(), error)
+        } else {
+            error.to_owned()
+        })
+    }
+
     /// Get a short or long flag: `-f` / `--flag`
     ///
     /// Returns false if value isn't present
diff --git a/src/docs/guard.md b/src/docs/guard.md
index 81099b9..889633f 100644
--- a/src/docs/guard.md
+++ b/src/docs/guard.md
@@ -53,7 +53,7 @@ Options { number: 5 }
 But fails with the error message on higher values:
 ```console
 % app --number 11
-Couldn't parse "11": Values greater than 10 are only available in the DLC pack!
+"11": Values greater than 10 are only available in the DLC pack!
 ```
 
 But if function inside the parser fails - user will get the error back unless it's handled
diff --git a/src/docs/req_flag.md b/src/docs/req_flag.md
index 190fb7e..d7ec655 100644
--- a/src/docs/req_flag.md
+++ b/src/docs/req_flag.md
@@ -80,7 +80,7 @@ Options { decision: Undecided }
 `--on` and `--off` are mutually exclusive:
 ```console
 % app --on --off
---off is not expected in this context
+--off is not expected in this context: --off cannot be used at the same time as --on
 ```
 
 help
diff --git a/src/info.rs b/src/info.rs
index 89f23a1..cc0dee1 100644
--- a/src/info.rs
+++ b/src/info.rs
@@ -75,12 +75,16 @@ enum ExtraParams {
 }
 
 fn check_unexpected(args: &Args) -> Result<(), Error> {
-    match args.peek() {
+    match args.items_iter().next() {
         None => Ok(()),
-        Some(item) => Err(Error::Stderr(format!(
-            "{} is not expected in this context",
-            item
-        ))),
+        Some((ix, item)) => {
+            let mut msg = format!("{} is not expected in this context", item);
+            if let Some((acc, rej)) = args.conflicts.get(&ix) {
+                use std::fmt::Write;
+                write!(msg, ": {} cannot be used at the same time as {}", rej, acc).unwrap();
+            }
+            Err(Error::Stderr(msg))
+        }
     }
 }
 
diff --git a/src/lib.rs b/src/lib.rs
index 641f473..26e82ec 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -774,12 +774,13 @@ use std::str::FromStr;
 ///     // `bpaf_derive` inserts implicit `argument`, `optional` and the right type
 ///     number_3: Option<u32>,
 ///
-///     // fails to compile: you need to specify `argument`
+///     // fails to compile: you need to specify a consumer, `argument` or `argument_os`
 ///     // #[bpaf(optional)]
-///     // number_4: Option<u32>,
+///     // number_4: Option<u32>
 ///
-///     #[bpaf(argument("N"), optional)]
-///     number_5: Option<u32>,
+///     // fails to compile: you also need to specify how to go from String to u32
+///     // #[bpaf(argument("N"), optional)]
+///     // number_5: Option<u32>,
 ///
 ///     // explicit consumer and a full postprocessing chain
 ///     #[bpaf(argument::<u32>("N"), optional)]
@@ -1169,7 +1170,8 @@ pub trait Parser<T> {
     ///
     /// Allows to generate autocompletion information for shell. Completer places generated input
     /// in place of metavar placeholders, so running `completer` on something that doesn't have a
-    /// [`positional`] or an [`argument`](NamedArg::argument) doesn't make much sense.
+    /// [`positional`] or an [`argument`](NamedArg::argument) (or their `_os` variants) doesn't make
+    /// much sense.
     ///
     /// Takes a function as a parameter that tries to complete partial input to a full one with
     /// optional description. `bpaf` would substitute current positional item or an argument an empty
diff --git a/src/meta_usage.rs b/src/meta_usage.rs
index f77f136..dbd26b2 100644
--- a/src/meta_usage.rs
+++ b/src/meta_usage.rs
@@ -58,7 +58,7 @@ fn collect_usage_meta(meta: &Meta, is_pos: &mut bool) -> Option<UsageMeta> {
                     assert!(!*is_pos || this_pos,
                         "bpaf usage BUG: all positional and command items must be placed in the right \
                         most position of the structure or tuple they are in but {} breaks this rule. \
-                        See bpaf documentation for `positional` for details.",
+                        See bpaf documentation for `positional` and `positional_os` for details.",
                         usage_meta
                     );
                     *is_pos |= this_pos;
diff --git a/src/meta_youmean.rs b/src/meta_youmean.rs
index 02ad5a5..afc41fe 100644
--- a/src/meta_youmean.rs
+++ b/src/meta_youmean.rs
@@ -20,10 +20,6 @@ pub(crate) fn should_suggest(err: &Error) -> bool {
 
 /// Looks for potential typos
 pub(crate) fn suggest(args: &Args, meta: &Meta) -> Result<(), Error> {
-    if args.tainted {
-        return Ok(());
-    }
-
     let arg = match args.peek() {
         Some(arg) => arg,
         None => return Ok(()),
diff --git a/src/structs.rs b/src/structs.rs
index 731015b..59b33f1 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -202,8 +202,16 @@ where
             #[cfg(feature = "autocomplete")]
             comp_items,
         )? {
+            if res_b.is_some() {
+                args.conflicts
+                    .insert(args_b.head, (self.this.meta(), self.that.meta()));
+            }
             Ok(res_a.unwrap())
         } else {
+            if res_a.is_some() {
+                args.conflicts
+                    .insert(args_a.head, (self.that.meta(), self.this.meta()));
+            }
             Ok(res_b.unwrap())
         }
     }
@@ -276,10 +284,7 @@ fn this_or_that_picks_first(
             std::mem::swap(args, args_b);
             Ok(false)
         }
-        (Some(e1), Some(e2)) => {
-            args.tainted |= args_a.tainted | args_b.tainted;
-            Err(e1.combine_with(e2))
-        }
+        (Some(e1), Some(e2)) => Err(e1.combine_with(e2)),
     };
 
     #[cfg(feature = "autocomplete")]
@@ -369,7 +374,7 @@ where
         if (self.check)(&t) {
             Ok(t)
         } else {
-            Err(args.word_parse_error(self.message))
+            Err(args.word_validate_error(self.message))
         }
     }
 


Test patch:
diff --git a/tests/errors.rs b/tests/errors.rs
new file mode 100644
index 0000000..1ef5476
--- /dev/null
+++ b/tests/errors.rs
@@ -0,0 +1,49 @@
+use bpaf::*;
+
+#[test]
+fn better_error_with_enum() {
+    #[derive(Debug, Clone, Bpaf)]
+    #[bpaf(options)]
+    enum Foo {
+        Alpha,
+        Beta,
+        Gamma,
+    }
+
+    let res = foo()
+        .run_inner(Args::from(&["--alpha", "--beta"]))
+        .unwrap_err()
+        .unwrap_stderr();
+
+    assert_eq!(
+        res,
+        "--beta is not expected in this context: --beta cannot be used at the same time as --alpha"
+    );
+
+    let res = foo()
+        .run_inner(Args::from(&["--alpha", "--gamma"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as (--alpha | --beta)");
+
+    let res = foo()
+        .run_inner(Args::from(&["--beta", "--gamma"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as (--alpha | --beta)");
+}
+
+#[test]
+fn guard_message() {
+    let parser = short('a')
+        .argument::<u32>("N")
+        .guard(|n| *n <= 10u32, "too high")
+        .to_options();
+
+    let res = parser
+        .run_inner(Args::from(&["-a", "30"]))
+        .unwrap_err()
+        .unwrap_stderr();
+
+    assert_eq!(res, "\"30\": too high");
+}
diff --git a/tests/tests.rs b/tests/tests.rs
index f5154f4..2f4f0a0 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -500,31 +500,46 @@ fn from_several_alternatives_pick_more_meaningful() {
         .run_inner(Args::from(&["-a", "-b"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err1, "-b is not expected in this context");
+    assert_eq!(
+        err1,
+        "-b is not expected in this context: -b cannot be used at the same time as -a"
+    );
 
     let err2 = parser
         .run_inner(Args::from(&["-b", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err2, "-a is not expected in this context");
+    assert_eq!(
+        err2,
+        "-a is not expected in this context: -a cannot be used at the same time as -b"
+    );
 
     let err3 = parser
         .run_inner(Args::from(&["-c", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err3, "-a is not expected in this context");
+    assert_eq!(
+        err3,
+        "-a is not expected in this context: (-a | -b) cannot be used at the same time as -c"
+    );
 
     let err4 = parser
         .run_inner(Args::from(&["-a", "-c"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err4, "-c is not expected in this context");
+    assert_eq!(
+        err4,
+        "-c is not expected in this context: -c cannot be used at the same time as (-a | -b)"
+    );
 
     let err5 = parser
         .run_inner(Args::from(&["-c", "-b", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err5, "-b is not expected in this context");
+    assert_eq!(
+        err5,
+        "-b is not expected in this context: (-a | -b) cannot be used at the same time as -c"
+    );
 }
 
 #[test]
@@ -1342,13 +1357,13 @@ fn did_you_mean_two_or_arguments() {
         .run_inner(Args::from(&["--parameter", "--flag"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(r, "--flag is not expected in this context");
+    assert_eq!(r, "--flag is not expected in this context: [--flag] cannot be used at the same time as [--parameter]");
 
     let r = parser
         .run_inner(Args::from(&["--flag", "--parameter"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(r, "--parameter is not expected in this context");
+    assert_eq!(r, "--parameter is not expected in this context: [--parameter] cannot be used at the same time as [--flag]");
 }
 
 // problematic steps look something like this:
@@ -1371,13 +1386,13 @@ fn cargo_show_asm_issue_guard() {
         .run_inner(Args::from(&["asm", "-t", "x"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "Couldn't parse \"x\": nope");
+    assert_eq!(res, "\"x\": nope");
 
     let res = parser
         .run_inner(Args::from(&["-t", "x"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "Couldn't parse \"x\": nope");
+    assert_eq!(res, "\"x\": nope");
 }
 
 #[test]
@@ -1483,13 +1498,13 @@ fn did_you_mean_inside_command() {
         .run_inner(Args::from(&["cmd", "--parameter", "--flag"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(r, "--flag is not expected in this context");
+    assert_eq!(r, "--flag is not expected in this context: [--flag] cannot be used at the same time as [--parameter]");
 
     let r = parser
         .run_inner(Args::from(&["cmd", "--flag", "--parameter"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(r, "--parameter is not expected in this context");
+    assert_eq!(r, "--parameter is not expected in this context: [--parameter] cannot be used at the same time as [--flag]");
 }
 
 #[test]