ErrorKind::ProcessFailed and impl From<ExitStatusError> #88306
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We add this to the existing feature gate exit_status_error, since we will probably want to stabilise this trait impl along with the new type - or not at all. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
inquisitivecrystal
added
the
T-libs
This should be fine on all actual Unix platforms since errno values are quite conventional, especially very basic ones like ENOENT. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
JohnCSimon
added
S-waiting-on-review
JohnCSimon
added
S-waiting-on-review
yaahc
reviewed
Oct 5, 2021
There was a problem hiding this comment.
Thank you as always for the attention you're giving to improving the Command APIs. I see the value to providing more convenient ways to invoke Commands, and I think your goal with this PR is quite similar to @matklad's goal with #89004, which I agree with. However, I really worry that making it possible to automatically shove an ExitStatusError into an io::Error will encourage bad error reporting habits.
I think @matklad probably has the right balance of convenience and reporting. If we're going to collapse the error types we should do so in a way that works as a reasonable default and include the extra context such as the command which was run.
library/std/src/io/error.rs
Outdated
Comment on lines
264
to
275
| /// A custom error that does not fall under any other I/O error kind. | ||
| /// | ||
| /// This can be used to construct your own [`Error`]s that do not match any | ||
| /// [`ErrorKind`]. | ||
| /// | ||
| /// This [`ErrorKind`] is not used by the standard library. | ||
| /// This operation is unsupported on this platform. | ||
| /// | ||
| /// Errors from the standard library that do not fall under any of the I/O | ||
| /// error kinds cannot be `match`ed on, and will only match a wildcard (`_`) pattern. | ||
| /// New [`ErrorKind`]s might be added in the future for some of those. | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| Other, |
There was a problem hiding this comment.
Why'd you reorder these? I think this messes with the derived Ord impl which I'd prefer to avoid without strong justification.
yaahc
added
S-waiting-on-author
JohnCSimon
added
S-waiting-on-author
JohnCSimon
added
S-waiting-on-author
Should this PR be closed then? |
rustbot
added
the
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think this impl makes process error handling a lot more ergonomic. Look at the new example to see what I mean.
If we are going to do this, it makes more sense to do it now, while
ExitStatusErroris unstable. We can experiment with how we like the results, without insta-stably providing the trait impl.There is a possible concern related to the resulting error messages. Because of the way
io::Error'sDisplayimpl simply forwards to the underlying error, the message"subprocess failed"won't be part of those errors.But the
Displayimpl onExitStatusErrorprefixes the message fromExitStatuswith"process exited unsuccessfully", which is nicely unambiguous.#84908 (comment) has more discussion about this in the context of the other possible messages arising from process handling, in particular
Command::status().Other possibilities
This ability to represent a subprocess failure status as an
io::Errorwill be useful in other situations:This would make it possible to provide an
exit_okmethod onCommand, improving the ergonomics of simple subprocess executions.It would also make it much more possible to provide a version of
Command::outputthat is less prone to the programmer accidentally losing the exit status. It would even be possible to provide a "streaming" version of the output: a type which would encompass theChildand the pipe, andimpl io::Read, and which would give a read error if the child failed.Just the
ErrorKindcould be helpful for some other libraries that can otherwise useio::Errorfor their error handling.Note this MR branch is on top of #88298 which has already been approved and I expect to land soon.
Let me see if I can get highfive to do what I want:
r? @yaahc