Repository: pacak/bpaf
Pull request #209 (2023-05-14 13:03:04)

Error handing refactor
- Pass around error messages as ATDs instead of strings
- track conflicts by indices rather than parser metadata
- move most of the error rendering in one place
- a few clippy suggestions

Not really exciting by itself but makes further error message improvements easier. This also reduces executable size in a bloat test 152Kb -> 132Kb


Patch:
diff --git a/Changelog.md b/Changelog.md
index dec1bd1..457d2f6 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,9 @@
 # Change Log
 
+## bpaf [0.8.1] - Unreleased
+- more errors are now passed as ADTs rather than plain strings
+- conflicts are now tracked with indices rather than parser meta
+
 ## bpaf [0.8.0], bpaf_derive [0.4.0] - 2023-05-10
 
 ### Breaking changes
diff --git a/bpaf_derive/src/top.rs b/bpaf_derive/src/top.rs
index baf7535..a57cc85 100644
--- a/bpaf_derive/src/top.rs
+++ b/bpaf_derive/src/top.rs
@@ -610,7 +610,7 @@ impl ToTokens for BParser {
                 adjacent,
                 boxed,
             } => {
-                if let Fields::NoFields = fields {
+                if matches!(fields, Fields::NoFields) {
                     quote!(::bpaf::pure(#name)).to_tokens(tokens);
                 } else {
                     let parse_decls = fields.parser_decls();
diff --git a/docs/src/req_flag/cases.txt b/docs/src/req_flag/cases.txt
index 75e5cab..f85dcfe 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 cannot be used at the same time as --on
+"--off" cannot be used at the same time as "--on"
 
 ? help
 > --help
diff --git a/src/args.rs b/src/args.rs
index f0f778d..32fee2e 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -2,24 +2,36 @@ use std::ffi::OsString;
 
 pub(crate) use crate::arg::*;
 use crate::{
-    error::MissingItem, info::Info, item::Item, meta_help::Metavar, parsers::NamedArg, Error, Meta,
-    ParseFailure,
+    error::{Message, MissingItem},
+    info::Info,
+    item::Item,
+    meta_help::Metavar,
+    parsers::NamedArg,
+    Error, Meta, ParseFailure,
 };
 
 /// Shows which branch of [`ParseOrElse`] parsed the argument
-#[derive(Debug, Clone)]
-pub(crate) enum Conflict {
-    /// Only one branch succeeded
-    Solo(Meta),
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+pub(crate) enum ItemState {
+    /// Value is yet to be parsed
+    Unparsed,
     /// Both branches succeeded, first parser was taken in favor of the second one
-    Conflicts(Meta, Meta),
+    Conflict(usize),
+    /// Value was parsed
+    Parsed,
 }
 
-impl Conflict {
-    pub(crate) fn winner(&self) -> &Meta {
+impl ItemState {
+    pub(crate) fn parsed(&self) -> bool {
         match self {
-            Conflict::Solo(s) => s,
-            Conflict::Conflicts(w, _) => w,
+            ItemState::Unparsed | ItemState::Conflict(_) => false,
+            ItemState::Parsed => true,
+        }
+    }
+    pub(crate) fn present(&self) -> bool {
+        match self {
+            ItemState::Unparsed | ItemState::Conflict(_) => true,
+            ItemState::Parsed => false,
         }
     }
 }
@@ -38,7 +50,6 @@ impl std::fmt::Debug for Improve {
 /// Hides [`Args`] internal implementation
 mod inner {
     use std::{
-        collections::BTreeMap,
         ffi::{OsStr, OsString},
         ops::Range,
         rc::Rc,
@@ -46,7 +57,7 @@ mod inner {
 
     use crate::ParseFailure;
 
-    use super::{push_vec, Arg, Conflict};
+    use super::{push_vec, Arg, ItemState};
     /// All currently present command line parameters, use it for unit tests and manual parsing
     ///
     /// The easiest way to create `Args` is by using it's `From` instance.
@@ -62,11 +73,12 @@ mod inner {
     /// ```
     #[derive(Clone, Debug)]
     pub struct Args {
-        /// list of remaining arguments, for cheap cloning
+        /// list of all available command line arguments, for cheap cloning
         pub(crate) items: Rc<[Arg]>,
-        /// removed items, false - present, true - removed
-        removed: Vec<bool>,
-        /// performance optimization mostly,
+
+        item_state: Vec<ItemState>,
+
+        /// performance optimization mostly - tracks removed item and gives cheap is_empty
         remaining: usize,
 
         #[doc(hidden)]
@@ -92,10 +104,6 @@ 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, Conflict>,
-
         /// A way to customize behavior for --help and error handling
         pub(crate) improve_error: super::Improve,
 
@@ -154,9 +162,13 @@ mod inner {
     }
 
     impl Args {
+        pub(crate) fn present(&self, ix: usize) -> Option<bool> {
+            Some(self.item_state.get(ix)?.present())
+        }
+
         pub(crate) fn args_from(vec: Vec<Arg>, ambig: usize) -> Self {
             Args {
-                removed: vec![false; vec.len()],
+                item_state: vec![ItemState::Unparsed; vec.len()],
                 remaining: vec.len(),
                 scope: 0..vec.len(),
                 items: Rc::from(vec),
@@ -168,7 +180,6 @@ mod inner {
                 #[cfg(feature = "autocomplete")]
                 no_pos_ahead: false,
                 ambig,
-                conflicts: BTreeMap::new(),
                 improve_error: super::Improve(crate::help::improve_error),
             }
         }
@@ -227,11 +238,28 @@ mod inner {
         }
 
         pub(crate) fn remove(&mut self, index: usize) {
-            if self.scope.contains(&index) && !self.removed[index] {
+            if self.scope.contains(&index) && self.item_state[index].present() {
                 self.current = Some(index);
                 self.remaining -= 1;
                 self.head = self.head.min(index);
-                self.removed[index] = true;
+                self.item_state[index] = ItemState::Parsed;
+            }
+        }
+
+        pub(crate) fn conflict(self: &Args) -> Option<(usize, usize)> {
+            let (ix, _item) = self.items_iter().next()?;
+            if let ItemState::Conflict(other) = self.item_state.get(ix)? {
+                Some((ix, *other))
+            } else {
+                None
+            }
+        }
+
+        pub(crate) fn save_conflicts(&mut self, loser: &Args) {
+            for (winner, loser) in self.item_state.iter_mut().zip(loser.item_state.iter()) {
+                if winner.present() && loser.parsed() {
+                    *winner = ItemState::Conflict(self.head);
+                }
             }
         }
 
@@ -243,23 +271,12 @@ mod inner {
             self.remaining
         }
 
-        pub(crate) fn current_word(&self) -> Option<&OsStr> {
-            let ix = self.current?;
-            match &self.items[ix] {
-                Arg::Word(w) | Arg::PosWord(w) => Some(w),
-                Arg::Short(..) | Arg::Long(..) | Arg::Ambiguity(..) => None,
-            }
-        }
-
+        /// Get an argument from a scope that was not consumed yet
         pub(crate) fn get(&self, ix: usize) -> Option<&Arg> {
-            if !self.scope.contains(&ix) {
-                return None;
-            }
-            let arg = self.items.get(ix)?;
-            if *self.removed.get(ix)? {
-                None
+            if self.scope.contains(&ix) && self.item_state.get(ix)?.present() {
+                Some(self.items.get(ix)?)
             } else {
-                Some(arg)
+                None
             }
         }
 
@@ -283,10 +300,10 @@ mod inner {
             // last item on a command line is "--" it might be both double dash indicator
             // "the rest are strictly positionals" but it can also be part of the long name
             // restore it so completion logic is more internally consistent
-            if !self.removed.is_empty() {
-                let o = self.removed.len() - 1;
-                if self.removed[o] {
-                    self.removed[o] = false;
+            if !self.item_state.is_empty() {
+                let o = self.item_state.len() - 1;
+                if self.item_state[o].parsed() {
+                    self.item_state[o] = ItemState::Unparsed;
                     self.remaining += 1;
                     let mut items = self.items.to_vec();
 
@@ -311,21 +328,21 @@ mod inner {
 
             // starting at the beginning of the scope look for the first mismatch
             let start = self.scope().start;
-            for (mut offset, (this, orig)) in self.removed[start..]
+            for (mut offset, (this, orig)) in self.item_state[start..]
                 .iter()
-                .zip(original.removed[start..].iter())
+                .zip(original.item_state[start..].iter())
                 .enumerate()
             {
                 offset += start;
                 // once there's a mismatch we have the scope we are looking for:
                 // all the adjacent items consumed in this. It doesn't make sense to remove it if
                 // it matches the original scope though...
-                if !this && !orig {
+                if this.present() && orig.present() {
                     let proposed_scope = start..offset;
-                    return if self.scope() != proposed_scope {
-                        Some(proposed_scope)
-                    } else {
+                    return if self.scope() == proposed_scope {
                         None
+                    } else {
+                        Some(proposed_scope)
                     };
                 }
             }
@@ -337,8 +354,8 @@ mod inner {
             if self.items.is_empty() {
                 return None;
             }
-            for end in start..self.removed.len() {
-                if self.removed.get(end) != Some(&false) {
+            for end in start..self.item_state.len() {
+                if self.item_state.get(end).map_or(true, ItemState::parsed) {
                     return Some(start..end);
                 }
             }
@@ -356,11 +373,11 @@ mod inner {
         /// Mark everything outside of `range` as removed
         pub(crate) fn set_scope(&mut self, scope: Range<usize>) {
             self.scope = scope;
-            self.sync_remaining();
-        }
-
-        fn sync_remaining(&mut self) {
-            self.remaining = self.removed[self.scope()].iter().filter(|x| !**x).count();
+            self.remaining = self.item_state[self.scope()]
+                .iter()
+                .copied()
+                .filter(ItemState::present)
+                .count();
         }
 
         #[inline(never)]
@@ -413,11 +430,11 @@ mod inner {
                             let os = std::mem::take(os);
 
                             new_items.remove(pos);
-                            self.removed.remove(pos);
+                            self.item_state.remove(pos);
                             for short in items.iter().rev() {
                                 // inefficient but ambiguities shouldn't supposed to happen
                                 new_items.insert(pos, Arg::Short(*short, false, OsString::new()));
-                                self.removed.insert(pos, false);
+                                self.item_state.insert(pos, ItemState::Unparsed);
                             }
                             // items[0] is written twice - in a loop above without `os` and right here,
                             // with `os` present
@@ -432,7 +449,7 @@ mod inner {
                             ));
                             new_items[pos] = Arg::Short(items[0], true, std::mem::take(os));
                             new_items.insert(pos + 1, word);
-                            self.removed.insert(pos, false);
+                            self.item_state.insert(pos, ItemState::Unparsed);
 
                             self.remaining += 1; // removed Ambiguity, added short and word
                         }
@@ -465,7 +482,8 @@ mod inner {
         ///   ([-a] alpha) | beta
         /// and user passes "-a <TAB>" we should not suggest "beta"
         pub(crate) fn valid_complete_head(&self) -> bool {
-            self.is_empty() || (self.len() == 1 && self.removed.last() == Some(&false))
+            self.is_empty()
+                || (self.len() == 1 && self.item_state.last().expect("we just confirmed").present())
         }
 
         #[cfg(feature = "autocomplete")]
@@ -499,7 +517,7 @@ mod inner {
                 }
                 self.cur += 1;
 
-                if *self.args.removed.get(cur)? {
+                if !self.args.present(cur)? {
                     continue;
                 }
 
@@ -520,7 +538,7 @@ mod inner {
                     return None;
                 }
                 self.cur += 1;
-                if !*self.args.removed.get(ix)? {
+                if self.args.item_state.get(ix)?.present() {
                     return Some((ix, &self.args.items[ix]));
                 }
             }
@@ -538,28 +556,6 @@ impl Args {
         }
     }
 
-    pub(crate) fn word_parse_error(&mut self, error: &str) -> Error {
-        Error::Message(
-            if let Some(os) = self.current_word() {
-                format!("Couldn't parse {:?}: {}", os.to_string_lossy(), error)
-            } else {
-                format!("Couldn't parse: {}", error)
-            },
-            false,
-        )
-    }
-
-    pub(crate) fn word_validate_error(&mut self, error: &str) -> Error {
-        Error::Message(
-            if let Some(os) = self.current_word() {
-                format!("{:?}: {}", os.to_string_lossy(), error)
-            } else {
-                error.to_owned()
-            },
-            false,
-        )
-    }
-
     /// Get a short or long flag: `-f` / `--flag`
     ///
     /// Returns false if value isn't present
@@ -584,7 +580,7 @@ impl Args {
         named: &NamedArg,
         adjacent: bool,
     ) -> Result<Option<OsString>, Error> {
-        let (key_ix, arg) = match self
+        let (key_ix, _arg) = match self
             .items_iter()
             .find(|arg| named.matches_arg(arg.1, adjacent))
         {
@@ -595,27 +591,7 @@ impl Args {
         let val_ix = key_ix + 1;
         let val = match self.get(val_ix) {
             Some(Arg::Word(w)) => w,
-            Some(Arg::Short(_, _, os) | Arg::Long(_, _, os)) => {
-                let msg = if let (Arg::Short(s, _, fos), true) = (&arg, os.is_empty()) {
-                    let fos = fos.to_string_lossy();
-                    let repl = fos.strip_prefix('-').unwrap().strip_prefix(*s).unwrap();
-                    format!(
-                        "`{}` is not accepted, try using it as `-{}={}`",
-                        fos, s, repl
-                    )
-                } else {
-                    let os = os.to_string_lossy();
-                    format!( "`{}` requires an argument, got a flag-like `{}`, try `{}={}` to use it as an argument", arg, os, arg,os)
-                };
-                return Err(Error::Message(msg, false));
-            }
-            None | Some(Arg::PosWord(_) | Arg::Ambiguity(..)) => {
-                // TODO - this is missing!
-                return Err(Error::Message(
-                    format!("{} requires an argument", arg),
-                    false,
-                ));
-            }
+            _ => return Err(Error::Message(Message::NoArgument(key_ix))),
         };
         let val = val.clone();
         self.current = Some(val_ix);
@@ -632,6 +608,7 @@ impl Args {
         &mut self,
         metavar: Metavar,
     ) -> Result<Option<(bool, OsString)>, Error> {
+        #[allow(clippy::range_plus_one)] // gives wrong type otherwise
         match self.items_iter().next() {
             Some((ix, Arg::PosWord(w))) => {
                 let w = w.clone();
diff --git a/src/docs/req_flag.md b/src/docs/req_flag.md
index a128494..f1bd489 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 cannot be used at the same time as --on
+"--off" cannot be used at the same time as "--on"
 ```
 
 help
diff --git a/src/error.rs b/src/error.rs
index 66e6fed..5101974 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -1,14 +1,13 @@
 use std::ops::Range;
 
-use crate::item::Item;
+use crate::{item::Item, meta_help::Metavar};
 
 /// Unsuccessful command line parsing outcome, internal representation
 #[derive(Debug)]
 pub enum Error {
     /// Parsing failed, it is still possible to improve the error message
-    /// bool shows if this error can be caught with fallback, messages with it set to true are
-    /// generated by .some(msg) modifier and allow to use it with .fallback
-    Message(String, bool),
+    /// Some messages can be caught with .fallback or variants
+    Message(Message),
     /// Parsing failed and this is the final output
     ParseFailure(ParseFailure),
     /// Expected one of those values
@@ -17,6 +16,57 @@ pub enum Error {
     Missing(Vec<MissingItem>),
 }
 
+#[derive(Debug)]
+pub enum Message {
+    /// Tried to consume an env variable with no fallback, variable was not set
+    NoEnv(&'static str),
+
+    /// User specified an error message on some
+    ParseSome(&'static str),
+
+    /// User asked for parser to fail explicitly
+    ParseFail(&'static str),
+
+    /// pure_with failed to parse a value
+    PureFailed(String),
+
+    /// Tried to consume a strict positional argument, value was present but was not strictly
+    /// positional
+    StrictPos(Metavar),
+
+    /// User specified guard failed
+    Guard(&'static str),
+
+    /// Parser provided by user failed to parse a value
+    ParseFailed(Option<usize>, String),
+
+    /// Parser provided by user failed to validate a value
+    ValidateFailed(Option<usize>, String),
+
+    /// Argument requres a value but something else was passed,
+    /// required: --foo <BAR>
+    /// given: --foo --bar
+    ///        --foo -- bar
+    ///        --foo
+    NoArgument(usize),
+}
+
+impl Message {
+    pub(crate) fn can_catch(&self) -> bool {
+        match self {
+            Message::NoEnv(_)
+            | Message::ParseSome(_)
+            | Message::ParseFail(_)
+            | Message::PureFailed(_) => true,
+            Message::StrictPos(_)
+            | Message::Guard(_)
+            | Message::ParseFailed(_, _)
+            | Message::ValidateFailed(_, _)
+            | Message::NoArgument(_) => false,
+        }
+    }
+}
+
 /// Missing item in a context
 #[derive(Debug, Clone)]
 pub struct MissingItem {
@@ -39,8 +89,8 @@ impl Error {
             (_, b @ Error::ParseFailure(_)) => b,
 
             // unconditional parsing failure takes priority
-            (a @ Error::Message(_, _), _) => a,
-            (_, b @ Error::Message(_, _)) => b,
+            (a @ Error::Message(_), _) => a,
+            (_, b @ Error::Message(_)) => b,
 
             // combine missing elements
             (Error::Missing(mut a), Error::Missing(mut b)) => {
@@ -49,6 +99,13 @@ impl Error {
             }
         }
     }
+    pub(crate) fn can_catch(&self) -> bool {
+        match self {
+            Error::Message(msg) => msg.can_catch(),
+            Error::ParseFailure(_) => false,
+            Error::Missing(_) => true,
+        }
+    }
 }
 
 /// Unsuccessful command line parsing outcome, use it for unit tests
diff --git a/src/help.rs b/src/help.rs
index 81d0f24..24b66ab 100644
--- a/src/help.rs
+++ b/src/help.rs
@@ -1,10 +1,19 @@
 //! Improve non-parse cases
 //!
 //! covers `--help`, `--version` etc.
+//!
+//! # Notation rules:
+//! - Items in backticks refer to valid flag names known to bpaf:
+//!       expected `--foo`, etc
+//! - Items in quotes refer to user input which might or might not be a valid flag:
+//!       "--foo" cannot be used at the same time as "--bar"
 
 use crate::{
-    error::MissingItem, info::Info, meta_help::render_help, short, Args, Error, Meta, ParseFailure,
-    Parser,
+    args::Arg,
+    error::{Message, MissingItem},
+    info::Info,
+    meta_help::render_help,
+    short, Args, Error, Meta, ParseFailure, Parser,
 };
 
 struct ParseExtraParams {
@@ -19,10 +28,7 @@ impl Parser<ExtraParams> for ParseExtraParams {
 
         match self.version {
             Some(ver) => Self::ver(ver).eval(args),
-            None => Err(Error::Message(
-                String::from("Not a version or help flag"),
-                false,
-            )),
+            None => Err(Error::Missing(Vec::new())),
         }
     }
 
@@ -65,6 +71,14 @@ impl Info {
     }
 }
 
+fn check_conflicts(args: &Args) -> Option<String> {
+    let (loser, winner) = args.conflict()?;
+    Some(format!(
+        "\"{}\" cannot be used at the same time as \"{}\"",
+        args.items[loser], args.items[winner]
+    ))
+}
+
 pub(crate) fn improve_error(
     args: &mut Args,
     info: &Info,
@@ -97,7 +111,7 @@ pub(crate) fn improve_error(
     ParseFailure::Stderr(match err {
         // parse succeeded, need to explain an unused argument
         None => {
-            if let Some(msg) = crate::info::check_conflicts(args) {
+            if let Some(msg) = check_conflicts(args) {
                 msg
             } else if let Some(msg) = crate::meta_youmean::suggest(args, inner) {
                 msg
@@ -110,11 +124,63 @@ pub(crate) fn improve_error(
             }
         }
         Some(Error::ParseFailure(f)) => return f,
-        Some(Error::Message(msg, _)) => msg,
+        Some(Error::Message(msg)) => msg.render(args),
         Some(Error::Missing(xs)) => summarize_missing(&xs, inner, args),
     })
 }
 
+fn textual_part(args: &Args, ix: Option<usize>) -> Option<String> {
+    match args.items.get(ix?)? {
+        Arg::Ambiguity(_, _) | Arg::Short(_, _, _) | Arg::Long(_, _, _) => None,
+        Arg::Word(s) | Arg::PosWord(s) => Some(s.to_string_lossy().to_string()),
+    }
+}
+
+impl Message {
+    fn render(self, args: &Args) -> String {
+        match self {
+            Message::NoEnv(name) => {
+                format!("env variable {} is not set", name)
+            }
+            Message::StrictPos(x) => {
+                format!("Expected {} to be on the right side of --", x)
+            }
+            Message::ParseSome(s) => s.to_string(),
+            Message::Guard(_) => todo!(),
+            Message::ParseFail(s) => s.to_owned(),
+            Message::ParseFailed(mix, s) => match textual_part(args, mix) {
+                Some(field) => format!("Couldn't parse {:?}: {}", field, s),
+                None => format!("Couldn't parse: {}", s),
+            },
+            Message::ValidateFailed(mix, s) => match textual_part(args, mix) {
+                Some(field) => format!("{:?}: {}", field, s),
+                None => format!("Couldn't parse: {}", s),
+            },
+            Message::NoArgument(x) => match args.items.get(x + 1) {
+                Some(Arg::Ambiguity(_, _)) => unreachable!("unresolevd ambiguity?"),
+                Some(Arg::Short(_, _, os) | Arg::Long(_, _, os)) => {
+                    let arg = &args.items[x];
+                    if let (Arg::Short(s, _, fos), true) = (&arg, os.is_empty()) {
+                        let fos = fos.to_string_lossy();
+                        let repl = fos.strip_prefix('-').unwrap().strip_prefix(*s).unwrap();
+                        format!(
+                            "`{}` is not accepted, try using it as `-{}={}`",
+                            fos, s, repl
+                        )
+                    } else {
+                        let os = os.to_string_lossy();
+                        format!( "`{}` requires an argument, got a flag-like `{}`, try `{}={}` to use it as an argument", arg, os, arg,os)
+                    }
+                }
+                Some(Arg::Word(_)) => unreachable!("this is an argument!"),
+                Some(Arg::PosWord(_)) => todo!(),
+                None => format!("{} requires an argument", args.items[x]),
+            },
+            Message::PureFailed(s) => s,
+        }
+    }
+}
+
 #[inline(never)]
 pub(crate) fn summarize_missing(items: &[MissingItem], inner: &Meta, args: &Args) -> String {
     // missing items can belong to different scopes, pick the best scope to work with
diff --git a/src/info.rs b/src/info.rs
index 601bb23..4e7f368 100644
--- a/src/info.rs
+++ b/src/info.rs
@@ -1,10 +1,6 @@
 //! Help message generation and rendering
 
-use crate::{
-    args::{Args, Conflict},
-    parsers::ParseCommand,
-    Error, ParseFailure, Parser,
-};
+use crate::{args::Args, parsers::ParseCommand, Error, ParseFailure, Parser};
 
 /// Information about the parser
 ///
@@ -24,18 +20,6 @@ pub struct Info {
     pub usage: Option<&'static str>,
 }
 
-pub(crate) fn check_conflicts(args: &Args) -> Option<String> {
-    let (ix, _item) = args.items_iter().next()?;
-    if let Conflict::Conflicts(acc, rej) = args.conflicts.get(&ix)? {
-        Some(format!(
-            "{} cannot be used at the same time as {}",
-            rej, acc
-        ))
-    } else {
-        None
-    }
-}
-
 /// Ready to run [`Parser`] with additional information attached
 ///
 /// Created with [`to_options`](Parser::to_options)
diff --git a/src/meta.rs b/src/meta.rs
index bc19c43..d8b5422 100644
--- a/src/meta.rs
+++ b/src/meta.rs
@@ -11,12 +11,19 @@ pub enum DecorPlace {
 #[doc(hidden)]
 #[derive(Clone, Debug)]
 pub enum Meta {
+    /// All arguments listed in a vector must be present
     And(Vec<Meta>),
+    /// One of arguments listed in a vector must be present
     Or(Vec<Meta>),
+    /// Arguments are optional and encased in [] when rendered
     Optional(Box<Meta>),
+    /// Arguments are requred and encased in () when rendered
     Required(Box<Meta>),
+    /// Argumens are required to be adjacent to each other
     Adjacent(Box<Meta>),
+    /// Primitive argument as described
     Item(Box<Item>),
+    /// Accepts multiple arguments
     Many(Box<Meta>),
     Decorated(Box<Meta>, String, DecorPlace),
     Skip,
@@ -173,6 +180,20 @@ impl Meta {
         m
     }
 
+    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::Skip | Meta::Or(_) => None,
+            Meta::Optional(x)
+            | Meta::Required(x)
+            | Meta::Adjacent(x)
+            | Meta::Many(x)
+            | Meta::Decorated(x, _, _)
+            | Meta::HideUsage(x) => Self::first_item(x),
+        }
+    }
+
     /// Normalize meta info for display as usage. Required propagates outwards
     fn normalize(&mut self, for_usage: bool) {
         fn normalize_vec(xs: &mut Vec<Meta>, for_usage: bool) -> Option<Meta> {
diff --git a/src/meta_help.rs b/src/meta_help.rs
index 42e68a4..17c2cce 100644
--- a/src/meta_help.rs
+++ b/src/meta_help.rs
@@ -133,7 +133,6 @@ fn dedup(items: &mut BTreeSet<String>, buf: &mut Buffer, cp: Checkpoint) {
 }
 
 #[derive(Copy, Clone, Eq, PartialEq, Debug)]
-
 pub(crate) enum HiTy {
     Flag,
     Command,
@@ -551,6 +550,13 @@ fn write_as_lines(buf: &mut Buffer, line: &str) {
 }
 
 pub fn render_help(info: &Info, parser_meta: &Meta, help_meta: &Meta) -> String {
+    fn write_section_title(buf: &mut Buffer, label: &str) {
+        buf.newline();
+        buf.margin(0);
+        buf.write_str(label, Style::Section);
+        buf.newline();
+    }
+
     let mut res = String::new();
     let mut buf = Buffer::default();
 
@@ -581,12 +587,6 @@ pub fn render_help(info: &Info, parser_meta: &Meta, help_meta: &Meta) -> String
     items.append_meta(parser_meta);
     items.append_meta(help_meta);
 
-    fn write_section_title(buf: &mut Buffer, label: &str) {
-        buf.newline();
-        buf.margin(0);
-        buf.write_str(label, Style::Section);
-        buf.newline();
-    }
     res.push_str(&buf.to_string());
 
     let section = items.write_items(HiTy::Positional);
diff --git a/src/params.rs b/src/params.rs
index 90394f0..52b39ec 100644
--- a/src/params.rs
+++ b/src/params.rs
@@ -62,7 +62,11 @@ use std::{ffi::OsString, marker::PhantomData, str::FromStr};
 
 use super::{Args, Error, OptionParser, Parser};
 use crate::{
-    args::Arg, error::MissingItem, from_os_str::parse_os_str, item::ShortLong, meta_help::Metavar,
+    args::Arg,
+    error::{Message, MissingItem},
+    from_os_str::parse_os_str,
+    item::ShortLong,
+    meta_help::Metavar,
     Item, Meta,
 };
 
@@ -745,8 +749,7 @@ impl<T> ParseArgument<T> {
     fn take_argument(&self, args: &mut Args) -> Result<OsString, Error> {
         if self.named.short.is_empty() && self.named.long.is_empty() {
             if let Some(name) = self.named.env.first() {
-                let msg = format!("env variable {} is not set", name);
-                return Err(Error::Message(msg, true));
+                return Err(Error::Message(Message::NoEnv(name)));
             }
         }
         match args.take_arg(&self.named, self.adjacent) {
@@ -790,7 +793,7 @@ where
         let os = self.take_argument(args)?;
         match parse_os_str::<T>(os) {
             Ok(ok) => Ok(ok),
-            Err(err) => Err(args.word_parse_error(&err)),
+            Err(err) => Err(Error::Message(Message::ParseFailed(args.current, err))),
         }
     }
 
@@ -929,33 +932,31 @@ fn parse_word(
     metavar: &'static str,
     help: &Option<String>,
 ) -> Result<OsString, Error> {
-    if let Some((is_strict, word)) = args.take_positional_word(Metavar(metavar))? {
+    let metavar = Metavar(metavar);
+    if let Some((is_strict, word)) = args.take_positional_word(metavar)? {
         if strict && !is_strict {
             #[cfg(feature = "autocomplete")]
             args.push_value("--", &Some("-- Positional only items".to_owned()), false);
 
-            return Err(Error::Message(
-                format!("Expected <{}> to be on the right side of --", metavar),
-                false,
-            ));
+            return Err(Error::Message(Message::StrictPos(metavar)));
         }
         #[cfg(feature = "autocomplete")]
         if args.touching_last_remove() && !args.no_pos_ahead {
-            args.push_metadata(metavar, help, false);
+            args.push_metadata(metavar.0, help, false);
             args.no_pos_ahead = true;
         }
         Ok(word)
     } else {
         #[cfg(feature = "autocomplete")]
         if !args.no_pos_ahead {
-            args.push_metadata(metavar, help, false);
+            args.push_metadata(metavar.0, help, false);
             args.no_pos_ahead = true;
         }
 
         let position = args.items_iter().next().map_or(args.scope().end, |x| x.0);
         let missing = MissingItem {
             item: Item::Positional {
-                metavar: Metavar(metavar),
+                metavar,
                 help: help.clone(),
                 strict,
                 anywhere: false,
@@ -976,7 +977,7 @@ where
         let os = parse_word(args, self.strict, self.metavar, &self.help)?;
         match parse_os_str::<T>(os) {
             Ok(ok) => Ok(ok),
-            Err(err) => Err(args.word_parse_error(&err)),
+            Err(err) => Err(Error::Message(Message::ParseFailed(args.current, err))),
         }
     }
 
diff --git a/src/structs.rs b/src/structs.rs
index a43f823..40935e7 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -1,6 +1,8 @@
 //! Structures that implement different methods on [`Parser`] trait
 use crate::{
-    args::Conflict, error::MissingItem, item::Item, meta::DecorPlace, Args, Error, Meta, Parser,
+    error::{Message, MissingItem},
+    meta::DecorPlace,
+    Args, Error, Meta, Parser,
 };
 use std::marker::PhantomData;
 
@@ -32,12 +34,13 @@ where
             Err(e) => {
                 #[cfg(feature = "autocomplete")]
                 args.swap_comps(&mut clone);
-                match e {
-                    Error::Message(_, false) | Error::ParseFailure(_) => Err(e),
-                    Error::Missing(_) | Error::Message(_, true) => match (self.fallback)() {
+                if e.can_catch() {
+                    match (self.fallback)() {
                         Ok(ok) => Ok(ok),
-                        Err(e) => Err(Error::Message(e.to_string(), false)),
-                    },
+                        Err(e) => Err(Error::Message(Message::PureFailed(e.to_string()))),
+                    }
+                } else {
+                    Err(e)
                 }
             }
         }
@@ -114,7 +117,7 @@ where
         }
 
         if res.is_empty() {
-            Err(Error::Message(self.message.to_string(), true))
+            Err(Error::Message(Message::ParseSome(self.message)))
         } else {
             Ok(res)
         }
@@ -192,17 +195,24 @@ impl<T> Parser<T> for ParseOrElse<T> {
         #[cfg(feature = "autocomplete")]
         args.swap_comps_with(&mut comp_items);
 
-        // create forks for both branches, go with a successful one.
+        // create forks for both branches
         // if they both fail - fallback to the original arguments
+        // 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
+        // all invocations of parsers rather than being inlined into each one.
+
         let (res_a, err_a) = match self.this.eval(&mut args_a) {
             Ok(ok) => (Some(ok), None),
             Err(err) => (None, Some(err)),
         };
 
-        let mut args_b = args.clone();
-        args_b.head = usize::MAX;
         let (res_b, err_b) = match self.that.eval(&mut args_b) {
             Ok(ok) => (Some(ok), None),
             Err(err) => (None, Some(err)),
@@ -217,18 +227,8 @@ impl<T> Parser<T> for ParseOrElse<T> {
             #[cfg(feature = "autocomplete")]
             comp_items,
         )? {
-            if res_b.is_some() {
-                remember_conflict(args, self.this.meta(), &mut args_b, self.that.meta());
-            } else {
-                remember_winner(args, self.this.meta());
-            }
             Ok(res_a.unwrap())
         } else {
-            if res_a.is_some() {
-                remember_conflict(args, self.that.meta(), &mut args_a, self.this.meta());
-            } else {
-                remember_winner(args, self.that.meta());
-            }
             Ok(res_b.unwrap())
         }
     }
@@ -238,32 +238,8 @@ impl<T> Parser<T> for ParseOrElse<T> {
     }
 }
 
-fn remember_winner(args: &mut Args, meta: Meta) {
-    args.conflicts
-        .entry(args.head)
-        .or_insert(Conflict::Solo(meta));
-}
-
-fn remember_conflict(args: &mut Args, winner: Meta, failed: &mut Args, loser: Meta) {
-    let winner = args
-        .conflicts
-        .entry(args.head)
-        .or_insert(Conflict::Solo(winner))
-        .winner()
-        .clone();
-
-    let loser = failed
-        .conflicts
-        .get(&failed.head)
-        .map(Conflict::winner)
-        .cloned()
-        .unwrap_or(loser);
-
-    args.conflicts
-        .entry(failed.head)
-        .or_insert(Conflict::Conflicts(winner, loser));
-}
-
+/// Given two possible errors along with to sets of arguments produce a new error or an instruction
+/// to pick between two answers. Updates arguments state to match the results
 fn this_or_that_picks_first(
     err_a: Option<Error>,
     err_b: Option<Error>,
@@ -307,28 +283,39 @@ 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)]
+    #[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 {
-                std::mem::swap(args, args_a);
                 Ok(true)
             } else {
-                std::mem::swap(args, args_b);
                 Ok(false)
             }
         }
-        (None, Some(_)) => {
+        (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()),
+    };
+
+    match res {
+        Ok(true) => {
+            if ok_b {
+                args_a.save_conflicts(args_b);
+            }
             std::mem::swap(args, args_a);
-            Ok(true)
         }
-        (Some(_), None) => {
+        Ok(false) => {
+            if ok_a {
+                args_b.save_conflicts(args_a);
+            }
             std::mem::swap(args, args_b);
-            Ok(false)
         }
-        (Some(e1), Some(e2)) => Err(e1.combine_with(e2)),
-    };
+        // no winner, keep the completions but don't touch args otherwise
+        Err(_) => {}
+    }
 
     #[cfg(feature = "autocomplete")]
     if let Some(comp) = args.comp_mut() {
@@ -358,7 +345,10 @@ where
         let t = self.inner.eval(args)?;
         match (self.parse_fn)(t) {
             Ok(r) => Ok(r),
-            Err(e) => Err(args.word_parse_error(&e.to_string())),
+            Err(e) => Err(Error::Message(Message::ParseFailed(
+                args.current,
+                e.to_string(),
+            ))),
         }
     }
 
@@ -390,9 +380,10 @@ where
             Err(e) => {
                 #[cfg(feature = "autocomplete")]
                 args.swap_comps(&mut clone);
-                match e {
-                    Error::Message(_, false) | Error::ParseFailure(_) => Err(e),
-                    Error::Missing(_) | Error::Message(_, true) => Ok(self.value.clone()),
+                if e.can_catch() {
+                    Ok(self.value.clone())
+                } else {
+                    Err(e)
                 }
             }
         }
@@ -449,7 +440,10 @@ where
         if (self.check)(&t) {
             Ok(t)
         } else {
-            Err(args.word_validate_error(self.message))
+            Err(Error::Message(Message::ValidateFailed(
+                args.current,
+                self.message.to_string(),
+            )))
         }
     }
 
@@ -541,8 +535,8 @@ where
             // anything left unconsumed - this won't be lost.
 
             let res = match &err {
-                Error::Message(_, can_catch) => {
-                    if *can_catch || catch {
+                Error::Message(msg) => {
+                    if msg.can_catch() || catch {
                         Ok(None)
                     } else {
                         Err(err)
@@ -619,7 +613,7 @@ impl<T: Clone + 'static, F: Fn() -> Result<T, E>, E: ToString> Parser<T>
     fn eval(&self, _args: &mut Args) -> Result<T, Error> {
         match (self.0)() {
             Ok(ok) => Ok(ok),
-            Err(e) => Err(Error::Message(e.to_string(), true)),
+            Err(e) => Err(Error::Message(Message::PureFailed(e.to_string()))),
         }
     }
 
@@ -636,7 +630,7 @@ pub struct ParseFail<T> {
 impl<T> Parser<T> for ParseFail<T> {
     fn eval(&self, args: &mut Args) -> Result<T, Error> {
         args.current = None;
-        Err(Error::Message(self.field1.to_owned(), true))
+        Err(Error::Message(Message::ParseFail(self.field1)))
     }
 
     fn meta(&self) -> Meta {
@@ -844,7 +838,7 @@ where
     fn eval(&self, args: &mut Args) -> Result<T, Error> {
         let original_scope = args.scope();
 
-        let mut best_error = if let Some(item) = first_item(&self.inner.meta()) {
+        let mut best_error = if let Some(item) = Meta::first_item(&self.inner.meta()) {
             let missing_item = MissingItem {
                 item,
                 position: original_scope.start,
@@ -923,20 +917,6 @@ where
             }
         }
 
-        fn first_item(meta: &Meta) -> Option<Item> {
-            match meta {
-                Meta::And(xs) => xs.first().and_then(first_item),
-                Meta::Item(item) => Some(*item.clone()),
-                Meta::Skip | Meta::Or(_) => None,
-                Meta::Optional(x)
-                | Meta::Required(x)
-                | Meta::Adjacent(x)
-                | Meta::Many(x)
-                | Meta::Decorated(x, _, _)
-                | Meta::HideUsage(x) => first_item(x),
-            }
-        }
-
         std::mem::swap(args, &mut best_args);
         Err(best_error)
     }


Test patch:
diff --git a/tests/errors.rs b/tests/errors.rs
index 681b01b..e22185e 100644
--- a/tests/errors.rs
+++ b/tests/errors.rs
@@ -14,25 +14,37 @@ fn better_error_with_enum() {
         .run_inner(Args::from(&["--alpha", "--beta"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "--beta cannot be used at the same time as --alpha");
+    assert_eq!(
+        res,
+        "\"--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 cannot be used at the same time as --alpha");
+    assert_eq!(
+        res,
+        "\"--gamma\" cannot be used at the same time as \"--alpha\""
+    );
 
     let res = foo()
         .run_inner(Args::from(&["--beta", "--gamma"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "--gamma cannot be used at the same time as --beta");
+    assert_eq!(
+        res,
+        "\"--gamma\" cannot be used at the same time as \"--beta\""
+    );
 
     let res = foo()
         .run_inner(Args::from(&["--alpha", "--beta", "--gamma"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "--beta cannot be used at the same time as --alpha");
+    assert_eq!(
+        res,
+        "\"--beta\" cannot be used at the same time as \"--alpha\""
+    );
 }
 
 #[test]
diff --git a/tests/meta_youmean.rs b/tests/meta_youmean.rs
index a4e17bc..2cdacd2 100644
--- a/tests/meta_youmean.rs
+++ b/tests/meta_youmean.rs
@@ -291,3 +291,31 @@ Available commands:
 ";
     assert_eq!(r, expected);
 }
+
+#[test]
+fn big_conflict() {
+    let a = short('a').switch();
+    let b = short('b').switch();
+    let c = short('c').switch();
+    let d = short('d').switch();
+    let ab = construct!(a, b);
+    let cd = construct!(c, d);
+    let parser = construct!([ab, cd]).to_options();
+    let r = parser
+        .run_inner(Args::from(&["-a", "-b", "-c", "-d"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    let expected = "\"-c\" cannot be used at the same time as \"-a\"";
+    assert_eq!(r, expected);
+}
+
+#[test]
+fn pure_conflict() {
+    let a = short('a').switch();
+    let b = pure(false);
+    let parser = construct!([a, b]).to_options();
+    let r = parser.run_inner(Args::from(&[])).unwrap();
+    assert!(!r);
+    let r = parser.run_inner(Args::from(&["-a"])).unwrap();
+    assert!(r);
+}
diff --git a/tests/tests.rs b/tests/tests.rs
index 58d66f4..723c017 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -502,31 +502,31 @@ fn from_several_alternatives_pick_more_meaningful() {
         .run_inner(Args::from(&["-a", "-b"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err1, "-b cannot be used at the same time as -a");
+    assert_eq!(err1, "\"-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 cannot be used at the same time as -b");
+    assert_eq!(err2, "\"-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 cannot be used at the same time as -c");
+    assert_eq!(err3, "\"-a\" 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 cannot be used at the same time as -a");
+    assert_eq!(err4, "\"-c\" cannot be used at the same time as \"-a\"");
 
     let err5 = parser
         .run_inner(Args::from(&["-c", "-b", "-a"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(err5, "-b cannot be used at the same time as -c");
+    assert_eq!(err5, "\"-b\" cannot be used at the same time as \"-c\"");
 }
 
 #[test]
@@ -1371,7 +1371,7 @@ fn did_you_mean_two_or_arguments() {
         .unwrap_stderr();
     assert_eq!(
         r,
-        "[--flag] cannot be used at the same time as [--parameter]"
+        "\"--flag\" cannot be used at the same time as \"--parameter\""
     );
 
     let r = parser
@@ -1380,7 +1380,7 @@ fn did_you_mean_two_or_arguments() {
         .unwrap_stderr();
     assert_eq!(
         r,
-        "[--parameter] cannot be used at the same time as [--flag]"
+        "\"--parameter\" cannot be used at the same time as \"--flag\""
     );
 }
 
@@ -1521,7 +1521,7 @@ fn did_you_mean_inside_command() {
         .unwrap_stderr();
     assert_eq!(
         r,
-        "[--flag] cannot be used at the same time as [--parameter]"
+        "\"--flag\" cannot be used at the same time as \"--parameter\""
     );
 
     let r = parser
@@ -1530,7 +1530,7 @@ fn did_you_mean_inside_command() {
         .unwrap_stderr();
     assert_eq!(
         r,
-        "[--parameter] cannot be used at the same time as [--flag]"
+        "\"--parameter\" cannot be used at the same time as \"--flag\""
     );
 }