From fb27a34b9b9180dcb45486cecc2b55fe8d637dd1 Mon Sep 17 00:00:00 2001 From: Kornel Date: Sat, 4 Dec 2021 16:53:38 +0000 Subject: [PATCH] Avoid downcasting to ProcessError #10160 --- .../cargo-test-support/src/cross_compile.rs | 7 +-- crates/cargo-test-support/src/lib.rs | 35 ++++++------ crates/cargo-util/src/process_builder.rs | 53 +++++++++++-------- crates/cargo-util/src/process_error.rs | 20 ++++++- src/bin/cargo/cli.rs | 6 ++- src/bin/cargo/main.rs | 10 ++-- src/cargo/core/compiler/mod.rs | 18 +++---- src/cargo/ops/cargo_run.rs | 2 +- src/cargo/ops/cargo_test.rs | 2 - tests/testsuite/messages.rs | 3 +- tests/testsuite/old_cargos.rs | 11 ++-- 11 files changed, 92 insertions(+), 75 deletions(-) diff --git a/crates/cargo-test-support/src/cross_compile.rs b/crates/cargo-test-support/src/cross_compile.rs index eda21107703..34fb2273317 100644 --- a/crates/cargo-test-support/src/cross_compile.rs +++ b/crates/cargo-test-support/src/cross_compile.rs @@ -40,7 +40,7 @@ pub fn disabled() -> bool { let cross_target = alternate(); - let run_cross_test = || -> anyhow::Result { + let run_cross_test = || -> Result { let p = project() .at("cross_test") .file("Cargo.toml", &basic_manifest("cross_test", "1.0.0")) @@ -170,10 +170,7 @@ rustup does not appear to be installed. Make sure that the appropriate // Show the actual error message. match run_cross_test() { Ok(_) => message.push_str("\nUh oh, second run succeeded?\n"), - Err(err) => match err.downcast_ref::() { - Some(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(), - None => write!(message, "\nUnexpected non-process error: {}\n", err).unwrap(), - }, + Err(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(), } panic!("{}", message); diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index a4c4153bad2..c19b9705a26 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -763,7 +763,7 @@ impl Execs { self } - pub fn exec_with_output(&mut self) -> Result { + pub fn exec_with_output(&mut self) -> Result { self.ran = true; // TODO avoid unwrap let p = (&self.process_builder).clone().unwrap(); @@ -879,28 +879,27 @@ impl Execs { match res { Ok(out) => { self.match_output(out.status.code(), &out.stdout, &out.stderr)?; - return Ok(RawOutput { + Ok(RawOutput { stdout: out.stdout, stderr: out.stderr, code: out.status.code(), - }); + }) } - Err(e) => { - if let Some(ProcessError { - stdout: Some(stdout), - stderr: Some(stderr), + Err(ProcessError { + stdout: Some(stdout), + stderr: Some(stderr), + code, + .. + }) => { + self.match_output(code, &stdout, &stderr)?; + Ok(RawOutput { + stdout: stdout.to_vec(), + stderr: stderr.to_vec(), code, - .. - }) = e.downcast_ref::() - { - self.match_output(*code, stdout, stderr)?; - return Ok(RawOutput { - stdout: stdout.to_vec(), - stderr: stderr.to_vec(), - code: *code, - }); - } - bail!("could not exec process {}: {:?}", process, e) + }) + } + Err(e) => { + bail!("could not exec process {}: {:?}", process, e); } } } diff --git a/crates/cargo-util/src/process_builder.rs b/crates/cargo-util/src/process_builder.rs index 9c43d3b2f03..2396d7bfc1a 100644 --- a/crates/cargo-util/src/process_builder.rs +++ b/crates/cargo-util/src/process_builder.rs @@ -1,6 +1,6 @@ use crate::process_error::ProcessError; use crate::read2; -use anyhow::{bail, Context, Result}; +use anyhow::Result; use jobserver::Client; use shell_escape::escape; use std::collections::BTreeMap; @@ -162,10 +162,11 @@ impl ProcessBuilder { } /// Runs the process, waiting for completion, and mapping non-success exit codes to an error. - pub fn exec(&self) -> Result<()> { + pub fn exec(&self) -> Result<(), ProcessError> { let mut command = self.build_command(); - let exit = command.status().with_context(|| { + let exit = command.status().map_err(|err| { ProcessError::new(&format!("could not execute process {}", self), None, None) + .with_source(err) })?; if exit.success() { @@ -175,8 +176,7 @@ impl ProcessBuilder { &format!("process didn't exit successfully: {}", self), Some(exit), None, - ) - .into()) + )) } } @@ -195,16 +195,17 @@ impl ProcessBuilder { /// include our child process. If the child terminates then we'll reap them in Cargo /// pretty quickly, and if the child handles the signal then we won't terminate /// (and we shouldn't!) until the process itself later exits. - pub fn exec_replace(&self) -> Result<()> { + pub fn exec_replace(&self) -> Result<(), ProcessError> { imp::exec_replace(self) } /// Executes the process, returning the stdio output, or an error if non-zero exit status. - pub fn exec_with_output(&self) -> Result { + pub fn exec_with_output(&self) -> Result { let mut command = self.build_command(); - let output = command.output().with_context(|| { + let output = command.output().map_err(|e| { ProcessError::new(&format!("could not execute process {}", self), None, None) + .with_source(e) })?; if output.status.success() { @@ -214,8 +215,7 @@ impl ProcessBuilder { &format!("process didn't exit successfully: {}", self), Some(output.status), Some(&output), - ) - .into()) + )) } } @@ -233,7 +233,7 @@ impl ProcessBuilder { on_stdout_line: &mut dyn FnMut(&str) -> Result<()>, on_stderr_line: &mut dyn FnMut(&str) -> Result<()>, capture_output: bool, - ) -> Result { + ) -> Result { let mut stdout = Vec::new(); let mut stderr = Vec::new(); @@ -245,7 +245,7 @@ impl ProcessBuilder { let mut callback_error = None; let mut stdout_pos = 0; let mut stderr_pos = 0; - let status = (|| { + let res = (|| { let mut child = cmd.spawn()?; let out = child.stdout.take().unwrap(); let err = child.stderr.take().unwrap(); @@ -293,9 +293,11 @@ impl ProcessBuilder { *pos = 0; })?; child.wait() - })() - .with_context(|| { + })(); + + let status = res.map_err(|err| { ProcessError::new(&format!("could not execute process {}", self), None, None) + .with_source(err) })?; let output = Output { status, @@ -306,14 +308,14 @@ impl ProcessBuilder { { let to_print = if capture_output { Some(&output) } else { None }; if let Some(e) = callback_error { - let cx = ProcessError::new( + return Err(ProcessError::new( &format!("failed to parse process output: {}", self), Some(output.status), to_print, - ); - bail!(anyhow::Error::new(cx).context(e)); + ) + .with_source(e)); } else if !output.status.success() { - bail!(ProcessError::new( + return Err(ProcessError::new( &format!("process didn't exit successfully: {}", self), Some(output.status), to_print, @@ -388,14 +390,15 @@ mod imp { use anyhow::Result; use std::os::unix::process::CommandExt; - pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> { + pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> { let mut command = process_builder.build_command(); let error = command.exec(); - Err(anyhow::Error::from(error).context(ProcessError::new( + Err(ProcessError::new( &format!("could not execute process {}", process_builder), None, None, - ))) + ) + .with_source(error)) } } @@ -411,10 +414,14 @@ mod imp { TRUE } - pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> { + pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> { unsafe { if SetConsoleCtrlHandler(Some(ctrlc_handler), TRUE) == FALSE { - return Err(ProcessError::new("Could not set Ctrl-C handler.", None, None).into()); + return Err(ProcessError::new( + "Could not set Ctrl-C handler.", + None, + None, + )); } } diff --git a/crates/cargo-util/src/process_error.rs b/crates/cargo-util/src/process_error.rs index 57feffbef67..9d6cf980511 100644 --- a/crates/cargo-util/src/process_error.rs +++ b/crates/cargo-util/src/process_error.rs @@ -27,6 +27,8 @@ pub struct ProcessError { /// This can be `None` if the process failed to launch, or the output was /// not captured. pub stderr: Option>, + + pub source: Option>, } impl fmt::Display for ProcessError { @@ -35,7 +37,14 @@ impl fmt::Display for ProcessError { } } -impl std::error::Error for ProcessError {} +impl std::error::Error for ProcessError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self.source.as_ref() { + Some(s) => Some(&**s), + None => None, + } + } +} impl ProcessError { /// Creates a new [`ProcessError`]. @@ -93,8 +102,17 @@ impl ProcessError { code, stdout: stdout.map(|s| s.to_vec()), stderr: stderr.map(|s| s.to_vec()), + source: None, } } + + pub(crate) fn with_source( + mut self, + source: impl Into>, + ) -> Self { + self.source = Some(source.into()); + self + } } /// Converts an [`ExitStatus`] to a human-readable string suitable for diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index d4da7276b25..e0999a8372e 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -110,7 +110,11 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'", if let Some(code) = expanded_args.value_of("explain") { let mut procss = config.load_global_rustc(None)?.process(); - procss.arg("--explain").arg(code).exec()?; + procss + .arg("--explain") + .arg(code) + .exec() + .map_err(anyhow::Error::new)?; return Ok(()); } diff --git a/src/bin/cargo/main.rs b/src/bin/cargo/main.rs index 57895b766ec..f751ddd7e42 100644 --- a/src/bin/cargo/main.rs +++ b/src/bin/cargo/main.rs @@ -7,7 +7,7 @@ use cargo::core::shell::Shell; use cargo::util::toml::StringOrVec; use cargo::util::CliError; use cargo::util::{self, closest_msg, command_prelude, CargoResult, CliResult, Config}; -use cargo_util::{ProcessBuilder, ProcessError}; +use cargo_util::ProcessBuilder; use std::collections::BTreeMap; use std::env; use std::fs; @@ -177,12 +177,10 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> Cli Err(e) => e, }; - if let Some(perr) = err.downcast_ref::() { - if let Some(code) = perr.code { - return Err(CliError::code(code)); - } + if let Some(code) = err.code { + return Err(CliError::code(code)); } - Err(CliError::new(err, 101)) + Err(CliError::new(anyhow::Error::new(err), 101)) } #[cfg(unix)] diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index cb9b3c681ab..63526e3b36d 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -107,7 +107,7 @@ pub trait Executor: Send + Sync + 'static { mode: CompileMode, on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>, on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>, - ) -> CargoResult<()>; + ) -> Result<(), ProcessError>; /// Queried when queuing each unit of work. If it returns true, then the /// unit will always be rebuilt, independent of whether it needs to be. @@ -130,7 +130,7 @@ impl Executor for DefaultExecutor { _mode: CompileMode, on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>, on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>, - ) -> CargoResult<()> { + ) -> Result<(), ProcessError> { cmd.exec_with_streaming(on_stdout_line, on_stderr_line, false) .map(drop) } @@ -298,16 +298,14 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> Car } } - fn verbose_if_simple_exit_code(err: Error) -> Error { + fn verbose_if_simple_exit_code(err: ProcessError) -> 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::() - .as_ref() - .and_then(|perr| perr.code) - { - Some(n) if cargo_util::is_simple_exit_code(n) => VerboseError::new(err).into(), - _ => err, + match err.code { + Some(n) if cargo_util::is_simple_exit_code(n) => { + VerboseError::new(anyhow::Error::new(err)).into() + } + _ => anyhow::Error::new(err), } } diff --git a/src/cargo/ops/cargo_run.rs b/src/cargo/ops/cargo_run.rs index 69bae2c5912..bcb2ba429f2 100644 --- a/src/cargo/ops/cargo_run.rs +++ b/src/cargo/ops/cargo_run.rs @@ -97,5 +97,5 @@ pub fn run( config.shell().status("Running", process.to_string())?; - process.exec_replace() + process.exec_replace().map_err(anyhow::Error::new) } diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index c461c93a6a9..479c6c044d0 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/src/cargo/ops/cargo_test.rs @@ -116,7 +116,6 @@ fn run_unit_tests( let result = cmd.exec(); if let Err(e) = result { - let e = e.downcast::()?; errors.push(( unit.target.kind().clone(), unit.target.name().to_string(), @@ -255,7 +254,6 @@ fn run_doc_tests( .shell() .verbose(|shell| shell.status("Running", p.to_string()))?; if let Err(e) = p.exec() { - let e = e.downcast::()?; errors.push(e); if !options.no_fail_fast { return Ok((Test::Doc, errors)); diff --git a/tests/testsuite/messages.rs b/tests/testsuite/messages.rs index 0c55e15fbf7..45f6872bacc 100644 --- a/tests/testsuite/messages.rs +++ b/tests/testsuite/messages.rs @@ -3,7 +3,6 @@ //! Tests for message caching can be found in `cache_messages`. use cargo_test_support::{process, project, Project}; -use cargo_util::ProcessError; /// Captures the actual diagnostics displayed by rustc. This is done to avoid /// relying on the exact message formatting in rustc. @@ -22,7 +21,7 @@ pub fn raw_rustc_output(project: &Project, path: &str, extra: &[&str]) -> String .exec_with_output() { Ok(output) => output.stderr, - Err(e) => e.downcast::().unwrap().stderr.unwrap(), + Err(e) => e.stderr.unwrap(), }; // Do a little dance to remove rustc's "warnings emitted" message and the subsequent newline. let stderr = std::str::from_utf8(&rustc_output).expect("utf8"); diff --git a/tests/testsuite/old_cargos.rs b/tests/testsuite/old_cargos.rs index 10179bc2b9b..bfca99f1e1d 100644 --- a/tests/testsuite/old_cargos.rs +++ b/tests/testsuite/old_cargos.rs @@ -10,7 +10,6 @@ //! cargo test --test testsuite -- old_cargos --nocapture --ignored //! ``` -use cargo::CargoResult; use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::{self, Dependency, Package}; use cargo_test_support::{cargo_exe, execs, paths, process, project, rustc_host}; @@ -345,7 +344,7 @@ fn new_features() { }; // Runs `cargo build` and returns the versions selected in the lock. - let run_cargo = || -> CargoResult { + let run_cargo = || -> Result { match tc_process("cargo", toolchain) .args(&["build", "--verbose"]) .cwd(p.root()) @@ -387,13 +386,13 @@ fn new_features() { }; } - let check_err_contains = |tc_result: &mut Vec<_>, err: anyhow::Error, contents| { - if let Some(ProcessError { + let check_err_contains = |tc_result: &mut Vec<_>, err: ProcessError, contents| { + if let ProcessError { stderr: Some(stderr), .. - }) = err.downcast_ref::() + } = err { - let stderr = std::str::from_utf8(stderr).unwrap(); + let stderr = std::str::from_utf8(&stderr).unwrap(); if !stderr.contains(contents) { tc_result.push(format!( "{} expected to see error contents:\n{}\nbut saw:\n{}",