Auto merge of #7896 - ehuss:internal-errors, r=alexcrichton

Rework internal errors.

This changes the behavior of "internal" errors, which were previously hidden and only displayed with `--verbose`. The changes here:

- `internal` now means "a cargo bug", and is always displayed and requests that the user file a bug report.
- Added `VerboseError` to signify an error that should only be displayed with `--verbose`. This is only used in one place, to display the `rustc` compiler command if the compiler exited with a normal error code (i.e. not a crash).
- Audited the uses of internal errors, and promoted some to normal errors, as they seemed to contain useful information, but weren't necessarily bugs in cargo.
- Added some details to some errors.
- Sometimes the "run with --verbose" message wasn't being printed when I think it should. This happened when rustc failed while other rustc processes were running. Another case was with `cargo install` installing multiple packages, and one of them fails.

Author: bors

crates/cargo-test-support/src/lib.rs

@@ -1645,6 +1645,7 @@ fn substitute_macros(input: &str) -> String {
         ("[FINISHED]", "    Finished"),
         ("[ERROR]", "error:"),
         ("[WARNING]", "warning:"),
+        ("[NOTE]", "note:"),
         ("[DOCUMENTING]", " Documenting"),
         ("[FRESH]", "       Fresh"),
         ("[UPDATING]", "    Updating"),
@@ -1666,7 +1667,6 @@ fn substitute_macros(input: &str) -> String {
         ("[IGNORED]", "     Ignored"),
         ("[INSTALLED]", "   Installed"),
         ("[REPLACED]", "    Replaced"),
-        ("[NOTE]", "        Note"),
     ];
     let mut result = input.to_owned();
     for &(pat, subst) in &macros {

src/cargo/core/compiler/context/mod.rs

@@ -9,7 +9,7 @@ use jobserver::Client;
 use crate::core::compiler::{self, compilation, Unit};
 use crate::core::PackageId;
 use crate::util::errors::{CargoResult, CargoResultExt};
-use crate::util::{internal, profile, Config};
+use crate::util::{profile, Config};
 
 use super::build_plan::BuildPlan;
 use super::custom_build::{self, BuildDeps, BuildScriptOutputs, BuildScripts};
@@ -313,11 +313,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.files_mut()
             .host
             .prepare()
-            .chain_err(|| internal("couldn't prepare build directories"))?;
+            .chain_err(|| "couldn't prepare build directories")?;
         for target in self.files.as_mut().unwrap().target.values_mut() {
             target
                 .prepare()
-                .chain_err(|| internal("couldn't prepare build directories"))?;
+                .chain_err(|| "couldn't prepare build directories")?;
         }
 
         self.compilation.host_deps_output = self.files_mut().host.deps().to_path_buf();

src/cargo/core/compiler/custom_build.rs

@@ -292,12 +292,8 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
         //
         // If we have an old build directory, then just move it into place,
         // otherwise create it!
-        paths::create_dir_all(&script_out_dir).chain_err(|| {
-            internal(
-                "failed to create script output directory for \
-                 build command",
-            )
-        })?;
+        paths::create_dir_all(&script_out_dir)
+            .chain_err(|| "failed to create script output directory for build command")?;
 
         // For all our native lib dependencies, pick up their metadata to pass
         // along to this custom build command. We're also careful to augment our

src/cargo/core/compiler/job_queue.rs

@@ -71,7 +71,6 @@ use super::job::{
 use super::timings::Timings;
 use super::{BuildContext, BuildPlan, CompileMode, Context, Unit};
 use crate::core::{PackageId, TargetKind};
-use crate::handle_error;
 use crate::util;
 use crate::util::diagnostic_server::{self, DiagnosticPrinter};
 use crate::util::{internal, profile, CargoResult, CargoResultExt, ProcessBuilder};
@@ -531,7 +530,7 @@ impl<'a, 'cfg> DrainState<'a, 'cfg> {
                         self.emit_warnings(Some(msg), &unit, cx)?;
 
                         if !self.active.is_empty() {
-                            handle_error(&e, &mut *cx.bcx.config.shell());
+                            crate::display_error(&e, &mut *cx.bcx.config.shell());
                             cx.bcx.config.shell().warn(
                                 "build failed, waiting for other \
                                  jobs to finish...",

src/cargo/core/compiler/mod.rs

@@ -44,7 +44,7 @@ pub use crate::core::compiler::unit::{Unit, UnitInterner};
 use crate::core::manifest::TargetSourcePath;
 use crate::core::profiles::{Lto, PanicStrategy, Profile};
 use crate::core::{Edition, Feature, InternedString, PackageId, Target};
-use crate::util::errors::{self, CargoResult, CargoResultExt, Internal, ProcessError};
+use crate::util::errors::{self, CargoResult, CargoResultExt, ProcessError, VerboseError};
 use crate::util::machine_message::Message;
 use crate::util::{self, machine_message, ProcessBuilder};
 use crate::util::{internal, join_paths, paths, profile};
@@ -262,15 +262,15 @@ fn rustc<'a, 'cfg>(
             }
         }
 
-        fn internal_if_simple_exit_code(err: Error) -> Error {
+        fn verbose_if_simple_exit_code(err: Error) -> Error {
             // If a signal on unix (`code == None`) or an abnormal termination
             // on Windows (codes like `0xC0000409`), don't hide the error details.
             match err
                 .downcast_ref::<ProcessError>()
                 .as_ref()
                 .and_then(|perr| perr.exit.and_then(|e| e.code()))
             {
-                Some(n) if errors::is_simple_exit_code(n) => Internal::new(err).into(),
+                Some(n) if errors::is_simple_exit_code(n) => VerboseError::new(err).into(),
                 _ => err,
             }
         }
@@ -288,7 +288,7 @@ fn rustc<'a, 'cfg>(
                 &mut |line| on_stdout_line(state, line, package_id, &target),
                 &mut |line| on_stderr_line(state, line, package_id, &target, &mut output_options),
             )
-            .map_err(internal_if_simple_exit_code)
+            .map_err(verbose_if_simple_exit_code)
             .chain_err(|| format!("could not compile `{}`.", name))?;
         }
 
@@ -302,8 +302,7 @@ fn rustc<'a, 'cfg>(
                     .replace(&real_name, &crate_name),
             );
             if src.exists() && src.file_name() != dst.file_name() {
-                fs::rename(&src, &dst)
-                    .chain_err(|| internal(format!("could not rename crate {:?}", src)))?;
+                fs::rename(&src, &dst).chain_err(|| format!("could not rename crate {:?}", src))?;
             }
         }
 

src/cargo/core/compiler/output_depinfo.rs

@@ -44,7 +44,7 @@ fn render_filename<P: AsRef<Path>>(path: P, basedir: Option<&str>) -> CargoResul
     };
     relpath
         .to_str()
-        .ok_or_else(|| internal("path not utf-8"))
+        .ok_or_else(|| internal(format!("path `{:?}` not utf-8", relpath)))
         .map(|f| f.replace(" ", "\\ "))
 }
 
@@ -119,7 +119,7 @@ pub fn output_depinfo<'a, 'b>(cx: &mut Context<'a, 'b>, unit: &Unit<'a>) -> Carg
                 .resolve_path(bcx.config)
                 .as_os_str()
                 .to_str()
-                .ok_or_else(|| internal("build.dep-info-basedir path not utf-8"))?
+                .ok_or_else(|| anyhow::format_err!("build.dep-info-basedir path not utf-8"))?
                 .to_string();
             Some(basedir_string.as_str())
         }

src/cargo/core/shell.rs

@@ -228,6 +228,11 @@ impl Shell {
         }
     }
 
+    /// Prints a cyan 'note' message.
+    pub fn note<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
+        self.print(&"note", Some(&message), Cyan, false)
+    }
+
     /// Updates the verbosity of the shell.
     pub fn set_verbosity(&mut self, verbosity: Verbosity) {
         self.verbosity = verbosity;

src/cargo/lib.rs

@@ -37,7 +37,7 @@ use log::debug;
 use serde::ser;
 use std::fmt;
 
-pub use crate::util::errors::Internal;
+pub use crate::util::errors::{InternalError, VerboseError};
 pub use crate::util::{CargoResult, CliError, CliResult, Config};
 
 pub const CARGO_ENV: &str = "CARGO";
@@ -106,64 +106,60 @@ pub fn exit_with_error(err: CliError, shell: &mut Shell) -> ! {
         }
     }
 
-    let CliError {
-        error,
-        exit_code,
-        unknown,
-    } = err;
-    // `exit_code` of 0 means non-fatal error (e.g., docopt version info).
-    let fatal = exit_code != 0;
-
-    let hide = unknown && shell.verbosity() != Verbose;
-
+    let CliError { error, exit_code } = err;
     if let Some(error) = error {
-        if hide {
-            drop(shell.error("An unknown error occurred"))
-        } else if fatal {
-            drop(shell.error(&error))
-        } else {
-            println!("{}", error);
-        }
-
-        if !handle_cause(&error, shell) || hide {
-            drop(writeln!(
-                shell.err(),
-                "\nTo learn more, run the command again \
-                 with --verbose."
-            ));
-        }
+        display_error(&error, shell);
     }
 
     std::process::exit(exit_code)
 }
 
-pub fn handle_error(err: &Error, shell: &mut Shell) {
-    debug!("handle_error; err={:?}", err);
-
-    let _ignored_result = shell.error(err);
-    handle_cause(err, shell);
+/// Displays an error, and all its causes, to stderr.
+pub fn display_error(err: &Error, shell: &mut Shell) {
+    debug!("display_error; err={:?}", err);
+    let has_verbose = _display_error(err, shell);
+    if has_verbose {
+        drop(writeln!(
+            shell.err(),
+            "\nTo learn more, run the command again with --verbose."
+        ));
+    }
+    if err
+        .chain()
+        .any(|e| e.downcast_ref::<InternalError>().is_some())
+    {
+        drop(shell.note("this is an unexpected cargo internal error"));
+        drop(
+            shell.note(
+                "we would appreciate a bug report: https://github.com/rust-lang/cargo/issues/",
+            ),
+        );
+        drop(shell.note(format!("{}", version())));
+        // Once backtraces are stabilized, this should print out a backtrace
+        // if it is available.
+    }
 }
 
-fn handle_cause(cargo_err: &Error, shell: &mut Shell) -> bool {
-    fn print(error: &str, shell: &mut Shell) {
-        drop(writeln!(shell.err(), "\nCaused by:"));
-        drop(writeln!(shell.err(), "  {}", error));
+fn _display_error(err: &Error, shell: &mut Shell) -> 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;
     }
-
-    let verbose = shell.verbosity();
-
-    // The first error has already been printed to the shell.
-    for err in cargo_err.chain().skip(1) {
+    drop(shell.error(&err));
+    for cause in err.chain().skip(1) {
         // If we're not in verbose mode then print remaining errors until one
-        // marked as `Internal` appears.
-        if verbose != Verbose && err.downcast_ref::<Internal>().is_some() {
-            return false;
+        // marked as `VerboseError` appears.
+        if is_verbose(cause) {
+            return true;
         }
-
-        print(&err.to_string(), shell);
+        drop(writeln!(shell.err(), "\nCaused by:"));
+        drop(writeln!(shell.err(), "  {}", cause));
     }
-
-    true
+    false
 }
 
 pub fn version() -> VersionInfo {

src/cargo/ops/cargo_install.rs

@@ -82,7 +82,7 @@ pub fn install(
             ) {
                 Ok(()) => succeeded.push(krate),
                 Err(e) => {
-                    crate::handle_error(&e, &mut opts.config.shell());
+                    crate::display_error(&e, &mut opts.config.shell());
                     failed.push(krate)
                 }
             }

src/cargo/ops/cargo_new.rs

@@ -362,6 +362,11 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
 }
 
 pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
+    // This is here just as a random location to exercise the internal error handling.
+    if std::env::var_os("__CARGO_TEST_INTERNAL_ERROR").is_some() {
+        return Err(crate::util::internal("internal error test"));
+    }
+
     let path = &opts.path;
 
     if fs::metadata(&path.join("Cargo.toml")).is_ok() {