std::process::ExitStatus Into Result, or .expect(), or something

[ijackson]

It is not particularly convenient to use the return value from wait() in std::process. Perhaps it should be convertable to a Result ? Or gain its own expect methods etc. ?

[ijackson]

@KodrAus apropos of recent discussion in #81452 and #73131:

Motivation

It is far too easy to accidentally drop an ExitStatus on the floor, rather than checking it properly. Indeed almost none of the examples in std::process get this right !

Doing it right with the current API is highly un-ergonomic. We should be using Result.

Furthermore, there is no code in the Rust stdlib which will generate an appropriate message describing the fate of a subprocess. (I.e., generate a message comparable to that from the shell naming a fatal signal, or a message quoting the value passed to exit.)

Proposal sketch

impl ExitStatus {
    pub fn exit_ok(self) -> Result<(), ExitStatusError>;
}

#[derive(Copy,Clone,Debug,Eq,PartialEq)]
/// Subprocess status other than success.
pub struct ExitStatusError {
    wait_status: NonZeroU32 // on Unix; something else on Windows
}

impl std::error::Error for ExitStatusError { }
impl From<ExitStatusError> for ExitStatus {...}
impl ExitStatusExt for ExitStatusError { ... same as on ExitStatus ... }

impl Display for ExitStatusError {
   ... checks ExitStatusExt::signal(), ::coredump() etc.
}

impl ExitStatusError {
    fn into_io_error(self) -> std::io::Error { ... }
}

impl From<ExitStatusError> for std::io::Error {...}

Improved ergonomics

This allows the programmer to use ?; the panoply of methods on Result; and crates such as anyhow and thiserror.

Added functionality

There is no new functionality here - only new more ergonomic types.

The only nontrivial functionality being added here is the Display impl.

Currently, Rust does not provide a way to print a sensible message about the fate of a child process. The Display impl for ExitStatus simply prints the numeric value. (Actually, on Unix the printed value is more properly called a "wait status", since it's a value returned from wait; this is a different value to the one passed to exit, which is a true exit status.)

We probably don't want to change the Display impl for ExitStatus (especially because people might be doing something more with it, eg putting it in logfiles which they then search, etc.). But we do want to provide something more useful. The Display impl for ExitStatusError would produce, in the usual cases, a message like one of these:

In fact I was confused and this behaviour is in the Debug impl. The Display impl is more suitable and will be made more correct in #82411.

It would be nice to change the messages to something clearer, maybe:

  exited with error exit status 4
  died due to fatal signal Terminated
  died due to fatal signal Segmentation fault (core dumped)

but that is for the future.

Example use

            Command::new(adb_path)
                .arg("push")
                .arg(&exe_file)
                .arg(&self.config.adb_test_dir)
                .status()
                .unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path))
                .exit_ok()
                .unwrap_or_else(|e| panic!("{:?} failed: {}", adb_path, e));

In a call site using anyhow:

        (|| Ok::<_,anyhow::Error>({
            Command::new(adb_path)
                .arg("push")
                .arg(&exe_file)
                .arg(&self.config.adb_test_dir)
                .status()?
                .exit_ok()?
        }))().with_context(|| adb_path.clone())?;

Alternatives

Do nothing

This is unattractive because the existing API is cumbersome. Improving the ergnomics is necessary before making ExitStatus #[must_use], or we would be asking everyone in the community to write unergonomic code now and hope to improve it later.

Return Result<(),std::io::Error>

This would have the benefit of consistency with most other functions in std::process. However, it is not suitable.

io::Error is primarily an abstraction over operating system error codes. (It can contain other errors so that implementors of important traits such as Read are not forced to lose error information.) This can be seen in the public API but it is most convenient to summarise the internal representation, which is a three-variant enum:

• Os(i32), containing a raw OS error code. But a wait status is not an OS error code so we cannot use this.
• Simple(ErrorKind). The ErrorKinds correspond to (subsets of) OS errors. We would hae to use Other and completely lose the child's wait status.
• Custom(Box<(ErrorKind, Box<dyn std::error::Error>)). That would require us to invent our own Error type, since we need something to record as the "inner error".

Inventing our own error type is necessary in any case because we need something to hang the new Display functionality on. Given that, wrapping it up in an io::Error seems perverse.

Provide Into<io::Error> as well as or instead of into_io_error(), or neither

Because most of the rest of std::process generates only operating system errors, call sites are fairly likely to be in functions themselves returning io::Result.

It seems to me that in this specific case we at the very least need to provide a convenient explicit conversion facility to help smooth the recovery from the ecosystem breakage which will expect when we make ExitStatus #[must_use]. When fixing code which previously ignored the exit status, it may be awkard to bubble an entirely different error type up through a program or library. A provided conversion to io::Error would make a local fix to such code considerably easier.

While an io::Error is not the most precise representation, code that does not know better will be reasonably well served by this conversion. The risk of unexpected lossage from its use seems fairly low because anywhere that handles io::Error must already be prepared to handle "unexpected" errors.

As an alternative, we could do this as a From impl. This would be quite unusual. The Rust stdlib does not generally provide builtin conversions to generic portmanteau error types, let alone a From impl. This is particularly hazardous because of the ? autoconversion.

So I propose this:

impl ExitStatusError {
    /// The returned `io::Error` will have kind `Other` and have
    /// this `ExitStatusError` as its inner error.  This is a convenience
    /// wrapper equivalent to calling `io::Error::new`.
    /// 
    /// Provided for situations where it is not convenient to deal
    /// with a subprocess which ran but failed differently to other
    /// errors which occur when running a subprocess.
    fn into_io_error(self) -> std::io::Error { ... }
}

Provide something else, eg a method taking an error handling closure

Result is the ergonomic and idiomatic way to represent errors in Rust.

And we need something to hang the new error display functionality off. There would be nothing wrong with having that as a method on ExitStatus (although we'd have to decide what it did with success) but using the resulting interface would be clumsy in the usual case.

Provide .exit_ok on Command

This seems like a fairly obvious convenience. It would be roughly equivalent to .status()?.exit_ok(). But its error type would have to be a specific custom enum, or wrap an exit status in io::Error where it is hard to fish out again.

So this seems to have open questions. I propose to leave it for future work.

Bikesheds

What precise part of speech and grammar to use for the messages

I have suggested phrases like "exited with error exit status 4". An alternative would be a shorter version "fatal signal Terminated", "error exit status 4". But the latter risks confusion between the true exit status (what was passed to exit) and the wait status; this confusion is particularly bad for Rust because std::process calls the wait status an ExitStatus. Using the verb "exited" helps avoid this.

On Unix, the wait status resulting from a call to exit is the exit_status << 8. So my example "exited with error exit status 4" corresponds to std::process::exit(4) and ExitStatus(1024).

Alternative method name instead of exit_ok

The name should have the following properties:

• It should be fairly short, since most users of Command will need it.
• When found in a series of chained method calls, as is common with Command, its function should be reasonably clear.

Prior art

Most fallible functions in stdlib return a custom error type whose payload characterises the error.

In many cases these error types are thing wrappers around another type or around unit. Eg, std::path::StripPrefixError, std::char::DecodeUTF16Error. std::array::TryFromSliceError, std::num::TryFromIntError.

Some of these types wrap an original input value, or other information about the error. For example std::sync::PoisonError, std::io::IntoInnerError, std::ffi::IntoStringError, std::ffi::NulError std::str::Utf8Error, std::string::FromUtf8Error, std::ime::SystemTimeError and arguably NoneError.

Most other functions in std::process return io::Result because their failures correspond to failed operating system calls with an OS error number and corresponding ErrorKind.