Skip to content

Commit fb27a34

Browse files
kornelskikornelski
committed
Avoid downcasting to ProcessError
#10160
1 parent 0507d9f commit fb27a34

File tree

11 files changed

+92
-75
lines changed

11 files changed

+92
-75
lines changed

‎crates/cargo-test-support/src/cross_compile.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ pub fn disabled() -> bool {
4040

4141
let cross_target = alternate();
4242

43-
let run_cross_test = || -> anyhow::Result<Output> {
43+
let run_cross_test = || -> Result<Output, ProcessError> {
4444
let p = project()
4545
.at("cross_test")
4646
.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
170170
// Show the actual error message.
171171
match run_cross_test() {
172172
Ok(_) => message.push_str("\nUh oh, second run succeeded?\n"),
173-
Err(err) => match err.downcast_ref::<ProcessError>() {
174-
Some(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(),
175-
None => write!(message, "\nUnexpected non-process error: {}\n", err).unwrap(),
176-
},
173+
Err(proc_err) => write!(message, "\nTest error: {}\n", proc_err).unwrap(),
177174
}
178175

179176
panic!("{}", message);

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

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,7 @@ impl Execs {
763763
self
764764
}
765765

766-
pub fn exec_with_output(&mut self) -> Result<Output> {
766+
pub fn exec_with_output(&mut self) -> Result<Output, ProcessError> {
767767
self.ran = true;
768768
// TODO avoid unwrap
769769
let p = (&self.process_builder).clone().unwrap();
@@ -879,28 +879,27 @@ impl Execs {
879879
match res {
880880
Ok(out) => {
881881
self.match_output(out.status.code(), &out.stdout, &out.stderr)?;
882-
return Ok(RawOutput {
882+
Ok(RawOutput {
883883
stdout: out.stdout,
884884
stderr: out.stderr,
885885
code: out.status.code(),
886-
});
886+
})
887887
}
888-
Err(e) => {
889-
if let Some(ProcessError {
890-
stdout: Some(stdout),
891-
stderr: Some(stderr),
888+
Err(ProcessError {
889+
stdout: Some(stdout),
890+
stderr: Some(stderr),
891+
code,
892+
..
893+
}) => {
894+
self.match_output(code, &stdout, &stderr)?;
895+
Ok(RawOutput {
896+
stdout: stdout.to_vec(),
897+
stderr: stderr.to_vec(),
892898
code,
893-
..
894-
}) = e.downcast_ref::<ProcessError>()
895-
{
896-
self.match_output(*code, stdout, stderr)?;
897-
return Ok(RawOutput {
898-
stdout: stdout.to_vec(),
899-
stderr: stderr.to_vec(),
900-
code: *code,
901-
});
902-
}
903-
bail!("could not exec process {}: {:?}", process, e)
899+
})
900+
}
901+
Err(e) => {
902+
bail!("could not exec process {}: {:?}", process, e);
904903
}
905904
}
906905
}

‎crates/cargo-util/src/process_builder.rs

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::process_error::ProcessError;
22
use crate::read2;
3-
use anyhow::{bail, Context, Result};
3+
use anyhow::Result;
44
use jobserver::Client;
55
use shell_escape::escape;
66
use std::collections::BTreeMap;
@@ -162,10 +162,11 @@ impl ProcessBuilder {
162162
}
163163

164164
/// Runs the process, waiting for completion, and mapping non-success exit codes to an error.
165-
pub fn exec(&self) -> Result<()> {
165+
pub fn exec(&self) -> Result<(), ProcessError> {
166166
let mut command = self.build_command();
167-
let exit = command.status().with_context(|| {
167+
let exit = command.status().map_err(|err| {
168168
ProcessError::new(&format!("could not execute process {}", self), None, None)
169+
.with_source(err)
169170
})?;
170171

171172
if exit.success() {
@@ -175,8 +176,7 @@ impl ProcessBuilder {
175176
&format!("process didn't exit successfully: {}", self),
176177
Some(exit),
177178
None,
178-
)
179-
.into())
179+
))
180180
}
181181
}
182182

@@ -195,16 +195,17 @@ impl ProcessBuilder {
195195
/// include our child process. If the child terminates then we'll reap them in Cargo
196196
/// pretty quickly, and if the child handles the signal then we won't terminate
197197
/// (and we shouldn't!) until the process itself later exits.
198-
pub fn exec_replace(&self) -> Result<()> {
198+
pub fn exec_replace(&self) -> Result<(), ProcessError> {
199199
imp::exec_replace(self)
200200
}
201201

202202
/// Executes the process, returning the stdio output, or an error if non-zero exit status.
203-
pub fn exec_with_output(&self) -> Result<Output> {
203+
pub fn exec_with_output(&self) -> Result<Output, ProcessError> {
204204
let mut command = self.build_command();
205205

206-
let output = command.output().with_context(|| {
206+
let output = command.output().map_err(|e| {
207207
ProcessError::new(&format!("could not execute process {}", self), None, None)
208+
.with_source(e)
208209
})?;
209210

210211
if output.status.success() {
@@ -214,8 +215,7 @@ impl ProcessBuilder {
214215
&format!("process didn't exit successfully: {}", self),
215216
Some(output.status),
216217
Some(&output),
217-
)
218-
.into())
218+
))
219219
}
220220
}
221221

@@ -233,7 +233,7 @@ impl ProcessBuilder {
233233
on_stdout_line: &mut dyn FnMut(&str) -> Result<()>,
234234
on_stderr_line: &mut dyn FnMut(&str) -> Result<()>,
235235
capture_output: bool,
236-
) -> Result<Output> {
236+
) -> Result<Output, ProcessError> {
237237
let mut stdout = Vec::new();
238238
let mut stderr = Vec::new();
239239

@@ -245,7 +245,7 @@ impl ProcessBuilder {
245245
let mut callback_error = None;
246246
let mut stdout_pos = 0;
247247
let mut stderr_pos = 0;
248-
let status = (|| {
248+
let res = (|| {
249249
let mut child = cmd.spawn()?;
250250
let out = child.stdout.take().unwrap();
251251
let err = child.stderr.take().unwrap();
@@ -293,9 +293,11 @@ impl ProcessBuilder {
293293
*pos = 0;
294294
})?;
295295
child.wait()
296-
})()
297-
.with_context(|| {
296+
})();
297+
298+
let status = res.map_err(|err| {
298299
ProcessError::new(&format!("could not execute process {}", self), None, None)
300+
.with_source(err)
299301
})?;
300302
let output = Output {
301303
status,
@@ -306,14 +308,14 @@ impl ProcessBuilder {
306308
{
307309
let to_print = if capture_output { Some(&output) } else { None };
308310
if let Some(e) = callback_error {
309-
let cx = ProcessError::new(
311+
return Err(ProcessError::new(
310312
&format!("failed to parse process output: {}", self),
311313
Some(output.status),
312314
to_print,
313-
);
314-
bail!(anyhow::Error::new(cx).context(e));
315+
)
316+
.with_source(e));
315317
} else if !output.status.success() {
316-
bail!(ProcessError::new(
318+
return Err(ProcessError::new(
317319
&format!("process didn't exit successfully: {}", self),
318320
Some(output.status),
319321
to_print,
@@ -388,14 +390,15 @@ mod imp {
388390
use anyhow::Result;
389391
use std::os::unix::process::CommandExt;
390392

391-
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> {
393+
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> {
392394
let mut command = process_builder.build_command();
393395
let error = command.exec();
394-
Err(anyhow::Error::from(error).context(ProcessError::new(
396+
Err(ProcessError::new(
395397
&format!("could not execute process {}", process_builder),
396398
None,
397399
None,
398-
)))
400+
)
401+
.with_source(error))
399402
}
400403
}
401404

@@ -411,10 +414,14 @@ mod imp {
411414
TRUE
412415
}
413416

414-
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<()> {
417+
pub fn exec_replace(process_builder: &ProcessBuilder) -> Result<(), ProcessError> {
415418
unsafe {
416419
if SetConsoleCtrlHandler(Some(ctrlc_handler), TRUE) == FALSE {
417-
return Err(ProcessError::new("Could not set Ctrl-C handler.", None, None).into());
420+
return Err(ProcessError::new(
421+
"Could not set Ctrl-C handler.",
422+
None,
423+
None,
424+
));
418425
}
419426
}
420427

‎crates/cargo-util/src/process_error.rs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ pub struct ProcessError {
2727
/// This can be `None` if the process failed to launch, or the output was
2828
/// not captured.
2929
pub stderr: Option<Vec<u8>>,
30+
31+
pub source: Option<Box<dyn std::error::Error + Send + Sync>>,
3032
}
3133

3234
impl fmt::Display for ProcessError {
@@ -35,7 +37,14 @@ impl fmt::Display for ProcessError {
3537
}
3638
}
3739

38-
impl std::error::Error for ProcessError {}
40+
impl std::error::Error for ProcessError {
41+
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
42+
match self.source.as_ref() {
43+
Some(s) => Some(&**s),
44+
None => None,
45+
}
46+
}
47+
}
3948

4049
impl ProcessError {
4150
/// Creates a new [`ProcessError`].
@@ -93,8 +102,17 @@ impl ProcessError {
93102
code,
94103
stdout: stdout.map(|s| s.to_vec()),
95104
stderr: stderr.map(|s| s.to_vec()),
105+
source: None,
96106
}
97107
}
108+
109+
pub(crate) fn with_source(
110+
mut self,
111+
source: impl Into<Box<dyn std::error::Error + Send + Sync>>,
112+
) -> Self {
113+
self.source = Some(source.into());
114+
self
115+
}
98116
}
99117

100118
/// Converts an [`ExitStatus`] to a human-readable string suitable for

‎src/bin/cargo/cli.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,11 @@ Run with 'cargo -Z [FLAG] [SUBCOMMAND]'",
110110

111111
if let Some(code) = expanded_args.value_of("explain") {
112112
let mut procss = config.load_global_rustc(None)?.process();
113-
procss.arg("--explain").arg(code).exec()?;
113+
procss
114+
.arg("--explain")
115+
.arg(code)
116+
.exec()
117+
.map_err(anyhow::Error::new)?;
114118
return Ok(());
115119
}
116120

‎src/bin/cargo/main.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use cargo::core::shell::Shell;
77
use cargo::util::toml::StringOrVec;
88
use cargo::util::CliError;
99
use cargo::util::{self, closest_msg, command_prelude, CargoResult, CliResult, Config};
10-
use cargo_util::{ProcessBuilder, ProcessError};
10+
use cargo_util::ProcessBuilder;
1111
use std::collections::BTreeMap;
1212
use std::env;
1313
use std::fs;
@@ -177,12 +177,10 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> Cli
177177
Err(e) => e,
178178
};
179179

180-
if let Some(perr) = err.downcast_ref::<ProcessError>() {
181-
if let Some(code) = perr.code {
182-
return Err(CliError::code(code));
183-
}
180+
if let Some(code) = err.code {
181+
return Err(CliError::code(code));
184182
}
185-
Err(CliError::new(err, 101))
183+
Err(CliError::new(anyhow::Error::new(err), 101))
186184
}
187185

188186
#[cfg(unix)]

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

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ pub trait Executor: Send + Sync + 'static {
107107
mode: CompileMode,
108108
on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>,
109109
on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>,
110-
) -> CargoResult<()>;
110+
) -> Result<(), ProcessError>;
111111

112112
/// Queried when queuing each unit of work. If it returns true, then the
113113
/// unit will always be rebuilt, independent of whether it needs to be.
@@ -130,7 +130,7 @@ impl Executor for DefaultExecutor {
130130
_mode: CompileMode,
131131
on_stdout_line: &mut dyn FnMut(&str) -> CargoResult<()>,
132132
on_stderr_line: &mut dyn FnMut(&str) -> CargoResult<()>,
133-
) -> CargoResult<()> {
133+
) -> Result<(), ProcessError> {
134134
cmd.exec_with_streaming(on_stdout_line, on_stderr_line, false)
135135
.map(drop)
136136
}
@@ -298,16 +298,14 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
298298
}
299299
}
300300

301-
fn verbose_if_simple_exit_code(err: Error) -> Error {
301+
fn verbose_if_simple_exit_code(err: ProcessError) -> Error {
302302
// If a signal on unix (`code == None`) or an abnormal termination
303303
// on Windows (codes like `0xC0000409`), don't hide the error details.
304-
match err
305-
.downcast_ref::<ProcessError>()
306-
.as_ref()
307-
.and_then(|perr| perr.code)
308-
{
309-
Some(n) if cargo_util::is_simple_exit_code(n) => VerboseError::new(err).into(),
310-
_ => err,
304+
match err.code {
305+
Some(n) if cargo_util::is_simple_exit_code(n) => {
306+
VerboseError::new(anyhow::Error::new(err)).into()
307+
}
308+
_ => anyhow::Error::new(err),
311309
}
312310
}
313311

‎src/cargo/ops/cargo_run.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,5 +97,5 @@ pub fn run(
9797

9898
config.shell().status("Running", process.to_string())?;
9999

100-
process.exec_replace()
100+
process.exec_replace().map_err(anyhow::Error::new)
101101
}

‎src/cargo/ops/cargo_test.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ fn run_unit_tests(
116116
let result = cmd.exec();
117117

118118
if let Err(e) = result {
119-
let e = e.downcast::<ProcessError>()?;
120119
errors.push((
121120
unit.target.kind().clone(),
122121
unit.target.name().to_string(),
@@ -255,7 +254,6 @@ fn run_doc_tests(
255254
.shell()
256255
.verbose(|shell| shell.status("Running", p.to_string()))?;
257256
if let Err(e) = p.exec() {
258-
let e = e.downcast::<ProcessError>()?;
259257
errors.push(e);
260258
if !options.no_fail_fast {
261259
return Ok((Test::Doc, errors));

‎tests/testsuite/messages.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
//! Tests for message caching can be found in `cache_messages`.
44
55
use cargo_test_support::{process, project, Project};
6-
use cargo_util::ProcessError;
76

87
/// Captures the actual diagnostics displayed by rustc. This is done to avoid
98
/// relying on the exact message formatting in rustc.
@@ -22,7 +21,7 @@ pub fn raw_rustc_output(project: &Project, path: &str, extra: &[&str]) -> String
2221
.exec_with_output()
2322
{
2423
Ok(output) => output.stderr,
25-
Err(e) => e.downcast::<ProcessError>().unwrap().stderr.unwrap(),
24+
Err(e) => e.stderr.unwrap(),
2625
};
2726
// Do a little dance to remove rustc's "warnings emitted" message and the subsequent newline.
2827
let stderr = std::str::from_utf8(&rustc_output).expect("utf8");

0 commit comments

Comments
 (0)