Add #[must_use] to process::Command, process::Child and process::ExitStatus #81452
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
|
r? @KodrAus (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
rustbot
added
the
T-libs-api
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Well there are quite a lot of these, aren't there! |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@rustbot modify labels -S-waiting-on-review +S-waiting-on-author Looks like I am going to be iterating this through the CI for a little while still... |
rustbot
added
S-waiting-on-author
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
If you don't need to call methods on the `Child`, then `status` is more convenient. This should be mentioned. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
All these examples called `spawn` and then didn't wait for the child (!) Fix this by having them call .status() rather than .spawn(), and assert that the status was success. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
See rust-lang#70186 which makes the point about io handles. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
These three doctests called `.status()` but ignored the result. Fix this. In two cases, with an assertion, and in the case where we are just printing something, by printing the exit status value. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
We are making `ExitStatus` `#[must_use]`. So we need to explicitly handle the exit status here. Handle it the same way as the values from the repeated calls to `try_wait`. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Simply dropping this is usually a bad idea, for the reasons extensively discussed in the documentation. Closes: rust-lang#70186 Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
For the same reason as futures. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
For the same reason as `Result` is #[must_use] Cloess: rust-lang#73127 Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
ijackson
force-pushed
the
must-use-processes
branch
from
361c9e3
to
b6a8dd6
Compare
|
OK, with all those fixes, rust-lang/rust compiles again now.... @rustbot modify labels +S-waiting-on-review -S-waiting-on-author |
rustbot
removed
the
S-waiting-on-author
rustbot
added
the
S-waiting-on-review
|
Thanks for the PR @ijackson! This would be great to clean up, but I think we should tread carefully here, because let mut cmd = Command::new("proc").args(["-a", "-b"]);
if some_condition {
cmd.arg("-c");
}
// return here without using the command
// #[must_use] doesn't trigger because we "used" the `Command` to add an extra argBefore adding impl process::ExitStatus {
pub fn success_or<E>(self, err: E) -> Result<(), E> {
self.success_or_else(|| err)
}
pub fn success_or_else<F, E>(self, err: F) -> Result<(), E>
where
F: FnOnce() -> E,
{
if self.success() {
Ok(())
} else {
Err(err())
}
}
}So that instead of writing: let status = 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));
if !status.success() {
panic!("Program failed `{:}`", adb_path);
}We could write: Command::new(adb_path)
.args(&["forward", "tcp:5039", "tcp:5039"])
.status()
.and_then(|status| status.success_or_else(|| panic!("Program failed `{:}`", adb_path)))
.unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path));So it seems like something worth pursuing, but I think there's more we should do to make |
|
Ashley Mannix writes ("Re: [rust-lang/rust] Add #[must_use] to process::Command, process::Child and process::ExitStatus (#81452)"):
Before adding #[must_use] to ExitStatus it might be good to look into a nicer
API for converting the ExitStatus itself into a Result and working with that.
I definitely think something like you suggest is needed. I even filed
an issue #73125 asking for it, and suggested something along these
lines in my pre-RFC on internals :-).
I think perhaps the people who advised me to go straight to
`#[must_use]` didn't appreciate how many bugs it would show up
(certainly it surprised me) and how un-ergonomic handling of an
`ExitStatus` is.
I think my preferred approach would be something like this
```
impl ExitStatus {
pub fn into_result(self) -> Result<(), ExitStatusError>;
```
where `ExitStatusError` is (on Unix) a newtype for NonZeroUwhatever,
implementing `Error` etc., and also `unix::ExitStatusExt` (so you have
`.signal()` et al.)
I'm not sure `into_result` is the right name. `.ok` is temptingly
short but elsewhere (eg on `Result`) returns `Option<>`.
We could write:
Command::new(adb_path)
.args(&["forward", "tcp:5039", "tcp:5039"])
.status()
.and_then(|status| status.success_or_else(|_| panic!("Program failed `{:}`", adb_path)))
.unwrap_or_else(|_| panic!("failed to exec `{:?}`", adb_path));
I think this would be better for the example:
```
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));
.ok()
.unwrap_or_else(|s| panic!("`{:?}` failed: {}", adb_path, s));
```
If you're using something like anyhow, you could do this:
```
Command::new(adb_path)
.arg("push")
.arg(&exe_file)
.arg(&self.config.adb_test_dir)
.status()?
.ok()?;
```
(maybe with some calls to `context`)
Thanks for the PR @ijackson! This would be great to clean up, but I
think we should tread carefully here, because #[must_use] isn't a
silver bullet. Adding it to Command seems like a good idea, because
it is pretty useless if you don't actually spawn something from it,
but it won't catch cases like this:
let mut cmd = Command::new("proc").args(["-a", "-b"]);
if some_condition {
cmd.arg("-c");
}
// return here without using the command
// #[must_use] doesn't trigger because we "used" the `Command` to add an extra arg
Indeed. I still think it's useful even if the win is only partial.
So it seems like something worth pursuing, but I think there's more we should
do to make ExitStatus nicer to work with using standard error handling tools
first.
Thanks for the review.
I think that means that of the items in my tracking issue #73131,
I should pursue #73125 (request for affordance to make Result from
ExitStatus) next and then come back to this one.
Thanks,
Ian.
…--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.
|
6 tasks
|
Sure. Closing this MR for now if fine by me. I will do that. |
5 tasks
|
I just saw this question on StackOverflow where the asking person would have been saved by the |
ijackson
added a commit
to ijackson/rust
that referenced
this pull request
May 12, 2021
Closes rust-lang#73125 This is in pursuance of Issue rust-lang#73127 Consider adding #[must_use] to std::process::ExitStatus In MR rust-lang#81452 Add #[must_use] to [...] process::ExitStatus we concluded that the existing arrangements in are too awkward so adding that #[must_use] is blocked on improving the ergonomics. I wrote a mini-RFC-style discusion of the approach in rust-lang#73125 (comment) Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 18, 2021
Provide ExitStatusError Closes rust-lang#73125 In MR rust-lang#81452 "Add #[must_use] to [...] process::ExitStatus" we concluded that the existing arrangements in are too awkward so adding that `#[must_use]` is blocked on improving the ergonomics. I wrote a mini-RFC-style discusion of the approach in rust-lang#73125 (comment)
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.
Motivation
These three critical types in
std::processshould all be#[must_use]. Failure to use aCommandis hopefully fairly obvious but it seems friendly to mark this one for the same reason as futures.Failure to use a
ChildorExitStatusare more serious.On unix, failure to use a
Childleaks the process. It continues running in parallel, even after we have exited (or crashed), possibly interfering with the user's terminal or driveling nonsense into a logfile long after the original event. In a long-running process, the child will become a zombie when it exits, and accumulation of zombies can prevent the spawning of new children. IDK what failure to use aChilddoes on Windows but the docs are not encouraging.Failure to use an
ExitStatusis the kind of error-handling bug Rust usually tries to save us from.Contents of this MR
The main aim is just to mark these three types
#[must_use]. However, I reviewed the documentation and it was missing two of the problems with leakingChild. I also felt.spawn()ought to direct the reader to.status().More worryingly, the
#[must_use]changes revealed that most of the examples were broken. Most of them leak theChildand nearly none of them check the exit status. So I fixed that.Bugs this revealed in other parts of rust-lang/rust, outside the stdlib
The bootstrap wrapper for rustc ignored the status of the
on_failcommand. Although failing to run it at all produces a panic, I felt that was a bit much for if it simply fails. So I chose to print a warning to stderr. We are already failing here, so the shim will exit nonzero in any case.Two tests in
src/ui/testfailed to check the exit status; in one case this is a clear bug.The runtest wrapper failed to check the exit status from two of its invocations of adb.
Concern
During a pre-RFC discussion I was told that adding a
#[must_use]is not considered a breaking change because it's only a lint.But I worry that this imay be too disruptive a change, given the problems in the stdlib examples themselves, and the rest of the lang tree.
On the other hand: almost all of the things that this found in the stdlib were actual bugs - bugs which would have been quite serious in deployed code. Perhaps it is better to do our community the service of highlighting these problems rather than tolerating them.
A friend observed that adding these
#[must_use]would be "doing the community a favour, but I'm not sure they'll enjoy it".