Rework internal errors.

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() {

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(|| {
-                internal(format!("could not archive source file `{}`", relative_str))
-            })?;
+            ar.append(&header, &mut file)
+                .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(|| {
-                internal(format!("could not archive source file `{}`", relative_str))
-            })?;
+            ar.append(&header, &mut file)
+                .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
-                .url()
-                .ok_or_else(|| internal("non-utf8 url for submodule"))?;
+            let url = child.url().ok_or_else(|| {
+                anyhow::format_err!("non-utf8 url for submodule {:?}?", child.path())
+            })?;
 
             // 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)?;