Auto merge of #10394 - dtolnay-contrib:displayerror, r=ehuss

bors · bors · commit 5e09899f3374 · 2022-03-21T18:48:36.000Z
Consistently use crate::display_error on errors during drain As suggested in #10383 (comment) and #10383 (comment).
diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs
@@ -77,6 +77,7 @@ use crate::core::compiler::future_incompat::{
 use crate::core::resolver::ResolveBehavior;
 use crate::core::{PackageId, Shell, TargetKind};
 use crate::util::diagnostic_server::{self, DiagnosticPrinter};
+use crate::util::errors::AlreadyPrintedError;
 use crate::util::machine_message::{self, Message as _};
 use crate::util::CargoResult;
 use crate::util::{self, internal, profile};
@@ -169,6 +170,41 @@ struct DrainState<'cfg> {
     per_package_future_incompat_reports: Vec<FutureIncompatReportPackage>,
 }
 
+pub struct ErrorsDuringDrain {
+    pub count: usize,
+}
+
+struct ErrorToHandle {
+    error: anyhow::Error,
+
+    /// This field is true for "interesting" errors and false for "mundane"
+    /// errors. If false, we print the above error only if it's the first one
+    /// encountered so far while draining the job queue.
+    ///
+    /// At most places that an error is propagated, we set this to false to
+    /// avoid scenarios where Cargo might end up spewing tons of redundant error
+    /// messages. For example if an i/o stream got closed somewhere, we don't
+    /// care about individually reporting every thread that it broke; just the
+    /// first is enough.
+    ///
+    /// The exception where print_always is true is that we do report every
+    /// instance of a rustc invocation that failed with diagnostics. This
+    /// corresponds to errors from Message::Finish.
+    print_always: bool,
+}
+
+impl<E> From<E> for ErrorToHandle
+where
+    anyhow::Error: From<E>,
+{
+    fn from(error: E) -> Self {
+        ErrorToHandle {
+            error: anyhow::Error::from(error),
+            print_always: false,
+        }
+    }
+}
+
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub struct JobId(pub u32);
 
@@ -613,7 +649,7 @@ impl<'cfg> DrainState<'cfg> {
         jobserver_helper: &HelperThread,
         plan: &mut BuildPlan,
         event: Message,
-    ) -> CargoResult<()> {
+    ) -> Result<(), ErrorToHandle> {
         match event {
             Message::Run(id, cmd) => {
                 cx.bcx
@@ -678,11 +714,14 @@ impl<'cfg> DrainState<'cfg> {
                 debug!("end ({:?}): {:?}", unit, result);
                 match result {
                     Ok(()) => self.finish(id, &unit, artifact, cx)?,
-                    Err(e) => {
+                    Err(error) => {
                         let msg = "The following warnings were emitted during compilation:";
                         self.emit_warnings(Some(msg), &unit, cx)?;
                         self.back_compat_notice(cx, &unit)?;
-                        return Err(e);
+                        return Err(ErrorToHandle {
+                            error,
+                            print_always: true,
+                        });
                     }
                 }
             }
@@ -787,14 +826,14 @@ impl<'cfg> DrainState<'cfg> {
         // After a job has finished we update our internal state if it was
         // successful and otherwise wait for pending work to finish if it failed
         // and then immediately return.
-        let mut error = None;
+        let mut errors = ErrorsDuringDrain { count: 0 };
         // CAUTION! Do not use `?` or break out of the loop early. Every error
         // must be handled in such a way that the loop is still allowed to
         // drain event messages.
         loop {
-            if error.is_none() {
+            if errors.count == 0 {
                 if let Err(e) = self.spawn_work_if_possible(cx, jobserver_helper, scope) {
-                    self.handle_error(&mut cx.bcx.config.shell(), &mut error, e);
+                    self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e);
                 }
             }
 
@@ -805,7 +844,7 @@ impl<'cfg> DrainState<'cfg> {
             }
 
             if let Err(e) = self.grant_rustc_token_requests() {
-                self.handle_error(&mut cx.bcx.config.shell(), &mut error, e);
+                self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e);
             }
 
             // And finally, before we block waiting for the next event, drop any
@@ -815,7 +854,7 @@ impl<'cfg> DrainState<'cfg> {
             // to the jobserver itself.
             for event in self.wait_for_events() {
                 if let Err(event_err) = self.handle_event(cx, jobserver_helper, plan, event) {
-                    self.handle_error(&mut cx.bcx.config.shell(), &mut error, event_err);
+                    self.handle_error(&mut cx.bcx.config.shell(), &mut errors, event_err);
                 }
             }
         }
@@ -840,30 +879,24 @@ impl<'cfg> DrainState<'cfg> {
         }
 
         let time_elapsed = util::elapsed(cx.bcx.config.creation_time().elapsed());
-        if let Err(e) = self.timings.finished(cx, &error) {
-            if error.is_some() {
-                crate::display_error(&e, &mut cx.bcx.config.shell());
-            } else {
-                return Some(e);
-            }
+        if let Err(e) = self.timings.finished(cx, &errors.to_error()) {
+            self.handle_error(&mut cx.bcx.config.shell(), &mut errors, e);
         }
         if cx.bcx.build_config.emit_json() {
             let mut shell = cx.bcx.config.shell();
             let msg = machine_message::BuildFinished {
-                success: error.is_none(),
+                success: errors.count == 0,
             }
             .to_json_string();
             if let Err(e) = writeln!(shell.out(), "{}", msg) {
-                if error.is_some() {
-                    crate::display_error(&e.into(), &mut shell);
-                } else {
-                    return Some(e.into());
-                }
+                self.handle_error(&mut shell, &mut errors, e);
             }
         }
 
-        if let Some(e) = error {
-            Some(e)
+        if let Some(error) = errors.to_error() {
+            // Any errors up to this point have already been printed via the
+            // `display_error` inside `handle_error`.
+            Some(anyhow::Error::new(AlreadyPrintedError::new(error)))
         } else if self.queue.is_empty() && self.pending_queue.is_empty() {
             let message = format!(
                 "{} [{}] target(s) in {}",
@@ -888,18 +921,18 @@ impl<'cfg> DrainState<'cfg> {
     fn handle_error(
         &self,
         shell: &mut Shell,
-        err_state: &mut Option<anyhow::Error>,
-        new_err: anyhow::Error,
+        err_state: &mut ErrorsDuringDrain,
+        new_err: impl Into<ErrorToHandle>,
     ) {
-        if err_state.is_some() {
-            // Already encountered one error.
-            log::warn!("{:?}", new_err);
-        } else if !self.active.is_empty() {
-            crate::display_error(&new_err, shell);
-            drop(shell.warn("build failed, waiting for other jobs to finish..."));
-            *err_state = Some(anyhow::format_err!("build failed"));
+        let new_err = new_err.into();
+        if new_err.print_always || err_state.count == 0 {
+            crate::display_error(&new_err.error, shell);
+            if err_state.count == 0 && !self.active.is_empty() {
+                drop(shell.warn("build failed, waiting for other jobs to finish..."));
+            }
+            err_state.count += 1;
         } else {
-            *err_state = Some(new_err);
+            log::warn!("{:?}", new_err.error);
         }
     }
 
@@ -1217,3 +1250,13 @@ feature resolver. Try updating to diesel 1.4.8 to fix this error.
         Ok(())
     }
 }
+
+impl ErrorsDuringDrain {
+    fn to_error(&self) -> Option<anyhow::Error> {
+        match self.count {
+            0 => None,
+            1 => Some(format_err!("1 job failed")),
+            n => Some(format_err!("{} jobs failed", n)),
+        }
+    }
+}
diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs
@@ -11,7 +11,7 @@ use crate::core::Shell;
 use anyhow::Error;
 use log::debug;
 
-pub use crate::util::errors::{InternalError, VerboseError};
+pub use crate::util::errors::{AlreadyPrintedError, InternalError, VerboseError};
 pub use crate::util::{indented_lines, CargoResult, CliError, CliResult, Config};
 pub use crate::version::version;
 
@@ -74,31 +74,27 @@ pub fn display_warning_with_error(warning: &str, err: &Error, shell: &mut Shell)
 }
 
 fn _display_error(err: &Error, shell: &mut Shell, as_err: bool) -> bool {
-    let verbosity = shell.verbosity();
-    let is_verbose = |e: &(dyn std::error::Error + 'static)| -> bool {
-        verbosity != Verbose && e.downcast_ref::<VerboseError>().is_some()
-    };
-    // Generally the top error shouldn't be verbose, but check it anyways.
-    if is_verbose(err.as_ref()) {
-        return true;
-    }
-    if as_err {
-        drop(shell.error(&err));
-    } else {
-        drop(writeln!(shell.err(), "{}", err));
-    }
-    for cause in err.chain().skip(1) {
-        // If we're not in verbose mode then print remaining errors until one
+    for (i, err) in err.chain().enumerate() {
+        // If we're not in verbose mode then only print cause chain until one
         // marked as `VerboseError` appears.
-        if is_verbose(cause) {
+        //
+        // Generally the top error shouldn't be verbose, but check it anyways.
+        if shell.verbosity() != Verbose && err.is::<VerboseError>() {
             return true;
         }
-        drop(writeln!(shell.err(), "\nCaused by:"));
-        drop(write!(
-            shell.err(),
-            "{}",
-            indented_lines(&cause.to_string())
-        ));
+        if err.is::<AlreadyPrintedError>() {
+            break;
+        }
+        if i == 0 {
+            if as_err {
+                drop(shell.error(&err));
+            } else {
+                drop(writeln!(shell.err(), "{}", err));
+            }
+        } else {
+            drop(writeln!(shell.err(), "\nCaused by:"));
+            drop(write!(shell.err(), "{}", indented_lines(&err.to_string())));
+        }
     }
     false
 }
diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs
@@ -99,6 +99,39 @@ impl fmt::Display for InternalError {
     }
 }
 
+// =============================================================================
+// Already printed error
+
+/// An error that does not need to be printed because it does not add any new
+/// information to what has already been printed.
+pub struct AlreadyPrintedError {
+    inner: Error,
+}
+
+impl AlreadyPrintedError {
+    pub fn new(inner: Error) -> Self {
+        AlreadyPrintedError { inner }
+    }
+}
+
+impl std::error::Error for AlreadyPrintedError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.inner.source()
+    }
+}
+
+impl fmt::Debug for AlreadyPrintedError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.inner.fmt(f)
+    }
+}
+
+impl fmt::Display for AlreadyPrintedError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.inner.fmt(f)
+    }
+}
+
 // =============================================================================
 // Manifest error
 
diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs
@@ -5949,7 +5949,6 @@ fn close_output() {
 hello stderr!
 [ERROR] [..]
 [WARNING] build failed, waiting for other jobs to finish...
-[ERROR] [..]
 ",
         &stderr,
         None,
diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs
@@ -879,11 +879,9 @@ fn compile_failure() {
         .with_status(101)
         .with_stderr_contains(
             "\
+[ERROR] could not compile `foo` due to previous error
 [ERROR] failed to compile `foo v0.0.1 ([..])`, intermediate artifacts can be \
     found at `[..]target`
-
-Caused by:
-  could not compile `foo` due to previous error
 ",
         )
         .run();
