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.

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(|| {
+        paths::create_dir_all(&script_out_dir)
-            internal(
+            .chain_err(|| "failed to create script output directory for build command")?;
-                "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,7 +262,7 @@ 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
@@ -270,7 +270,7 @@ fn rustc<'a, 'cfg>(
                 .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)
+                fs::rename(&src, &dst).chain_err(|| format!("could not rename crate {:?}", src))?;
-                    .chain_err(|| internal(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 {
+    let CliError { error, exit_code } = err;
-        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;
-
     if let Some(error) = error {
-        if hide {
+        display_error(&error, shell);
-            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."
-            ));
-        }
     }
 
     std::process::exit(exit_code)
 }
 
-pub fn handle_error(err: &Error, shell: &mut Shell) {
+/// Displays an error, and all its causes, to stderr.
-    debug!("handle_error; err={:?}", err);
+pub fn display_error(err: &Error, shell: &mut Shell) {
-
+    debug!("display_error; err={:?}", err);
-    let _ignored_result = shell.error(err);
+    let has_verbose = _display_error(err, shell);
-    handle_cause(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 _display_error(err: &Error, shell: &mut Shell) -> bool {
-    fn print(error: &str, shell: &mut Shell) {
+    let verbosity = shell.verbosity();
-        drop(writeln!(shell.err(), "\nCaused by:"));
+    let is_verbose = |e: &(dyn std::error::Error + 'static)| -> bool {
-        drop(writeln!(shell.err(), "  {}", error));
+        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;
     }
-
+    drop(shell.error(&err));
-    let verbose = shell.verbosity();
+    for cause in err.chain().skip(1) {
-
-    // The first error has already been printed to the shell.
-    for err in cargo_err.chain().skip(1) {
         // If we're not in verbose mode then print remaining errors until one
-        // marked as `Internal` appears.
+        // marked as `VerboseError` appears.
-        if verbose != Verbose && err.downcast_ref::<Internal>().is_some() {
+        if is_verbose(cause) {
-            return false;
+            return true;
         }
-
+        drop(writeln!(shell.err(), "\nCaused by:"));
-        print(&err.to_string(), shell);
+        drop(writeln!(shell.err(), "  {}", cause));
     }
-
+    false
-    true
 }
 
 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() {

src/cargo/ops/cargo_package.rs

@@ -14,7 +14,6 @@ use flate2::{Compression, GzBuilder};
 use log::debug;
 use serde_json::{self, json};
 use tar::{Archive, Builder, EntryType, Header};
-use termcolor::Color;
 
 use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
 use crate::core::resolver::ResolveOpts;
@@ -27,7 +26,7 @@ use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::paths;
 use crate::util::toml::TomlManifest;
-use crate::util::{self, internal, Config, FileLock};
+use crate::util::{self, Config, FileLock};
 
 pub struct PackageOpts<'cfg> {
     pub config: &'cfg Config,
@@ -443,17 +442,15 @@ fn tar(
             let orig = Path::new(&path).with_file_name("Cargo.toml.orig");
             header.set_path(&orig)?;
             header.set_cksum();
-            ar.append(&header, &mut file).chain_err(|| {
+            ar.append(&header, &mut file)
-                internal(format!("could not archive source file `{}`", relative_str))
+                .chain_err(|| format!("could not archive source file `{}`", relative_str))?;
-            })?;
 
             let toml = pkg.to_registry_toml(ws.config())?;
             add_generated_file(&mut ar, &path, &toml, relative_str)?;
         } else {
             header.set_cksum();
-            ar.append(&header, &mut file).chain_err(|| {
+            ar.append(&header, &mut file)
-                internal(format!("could not archive source file `{}`", relative_str))
+                .chain_err(|| format!("could not archive source file `{}`", relative_str))?;
-            })?;
         }
     }
 
@@ -581,7 +578,7 @@ fn compare_resolve(
             "package `{}` added to the packaged Cargo.lock file{}",
             pkg_id, extra
         );
-        config.shell().status_with_color("Note", msg, Color::Cyan)?;
+        config.shell().note(msg)?;
     }
     Ok(())
 }
@@ -794,6 +791,6 @@ fn add_generated_file<D: Display>(
     header.set_size(data.len() as u64);
     header.set_cksum();
     ar.append(&header, data.as_bytes())
-        .chain_err(|| internal(format!("could not archive source file `{}`", display)))?;
+        .chain_err(|| format!("could not archive source file `{}`", display))?;
     Ok(())
 }

src/cargo/ops/cargo_uninstall.rs

@@ -35,7 +35,7 @@ pub fn uninstall(
             match uninstall_one(&root, spec, bins, config) {
                 Ok(()) => succeeded.push(spec),
                 Err(e) => {
-                    crate::handle_error(&e, &mut config.shell());
+                    crate::display_error(&e, &mut config.shell());
                     failed.push(spec)
                 }
             }

src/cargo/sources/git/utils.rs

@@ -2,7 +2,7 @@ use crate::core::GitReference;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::paths;
 use crate::util::process_builder::process;
-use crate::util::{internal, network, Config, IntoUrl, Progress};
+use crate::util::{network, Config, IntoUrl, Progress};
 use curl::easy::{Easy, List};
 use git2::{self, ObjectType};
 use log::{debug, info};
@@ -358,9 +358,9 @@ impl<'a> GitCheckout<'a> {
             cargo_config: &Config,
         ) -> CargoResult<()> {
             child.init(false)?;
-            let url = child
+            let url = child.url().ok_or_else(|| {
-                .url()
+                anyhow::format_err!("non-utf8 url for submodule {:?}?", child.path())
-                .ok_or_else(|| internal("non-utf8 url for submodule"))?;
+            })?;
 
             // A submodule which is listed in .gitmodules but not actually
             // checked out will not have a head id, so we should ignore it.
@@ -393,11 +393,11 @@ impl<'a> GitCheckout<'a> {
             // Fetch data from origin and reset to the head commit
             let refspec = "refs/heads/*:refs/heads/*";
             fetch(&mut repo, url, refspec, cargo_config).chain_err(|| {
-                internal(format!(
+                format!(
                     "failed to fetch submodule `{}` from {}",
                     child.name().unwrap_or(""),
                     url
-                ))
+                )
             })?;
 
             let obj = repo.find_object(head, None)?;

src/cargo/sources/path.rs

@@ -67,7 +67,10 @@ impl<'cfg> PathSource<'cfg> {
 
         match self.packages.iter().find(|p| p.root() == &*self.path) {
             Some(pkg) => Ok(pkg.clone()),
-            None => Err(internal("no package found in source")),
+            None => Err(internal(format!(
+                "no package found in source {:?}",
+                self.path
+            ))),
         }
     }
 
@@ -208,7 +211,7 @@ impl<'cfg> PathSource<'cfg> {
         let index = repo.index()?;
         let root = repo
             .workdir()
-            .ok_or_else(|| internal("Can't list files on a bare repository."))?;
+            .ok_or_else(|| anyhow::format_err!("can't list files on a bare repository"))?;
         let pkg_path = pkg.root();
 
         let mut ret = Vec::<PathBuf>::new();
@@ -323,9 +326,10 @@ impl<'cfg> PathSource<'cfg> {
             use std::str;
             match str::from_utf8(data) {
                 Ok(s) => Ok(path.join(s)),
-                Err(..) => Err(internal(
+                Err(e) => Err(anyhow::format_err!(
-                    "cannot process path in git with a non \
+                    "cannot process path in git with a non utf8 filename: {}\n{:?}",
-                     unicode filename",
+                    e,
+                    data
                 )),
             }
         }
@@ -403,7 +407,10 @@ impl<'cfg> PathSource<'cfg> {
 
     pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> {
         if !self.updated {
-            return Err(internal("BUG: source was not updated"));
+            return Err(internal(format!(
+                "BUG: source `{:?}` was not updated",
+                self.path
+            )));
         }
 
         let mut max = FileTime::zero();

src/cargo/sources/registry/mod.rs

@@ -178,7 +178,7 @@ use crate::sources::PathSource;
 use crate::util::errors::CargoResultExt;
 use crate::util::hex;
 use crate::util::into_url::IntoUrl;
-use crate::util::{internal, CargoResult, Config, Filesystem};
+use crate::util::{CargoResult, Config, Filesystem};
 
 const PACKAGE_SOURCE_LOCK: &str = ".cargo-ok";
 pub const CRATES_IO_INDEX: &str = "https://github.com/rust-lang/crates.io-index";
@@ -511,7 +511,7 @@ impl<'cfg> RegistrySource<'cfg> {
     fn get_pkg(&mut self, package: PackageId, path: &File) -> CargoResult<Package> {
         let path = self
             .unpack_package(package, path)
-            .chain_err(|| internal(format!("failed to unpack package `{}`", package)))?;
+            .chain_err(|| format!("failed to unpack package `{}`", package))?;
         let mut src = PathSource::new(&path, self.source_id, self.config);
         src.update()?;
         let mut pkg = match src.download(package)? {

src/cargo/util/config/mod.rs

@@ -73,7 +73,7 @@ use self::ConfigValue as CV;
 use crate::core::shell::Verbosity;
 use crate::core::{nightly_features_allowed, CliUnstable, Shell, SourceId, Workspace};
 use crate::ops;
-use crate::util::errors::{internal, CargoResult, CargoResultExt};
+use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::toml as cargo_toml;
 use crate::util::{paths, validate_package_name};
 use crate::util::{FileLock, Filesystem, IntoUrl, IntoUrlWithBase, Rustc};
@@ -1439,13 +1439,13 @@ impl ConfigValue {
             | (expected @ &mut CV::Table(_, _), found)
             | (expected, found @ CV::List(_, _))
             | (expected, found @ CV::Table(_, _)) => {
-                return Err(internal(format!(
+                return Err(anyhow!(
                     "failed to merge config value from `{}` into `{}`: expected {}, but found {}",
                     found.definition(),
                     expected.definition(),
                     expected.desc(),
                     found.desc()
-                )));
+                ));
             }
             (old, mut new) => {
                 if force || new.definition().is_higher_priority(old.definition()) {

src/cargo/util/errors.rs

@@ -49,34 +49,81 @@ impl fmt::Display for HttpNot200 {
 
 impl std::error::Error for HttpNot200 {}
 
-pub struct Internal {
+// =============================================================================
+// Verbose error
+
+/// An error wrapper for errors that should only be displayed with `--verbose`.
+///
+/// This should only be used in rare cases. When emitting this error, you
+/// should have a normal error higher up the error-cause chain (like "could
+/// not compile `foo`"), so at least *something* gets printed without
+/// `--verbose`.
+pub struct VerboseError {
     inner: Error,
 }
 
-impl Internal {
+impl VerboseError {
-    pub fn new(inner: Error) -> Internal {
+    pub fn new(inner: Error) -> VerboseError {
-        Internal { inner }
+        VerboseError { inner }
     }
 }
 
-impl std::error::Error for Internal {
+impl std::error::Error for VerboseError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         self.inner.source()
     }
 }
 
-impl fmt::Debug for Internal {
+impl fmt::Debug for VerboseError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         self.inner.fmt(f)
     }
 }
 
-impl fmt::Display for Internal {
+impl fmt::Display for VerboseError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         self.inner.fmt(f)
     }
 }
 
+// =============================================================================
+// Internal error
+
+/// An unexpected, internal error.
+///
+/// This should only be used for unexpected errors. It prints a message asking
+/// the user to file a bug report.
+pub struct InternalError {
+    inner: Error,
+}
+
+impl InternalError {
+    pub fn new(inner: Error) -> InternalError {
+        InternalError { inner }
+    }
+}
+
+impl std::error::Error for InternalError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.inner.source()
+    }
+}
+
+impl fmt::Debug for InternalError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.inner.fmt(f)
+    }
+}
+
+impl fmt::Display for InternalError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.inner.fmt(f)
+    }
+}
+
+// =============================================================================
+// Manifest error
+
 /// Error wrapper related to a particular manifest and providing it's path.
 ///
 /// This error adds no displayable info of it's own.
@@ -143,8 +190,15 @@ impl<'a> ::std::iter::FusedIterator for ManifestCauses<'a> {}
 // Process errors
 #[derive(Debug)]
 pub struct ProcessError {
+    /// A detailed description to show to the user why the process failed.
     pub desc: String,
+    /// The exit status of the process.
+    ///
+    /// This can be `None` if the process failed to launch (like process not found).
     pub exit: Option<ExitStatus>,
+    /// The output from the process.
+    ///
+    /// This can be `None` if the process failed to launch, or the output was not captured.
     pub output: Option<Output>,
 }
 
@@ -247,22 +301,27 @@ impl CargoTestError {
 pub type CliResult = Result<(), CliError>;
 
 #[derive(Debug)]
+/// The CLI error is the error type used at Cargo's CLI-layer.
+///
+/// All errors from the lib side of Cargo will get wrapped with this error.
+/// Other errors (such as command-line argument validation) will create this
+/// directly.
 pub struct CliError {
+    /// The error to display. This can be `None` in rare cases to exit with a
+    /// code without displaying a message. For example `cargo run -q` where
+    /// the resulting process exits with a nonzero code (on Windows), or an
+    /// external subcommand that exits nonzero (we assume it printed its own
+    /// message).
     pub error: Option<anyhow::Error>,
-    pub unknown: bool,
+    /// The process exit code.
     pub exit_code: i32,
 }
 
 impl CliError {
     pub fn new(error: anyhow::Error, code: i32) -> CliError {
-        // Specifically deref the error to a standard library error to only
-        // check the top-level error to see if it's an `Internal`, we don't want
-        // `anyhow::Error`'s behavior of recursively checking.
-        let unknown = (&*error).downcast_ref::<Internal>().is_some();
         CliError {
             error: Some(error),
             exit_code: code,
-            unknown,
         }
     }
 
@@ -270,7 +329,6 @@ impl CliError {
         CliError {
             error: None,
             exit_code: code,
-            unknown: false,
         }
     }
 }
@@ -291,6 +349,10 @@ impl From<clap::Error> for CliError {
 // =============================================================================
 // Construction helpers
 
+/// Creates a new process error.
+///
+/// `status` can be `None` if the process did not launch.
+/// `output` can be `None` if the process did not launch, or output was not captured.
 pub fn process_error(
     msg: &str,
     status: Option<ExitStatus>,
@@ -412,9 +474,5 @@ pub fn is_simple_exit_code(code: i32) -> bool {
 }
 
 pub fn internal<S: fmt::Display>(error: S) -> anyhow::Error {
-    _internal(&error)
+    InternalError::new(anyhow::format_err!("{}", error)).into()
-}
-
-fn _internal(error: &dyn fmt::Display) -> anyhow::Error {
-    Internal::new(anyhow::format_err!("{}", error)).into()
 }

src/cargo/util/paths.rs

@@ -8,26 +8,21 @@ use std::path::{Component, Path, PathBuf};
 
 use filetime::FileTime;
 
-use crate::util::errors::{CargoResult, CargoResultExt, Internal};
+use crate::util::errors::{CargoResult, CargoResultExt};
 
 pub fn join_paths<T: AsRef<OsStr>>(paths: &[T], env: &str) -> CargoResult<OsString> {
-    let err = match env::join_paths(paths.iter()) {
+    env::join_paths(paths.iter())
-        Ok(paths) => return Ok(paths),
+        .chain_err(|| {
-        Err(e) => e,
+            let paths = paths.iter().map(Path::new).collect::<Vec<_>>();
-    };
+            format!("failed to join path array: {:?}", paths)
-    let paths = paths.iter().map(Path::new).collect::<Vec<_>>();
+        })
-    let err = anyhow::Error::from(err);
+        .chain_err(|| {
-    let explain = Internal::new(anyhow::format_err!(
+            format!(
-        "failed to join path array: {:?}",
+                "failed to join search paths together\n\
-        paths
+                     Does ${} have an unterminated quote character?",
-    ));
+                env
-    let err = anyhow::Error::from(err.context(explain));
+            )
-    let more_explain = format!(
+        })
-        "failed to join search paths together\n\
-         Does ${} have an unterminated quote character?",
-        env
-    );
-    Err(err.context(more_explain).into())
 }
 
 pub fn dylib_path_envvar() -> &'static str {

src/cargo/util/rustc.rs

@@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::core::InternedString;
 use crate::util::paths;
-use crate::util::{self, internal, profile, CargoResult, ProcessBuilder};
+use crate::util::{self, profile, CargoResult, CargoResultExt, ProcessBuilder};
 
 /// Information on the `rustc` executable
 #[derive(Debug)]
@@ -186,9 +186,11 @@ impl Cache {
                 debug!("running {}", cmd);
                 let output = cmd.exec_with_output()?;
                 let stdout = String::from_utf8(output.stdout)
-                    .map_err(|_| internal("rustc didn't return utf8 output"))?;
+                    .map_err(|e| anyhow::anyhow!("{}: {:?}", e, e.as_bytes()))
+                    .chain_err(|| anyhow::anyhow!("`{}` didn't return utf8 output", cmd))?;
                 let stderr = String::from_utf8(output.stderr)
-                    .map_err(|_| internal("rustc didn't return utf8 output"))?;
+                    .map_err(|e| anyhow::anyhow!("{}: {:?}", e, e.as_bytes()))
+                    .chain_err(|| anyhow::anyhow!("`{}` didn't return utf8 output", cmd))?;
                 let output = (stdout, stderr);
                 entry.insert(output.clone());
                 self.dirty = true;

tests/testsuite/error.rs

@@ -0,0 +1,19 @@
+//! General error tests that don't belong anywhere else.
+
+use cargo_test_support::cargo_process;
+
+#[cargo_test]
+fn internal_error() {
+    cargo_process("init")
+        .env("__CARGO_TEST_INTERNAL_ERROR", "1")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] internal error test
+[NOTE] this is an unexpected cargo internal error
+[NOTE] we would appreciate a bug report: https://github.com/rust-lang/cargo/issues/
+[NOTE] cargo [..]
+",
+        )
+        .run();
+}

tests/testsuite/main.rs

@@ -42,6 +42,7 @@ mod dep_info;
 mod directory;
 mod doc;
 mod edition;
+mod error;
 mod features;
 mod fetch;
 mod fix;