From 0d44a8267bf503645ceb5f5853abdd4cce407dc8 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Feb 2020 18:46:48 -0800 Subject: [PATCH] Rework internal errors. --- crates/cargo-test-support/src/lib.rs | 2 +- src/cargo/core/compiler/context/mod.rs | 6 +- src/cargo/core/compiler/custom_build.rs | 8 +- src/cargo/core/compiler/job_queue.rs | 3 +- src/cargo/core/compiler/mod.rs | 11 ++- src/cargo/core/compiler/output_depinfo.rs | 4 +- src/cargo/core/shell.rs | 5 ++ src/cargo/lib.rs | 90 ++++++++++----------- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/cargo_new.rs | 5 ++ src/cargo/ops/cargo_package.rs | 17 ++-- src/cargo/ops/cargo_uninstall.rs | 2 +- src/cargo/sources/git/utils.rs | 12 +-- src/cargo/sources/path.rs | 19 +++-- src/cargo/sources/registry/mod.rs | 4 +- src/cargo/util/config/mod.rs | 6 +- src/cargo/util/errors.rs | 96 ++++++++++++++++++----- src/cargo/util/paths.rs | 31 +++----- src/cargo/util/rustc.rs | 8 +- tests/testsuite/error.rs | 19 +++++ tests/testsuite/main.rs | 1 + 21 files changed, 215 insertions(+), 136 deletions(-) create mode 100644 tests/testsuite/error.rs diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index b17e40ec9..b9618d6b1 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/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 ¯os { diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 6597c9f96..197dcaca1 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/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(); diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 6f2314221..be03e05b5 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/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 diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 57b515992..60ecaa317 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/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...", diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 50795b679..95cf32f54 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/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) - .chain_err(|| internal(format!("could not rename crate {:?}", src)))?; + fs::rename(&src, &dst).chain_err(|| format!("could not rename crate {:?}", src))?; } } diff --git a/src/cargo/core/compiler/output_depinfo.rs b/src/cargo/core/compiler/output_depinfo.rs index 6f50c1d24..44375e7a9 100644 --- a/src/cargo/core/compiler/output_depinfo.rs +++ b/src/cargo/core/compiler/output_depinfo.rs @@ -44,7 +44,7 @@ fn render_filename>(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()) } diff --git a/src/cargo/core/shell.rs b/src/cargo/core/shell.rs index 30033285e..3310c3424 100644 --- a/src/cargo/core/shell.rs +++ b/src/cargo/core/shell.rs @@ -228,6 +228,11 @@ impl Shell { } } + /// Prints a cyan 'note' message. + pub fn note(&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; diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 492e34ee4..1281619b2 100644 --- a/src/cargo/lib.rs +++ b/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::().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::().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::().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 { diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 505b940e8..0136486e0 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/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) } } diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 6bf57ce89..dd892d5a1 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/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() { diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index a8b9bf2a8..5bfc27238 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/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( 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(()) } diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index 6532123b9..2d69eabd0 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/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) } } diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 23a41e4f0..8fa8b7dd0 100644 --- a/src/cargo/sources/git/utils.rs +++ b/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)?; diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index aa5175b3d..b3c92ac43 100644 --- a/src/cargo/sources/path.rs +++ b/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::::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( - "cannot process path in git with a non \ - unicode filename", + Err(e) => Err(anyhow::format_err!( + "cannot process path in git with a non utf8 filename: {}\n{:?}", + 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(); diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 566229027..794273b30 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/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 { 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)? { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 73902f6ca..9b5eeef71 100644 --- a/src/cargo/util/config/mod.rs +++ b/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()) { diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index c6ccfb34f..a65a3d37b 100644 --- a/src/cargo/util/errors.rs +++ b/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 { - pub fn new(inner: Error) -> Internal { - Internal { inner } +impl VerboseError { + pub fn new(inner: Error) -> VerboseError { + 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, + /// The output from the process. + /// + /// This can be `None` if the process failed to launch, or the output was not captured. pub output: Option, } @@ -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, - 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::().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 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, @@ -412,9 +474,5 @@ pub fn is_simple_exit_code(code: i32) -> bool { } pub fn internal(error: S) -> anyhow::Error { - _internal(&error) -} - -fn _internal(error: &dyn fmt::Display) -> anyhow::Error { - Internal::new(anyhow::format_err!("{}", error)).into() + InternalError::new(anyhow::format_err!("{}", error)).into() } diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index 10f4cad9c..95e55e6ec 100644 --- a/src/cargo/util/paths.rs +++ b/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>(paths: &[T], env: &str) -> CargoResult { - let err = match env::join_paths(paths.iter()) { - Ok(paths) => return Ok(paths), - Err(e) => e, - }; - let paths = paths.iter().map(Path::new).collect::>(); - let err = anyhow::Error::from(err); - let explain = Internal::new(anyhow::format_err!( - "failed to join path array: {:?}", - paths - )); - 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()) + env::join_paths(paths.iter()) + .chain_err(|| { + let paths = paths.iter().map(Path::new).collect::>(); + format!("failed to join path array: {:?}", paths) + }) + .chain_err(|| { + format!( + "failed to join search paths together\n\ + Does ${} have an unterminated quote character?", + env + ) + }) } pub fn dylib_path_envvar() -> &'static str { diff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs index d17c17af7..bb54119b9 100644 --- a/src/cargo/util/rustc.rs +++ b/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; diff --git a/tests/testsuite/error.rs b/tests/testsuite/error.rs new file mode 100644 index 000000000..410902c21 --- /dev/null +++ b/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(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index bbea3cc02..fa22a1e76 100644 --- a/tests/testsuite/main.rs +++ b/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;