refactor(validator): Decouple parser (#3645)

* fix(validate): Consistent conflict behavior

We had two different implementations of conflicts
- Validating conflicts
- Allowing conflicts to override `required`
  - Missing members of a group conflicting with each other
  - Missing symmetric conflicts (A conflicts with B so B conflicts with
    A)

This consolidates their implementations which should fix overriding of
`required`.

* fix(validate): Overrides always ignore required

Before, if two arguments were required *and* overrode each other, then
`cmd --opt=1 --other=2` succeded but `cmd --other=2` failed despite
ignoring `--opt=1`.  Requiring `--opt=1` to be present but unavailable
doesn't help anyone and makes the behavior less predictable.

Now both commands will have the same behavior.

* refactor(parser): Pull out long-help determination

This isn't a parser policy but command-level policy.

* refactor(help): Make bool's meaning clearer

* refactor(help): Consolidate help errors

* refactor(help): Move help writing down a layer

* refactor(parser): Parser is solely responsible for populating ArgMatches

* refactor(validator): Decouple parser

src/build/arg.rs

@@ -4339,6 +4339,8 @@ impl<'help> Arg<'help> {
     /// **NOTE:** When an argument is overridden it is essentially as if it never was used, any
     /// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed
     ///
+    /// **NOTE:** Overriding an argument implies they [conflict][Arg::conflicts_with`].
+    ///
     /// **WARNING:** Positional arguments and options which accept
     /// [`Arg::multiple_occurrences`] cannot override themselves (or we
     /// would never be able to advance to the next positional). If a positional
@@ -4454,6 +4456,8 @@ impl<'help> Arg<'help> {
     /// **NOTE:** When an argument is overridden it is essentially as if it never was used, any
     /// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed
     ///
+    /// **NOTE:** Overriding an argument implies they [conflict][Arg::conflicts_with_all`].
+    ///
     /// # Examples
     ///
     /// ```rust

src/build/command.rs

@@ -20,10 +20,12 @@ use crate::build::{arg::ArgProvider, Arg, ArgGroup, ArgPredicate};
 use crate::error::ErrorKind;
 use crate::error::Result as ClapResult;
 use crate::mkeymap::MKeyMap;
+use crate::output::fmt::Stream;
 use crate::output::{fmt::Colorizer, Help, HelpWriter, Usage};
 use crate::parse::{ArgMatcher, ArgMatches, Parser};
 use crate::util::ChildGraph;
 use crate::util::{color::ColorChoice, Id, Key};
+use crate::PossibleValue;
 use crate::{Error, INTERNAL_ERROR_MSG};
 
 #[cfg(debug_assertions)]
@@ -696,7 +698,7 @@ impl<'help> App<'help> {
         self._build();
         let color = self.get_color();
 
-        let mut c = Colorizer::new(false, color);
+        let mut c = Colorizer::new(Stream::Stdout, color);
         let usage = Usage::new(self);
         Help::new(HelpWriter::Buffer(&mut c), self, &usage, false).write_help()?;
         c.print()
@@ -721,7 +723,7 @@ impl<'help> App<'help> {
         self._build();
         let color = self.get_color();
 
-        let mut c = Colorizer::new(false, color);
+        let mut c = Colorizer::new(Stream::Stdout, color);
         let usage = Usage::new(self);
         Help::new(HelpWriter::Buffer(&mut c), self, &usage, true).write_help()?;
         c.print()
@@ -4675,6 +4677,57 @@ impl<'help> App<'help> {
     pub(crate) fn get_display_order(&self) -> usize {
         self.disp_ord.unwrap_or(999)
     }
+
+    pub(crate) fn write_help_err(
+        &self,
+        mut use_long: bool,
+        stream: Stream,
+    ) -> ClapResult<Colorizer> {
+        debug!(
+            "Parser::write_help_err: use_long={:?}, stream={:?}",
+            use_long && self.use_long_help(),
+            stream
+        );
+
+        use_long = use_long && self.use_long_help();
+        let usage = Usage::new(self);
+
+        let mut c = Colorizer::new(stream, self.color_help());
+        Help::new(HelpWriter::Buffer(&mut c), self, &usage, use_long).write_help()?;
+        Ok(c)
+    }
+
+    pub(crate) fn use_long_help(&self) -> bool {
+        debug!("Command::use_long_help");
+        // In this case, both must be checked. This allows the retention of
+        // original formatting, but also ensures that the actual -h or --help
+        // specified by the user is sent through. If hide_short_help is not included,
+        // then items specified with hidden_short_help will also be hidden.
+        let should_long = |v: &Arg| {
+            v.long_help.is_some()
+                || v.is_hide_long_help_set()
+                || v.is_hide_short_help_set()
+                || cfg!(feature = "unstable-v4")
+                    && v.possible_vals.iter().any(PossibleValue::should_show_help)
+        };
+
+        // Subcommands aren't checked because we prefer short help for them, deferring to
+        // `cmd subcmd --help` for more.
+        self.get_long_about().is_some()
+            || self.get_before_long_help().is_some()
+            || self.get_after_long_help().is_some()
+            || self.get_arguments().any(should_long)
+    }
+
+    // Should we color the help?
+    pub(crate) fn color_help(&self) -> ColorChoice {
+        #[cfg(feature = "color")]
+        if self.is_disable_colored_help_set() {
+            return ColorChoice::Never;
+        }
+
+        self.get_color()
+    }
 }
 
 impl<'help> Default for App<'help> {

src/error/mod.rs

@@ -15,6 +15,7 @@ use std::{
 use crate::{
     build::Arg,
     output::fmt::Colorizer,
+    output::fmt::Stream,
     parse::features::suggestions,
     util::{color::ColorChoice, safe_exit, SUCCESS_CODE, USAGE_CODE},
     AppSettings, Command,
@@ -97,10 +98,14 @@ impl Error {
     /// Should the message be written to `stdout` or not?
     #[inline]
     pub fn use_stderr(&self) -> bool {
-        !matches!(
-            self.kind(),
-            ErrorKind::DisplayHelp | ErrorKind::DisplayVersion
-        )
+        self.stream() == Stream::Stderr
+    }
+
+    pub(crate) fn stream(&self) -> Stream {
+        match self.kind() {
+            ErrorKind::DisplayHelp | ErrorKind::DisplayVersion => Stream::Stdout,
+            _ => Stream::Stderr,
+        }
     }
 
     /// Prints the error and exits.
@@ -608,7 +613,7 @@ impl Error {
         if let Some(message) = self.inner.message.as_ref() {
             message.formatted()
         } else {
-            let mut c = Colorizer::new(self.use_stderr(), self.inner.color_when);
+            let mut c = Colorizer::new(self.stream(), self.inner.color_when);
 
             start_error(&mut c);
 
@@ -1090,7 +1095,7 @@ impl Message {
     fn format(&mut self, cmd: &Command, usage: String) {
         match self {
             Message::Raw(s) => {
-                let mut c = Colorizer::new(true, cmd.get_color());
+                let mut c = Colorizer::new(Stream::Stderr, cmd.get_color());
 
                 let mut message = String::new();
                 std::mem::swap(s, &mut message);
@@ -1107,7 +1112,7 @@ impl Message {
     fn formatted(&self) -> Cow<Colorizer> {
         match self {
             Message::Raw(s) => {
-                let mut c = Colorizer::new(true, ColorChoice::Never);
+                let mut c = Colorizer::new(Stream::Stderr, ColorChoice::Never);
                 start_error(&mut c);
                 c.none(s);
                 Cow::Owned(c)

src/macros.rs

@@ -933,7 +933,7 @@ macro_rules! debug {
     ($($arg:tt)*) => ({
         let prefix = format!("[{:>w$}] \t", module_path!(), w = 28);
         let body = format!($($arg)*);
-        let mut color = $crate::output::fmt::Colorizer::new(true, $crate::ColorChoice::Auto);
+        let mut color = $crate::output::fmt::Colorizer::new($crate::output::fmt::Stream::Stderr, $crate::ColorChoice::Auto);
         color.hint(prefix);
         color.hint(body);
         color.none("\n");

src/output/fmt.rs

@@ -5,19 +5,25 @@ use std::{
     io::{self, Write},
 };
 
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub(crate) enum Stream {
+    Stdout,
+    Stderr,
+}
+
 #[derive(Clone, Debug)]
 pub(crate) struct Colorizer {
-    use_stderr: bool,
+    stream: Stream,
     #[allow(unused)]
     color_when: ColorChoice,
     pieces: Vec<(String, Style)>,
 }
 
 impl Colorizer {
     #[inline(never)]
-    pub(crate) fn new(use_stderr: bool, color_when: ColorChoice) -> Self {
+    pub(crate) fn new(stream: Stream, color_when: ColorChoice) -> Self {
         Colorizer {
-            use_stderr,
+            stream,
             color_when,
             pieces: vec![],
         }
@@ -58,14 +64,13 @@ impl Colorizer {
 
         let color_when = match self.color_when {
             ColorChoice::Always => DepColorChoice::Always,
-            ColorChoice::Auto if is_a_tty(self.use_stderr) => DepColorChoice::Auto,
+            ColorChoice::Auto if is_a_tty(self.stream) => DepColorChoice::Auto,
             _ => DepColorChoice::Never,
         };
 
-        let writer = if self.use_stderr {
-            BufferWriter::stderr(color_when)
-        } else {
-            BufferWriter::stdout(color_when)
+        let writer = match self.stream {
+            Stream::Stdout => BufferWriter::stderr(color_when),
+            Stream::Stderr => BufferWriter::stdout(color_when),
         };
 
         let mut buffer = writer.buffer();
@@ -101,14 +106,17 @@ impl Colorizer {
     pub(crate) fn print(&self) -> io::Result<()> {
         // [e]println can't be used here because it panics
         // if something went wrong. We don't want that.
-        if self.use_stderr {
-            let stderr = std::io::stderr();
-            let mut stderr = stderr.lock();
-            write!(stderr, "{}", self)
-        } else {
-            let stdout = std::io::stdout();
-            let mut stdout = stdout.lock();
-            write!(stdout, "{}", self)
+        match self.stream {
+            Stream::Stdout => {
+                let stdout = std::io::stdout();
+                let mut stdout = stdout.lock();
+                write!(stdout, "{}", self)
+            }
+            Stream::Stderr => {
+                let stderr = std::io::stderr();
+                let mut stderr = stderr.lock();
+                write!(stderr, "{}", self)
+            }
         }
     }
 }
@@ -140,11 +148,10 @@ impl Default for Style {
 }
 
 #[cfg(feature = "color")]
-fn is_a_tty(stderr: bool) -> bool {
-    let stream = if stderr {
-        atty::Stream::Stderr
-    } else {
-        atty::Stream::Stdout
+fn is_a_tty(stream: Stream) -> bool {
+    let stream = match stream {
+        Stream::Stdout => atty::Stream::Stdout,
+        Stream::Stderr => atty::Stream::Stderr,
     };
 
     atty::is(stream)