Tracking Issue for ExitStatusError

[ijackson]

Feature gate: #![feature(exit_status_error)]

This is a tracking issue for ExitStatusError (and ExitStatus::exit_ok).

This feature makes it convenient to properly check the exit status of subprocesses (such as from Command)

Example

use std::process::Command;

let bad = Command::new("false").status().unwrap().exit_ok().unwrap_err();
assert_eq!(bad.code(), Some(1));

Public API

(In pseudo-syntax:)

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

pub struct ExitStatusError(...); // newtype around a NonZero integer
impl Eq,Copy,Debug,Error,Display for ExitStatusError;
impl Into<ExitStatus> for ExitStatusError;
impl ExitStatusError  {
    fn code(&self) -> Option<i32> {..} } // WIFEXITED WEXITSTATUS
    fn code_nonzero(&self) -> Option<NonZeroi32> {..}
    ...
}
impl ExitStatusExt for ExitStatusError; // .is_signal() etc.

impl Output {
  fn exit_ok(self) -> Result<Self, ExitStatusError>;
}

Steps / History

• Implementation: Provide ExitStatusError #82973
• ACP for Output::exit_ok Add std::process::Output::exit_ok libs-team#554
• Final commenting period (FCP)
• Stabilization PR (including restoring mentions in ExitStatus docs)
• Add #[must_use] to process::ExitStatus (previous attempt Add #[must_use] to process::Command, process::Child and process::ExitStatus #81452); see also Tracking Issue for std::process error handling #73131.
• Consider implementing Try for ExitStatus

Unresolved Questions

• None yet.

[yaahc]

Can you also add a note about implementing Try for exit status to the Steps section?

related: #54889

[taralx]

Late to the party, but the double unwrap seems like it should be avoidable. Is that in the plan?

[ijackson]

Late to the party, but the double unwrap seems like it should be avoidable. Is that in the plan?

I have a plan for it :-)

The reason for the two unwraps is this: Command provides a method status which produces an io::Error (representing failure to set up or invoke or reap). When we get the exit status, exit_ok turns it into an ExitStatusError. So there are two error types. We can't collapse these without having a type that can represent both errors. We could use io::Error::custom with ErrorKind::Other but that loses a lot of information (and, now, is contrary to a policy we are adopting, that ErrorKind::Other won't ever be generated by stdlib - see #85746).

I agree that this is suboptimal, particularly in simple cases where the error handling can be unsophisticated (but obvs. still wants to be correct).

I think the fix is to provide impl Into<io::Error> for ExitStatusError. That would make it possible to provide an exit_ok() method on Command. There is actually a decision to make here, but having thought about this for a while I have convinced myself that this is the right approach.

I plan to do that but right now I am waiting for the dust to settle around some other changes I am trying to make to io::ErrorKind - #79965. The Into impl I propose above would involve a new io::ErrorKind variant.

[yaahc]

I don't know. I for one feel like the double unwrap is a useful property of this API, since those errors are coming from fundamentally different sources. The former comes from the OS when trying to setup the subprocess where as the latter represents the exit status of the process itself. I'd prefer to leave unifying the io::Error and the ExitStatusError to the users of this API, since unifying it ourselves will encourage less contextually relevant error reporting.

[ijackson]

I don't know. I for one feel like the double unwrap is a useful property of this API, since those errors are coming from fundamentally different sources. The former comes from the OS when trying to setup the subprocess where as the latter represents the exit status of the process itself. I'd prefer to leave unifying the io::Error and the ExitStatusError to the users of this API, since unifying it ourselves will encourage less contextually relevant error reporting.

I don't think it would produce less contextually relevant errors. But I think I can best demonstrate that by prototyping it and showing what the error messages are that come out. So I will do that.

It would also be possible to introduce the new ErrorKind, and a method to construct an io::Error from an ExitStatusError, and a method on Command to run the program and collect and check the status, without the From impl. That mean that bundling errors together would be more of a conscious decision, and not something you could do by mistake with just ?. I think it would still provide quite big ergonomic benefits for programs which do a lot of running of other commands, provided that the actual error reports it produces are good. Which is your concern (and which I agree ought to be a concern).

I have a secret other reason for wanting an ErrorKind that is for holding an ExitStatusError. Currently, the API for getting the stdout of a Command is awkward, and really invites mistakes which accidentally drop the exit status and/or can deadlock (depending which of the Command methods you use). I have an idea about how to improve this, at least for the easy cases. My idea is to have a type which contains both the pipe reading handle for the stdout, and the Child. It impls Read. In order to properly notify you of child failure, which is distinct from EOF, it needs to encapsulate the ExitStatus in an io::Error. To get the right message, it really ought to have its own ErrorKind. I have prototyped this in a project of my own (wiithout a custom ErrorKind) and it's nicely ergonomic, and all the tricky stuff with getting the sequencing right (and working around kernel bugs) can be done in one place.

[ijackson]

I did some experiments with io::Error::new (which is surely how any conversion would work) and the results are encourating. The io::Error made from an ExitStatusError prints and behaves almost identically to the original ExitStatusError:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b8b10bcea7dc0d14019a0f013ded84e6

[yaahc]

I'm less worried about the io::Error changing the display output than I am about having the original io::Error from the cmd setup being confusable with the cmd exit result. The more I think about it though the less convinced I am that this is actually a risk. I'd like to get an idea of the kinds of errors that .status() can return to double check but assuming those are all relatively clear that they came from process setup and not the subprocess I'll be more supportive of alternate APIs that don't double unwrap.

[ijackson]

@yaahc I have been thinking about this and have convinced myself that impl From<ExitStatusError> for io::Error is a thing we want to do now, before stabilising ExitStatusError. So I thought I would continue our conversation about the error message implications, here:

I'd like to get an idea of the kinds of errors that .status() can return

I can't speak to Windows. But on Unix, .status() can fail because:

• The program to run cannot be found or is not executable, or something: this could include any filesystem errors such as NotFound, FilesystemLoop, PermissionDenied, etc. This is by far the most common cause.
• fork failed (EAGAIN, unhelpfully clashing with many other situations, translated to ErrorKind::WouldBlock)
• strange other situations that are highly unusual (eg, the Rust program messed up its fd handling, which is kind of UB-like on unix, giving EBADF)
  Additionally, attempts to pipe into a program might fail with BrokenPipe. That's not really relevant for .status() but it might be relevant because I'd like to provide more "cooked" versions of the command pipe facilities.

I don't think these are readily confuseable with the kinds of things that go wrong with the process itself. Additionally, of course, if the process failed it probably printed something to stderr itself. Hopefully the user can see that. And, of course, this new impl needs a new ErrorKind.

[yaahc]

* The program to run cannot be found or is not executable, or something: this could include any filesystem errors such as `NotFound`, `FilesystemLoop`, `PermissionDenied`, etc.  This is by far the most common cause.

* fork failed (`EAGAIN`, unhelpfully clashing with many other situations, translated to `ErrorKind::WouldBlock`)

* strange other situations that are highly unusual (eg, the Rust program messed up its fd handling, which is kind of UB-like on unix, giving `EBADF`)
  Additionally, attempts to pipe into a program might fail with `BrokenPipe`.  That's not really relevant for `.status()` but it might be relevant because I'd like to provide more "cooked" versions of the command pipe facilities.

I think I agree that we probably want this and that the diagnostics won't be confusable but I'd like to add some tests for the various cases just to ensure we know exactly what output we're expecting. Just some basic tests in some new directory like src/test/ui/error-display that construct the errors and then compare them to the expected output so we can manually review the expected output to ensure it includes sufficient context.

Edit: I just realized the PR you opened has an example which tests exactly this for the case where the command itself fails. I'd like to do the same thing for the case where setting up the command is the part that fails to compare to make sure they're unambiguous, and if possible I'd like to make the same test work on windows / macos so we can make sure none of the platforms have major gaps in their diagnostics, but it's looking good so far.

I don't think these are readily confuseable with the kinds of things that go wrong with the process itself. Additionally, of course, if the process failed it probably printed something to stderr itself. Hopefully the user can see that. And, of course, this new impl needs a new ErrorKind.

Not following what you mean by "And, of course, this new impl needs a new ErrorKind." Do you mean an error kind to indicate that the io::Error is storing an ExitStatusError?

[ijackson]

Edit: I just realized the PR you opened has an example which tests exactly this for the case where the command itself fails.

:-). Sorry for not mentioning this explicitly...

I'd like to do the same thing for the case where setting up the command is the part that fails to compare to make sure they're unambiguous,

The tests in library/std/src/process/tests.rs already checked that the error had the right kind, for the most obvious kind of failure (program to run doesn't exist). But because of the way io::Error works, different errors with the same kind can display differently. So I added an explicit test of the precise string for #[cfg(unix)].

and if possible I'd like to make the same test work on windows / macos so we can make sure none of the platforms have major gaps in their diagnostics, but it's looking good so far.

I think macos is #[cfg(unix)]. If someone wants to add a suitable test for Windows that would be nice. I didn't want to iterate through the CI - especially as I don't know enough about Windows to know how to write a test that works on different versions or whatever.

Not following what you mean by "And, of course, this new impl needs a new ErrorKind." Do you mean an error kind to indicate that the io::Error is storing an ExitStatusError?

Roughtly that. An error kind to indicate that the problem was a subprocess failure and that it probably contains an ExitStatusError. See the docs I add in my MR for the precise semantics I thought were rright.

Maybe we should continue this converation in the MR discussion?