impl Try for ExitStatus

[ratmice]

I figured I could take a stab at issue #54889,

Currently just sys/unix is implemented,
More work needs to be done with testing and platforms which I don't have access to.

But given that i've not done much coding in libstd, and this is a bit roundabout with imp::process calling crate::process,
in a call from crate::process calling imp::...

Along with the from_output where we need to materialize an imp::ExitStatus out of (), I figured it'd be good to get some feedback early if possible.

Thanks

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[scottmcm]

(Marked this needs-fcp because ExitStatus is a stable type, so accepting this PR would make ?-on-ExitStatus insta-stable.)

[bors]

☔ The latest upstream changes (presumably #94514) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #94546) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #96904) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

We added ExitStatus::exit_ok, which seems like an alternative vision from this PR -- I wouldn't expect us to want both.

I'm going to r? @yaahc since you maybe felt otherwise in #84908 (comment) -- it seems like that would've been an unresolved question rather than a Step in my mind. Maybe that's what you meant :)

[yaahc]

I'm going to r? @yaahc since you maybe felt otherwise in #84908 (comment) -- it seems like that would've been an unresolved question rather than a Step in my mind. Maybe that's what you meant :)

It's been quite a while so I don't remember exactly what I was thinking at the time but your instinct to prefer only having one form over having both seems valid. Out of the two I think I'd prefer the version more closely integrated with Try (aka this one).

[ratmice]

FWIW, this is merely marked as draft, because I hadn't intiially implemented windows support,
subsequently I implemented that -- because it seemed straitforward. However, I don't have a windows machine to run/compiled/tested code on.

But that is I think the only reason it is still a draft, so I could mark it as ready for review if that helps,
also let me know if I should rebase on master, only been doing it so far as conflicts make it unmergable.

[Mark-Simulacrum]

Out of the two I think I'd prefer the version more closely integrated with Try (aka this one).

FWIW, I think I feel differently -- I don't think implementing Try on types that aren't in the return type is a good idea; a magic conversion from ExitStatus to Result feels iffy to me. This is particularly true since e.g. https://doc.rust-lang.org/nightly/std/process/struct.Command.html#method.status already returns Result<ExitStatus>, so you'd potentially end up with cmd.status()?? which is, well, not something I'd like to see; it looks just visually bad to my eyes.

exit_ok in contrast gives a nice separator and lets us have other meanings (e.g., maybe we want an exit_failed in the future, or an exit_ok_command(&Command) of some kind to include a debug-print of the Command in the error...). (I am not a fan of exit_ok's name since it is sort of "not" exiting on an OK exit, but that's a separate bikeshed).

But that is I think the only reason it is still a draft, so I could mark it as ready for review if that helps,
also let me know if I should rebase on master, only been doing it so far as conflicts make it unmergable.

I think it's worth settling the question of whether we want to pursue this path before you do a bunch of work over time updating this PR (rebasing, implementing Windows, etc.).

[ratmice]

This is particularly true since e.g. https://doc.rust-lang.org/nightly/std/process/struct.Command.html#method.status already returns Result<ExitStatus>, so you'd potentially end up with cmd.status()??

It is an interesting example, I suppose it strikes me the myriad of ways that the Result::flatten fails to apply in this situation,
Both in Result<Result<..>,..> vs X:Try .. Result<X, ..>, but also that it requires the same E type across both.

It at the very least makes me consider what (if any) the equivalent of flatten for Try should be...

Edit: I tried messing around with hacks to see if I could get anything flattenesque, best I could come up with 🤷
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=49503eff733d19ec2c5b7f61e97ce66d

[yaahc]

FWIW, I think I feel differently -- I don't think implementing Try on types that aren't in the return type is a good idea; a magic conversion from ExitStatus to Result feels iffy to me. This is particularly true since e.g. https://doc.rust-lang.org/nightly/std/process/struct.Command.html#method.status already returns Result<ExitStatus>, so you'd potentially end up with cmd.status()?? which is, well, not something I'd like to see; it looks just visually bad to my eyes.

exit_ok in contrast gives a nice separator and lets us have other meanings (e.g., maybe we want an exit_failed in the future, or an exit_ok_command(&Command) of some kind to include a debug-print of the Command in the error...). (I am not a fan of exit_ok's name since it is sort of "not" exiting on an OK exit, but that's a separate bikeshed).

Hmmm. I don't have the same experience of finding it confusing or magical. ExitStatus is a well established analog to Result and I think it logically makes sense to treat it as a source of control flow. I very much expect ExitStatus to be used as a return type in much the same way Result is.

That said I do see some advantages to forcing it to go into a Result first, such as making it easier to apply existing combinators for Results to the error such as .wrap_err/.context from eyre/anyhow, though I wonder if those combinators couldn't be rewritten to leverage Try conversions if Try were stable. That might be a good thing to experiment with.

[Mark-Simulacrum]

FWIW, if my method has a signature like -> ExitStatus, then that seems "okay" to have Try early-return to. (Though I think I've never written such a function, so utility seems low -- at minimum, I'd typically have it wrapped in io::Result). IOW, I think I disagree with "ExitStatus is a well established analog to Result", since I don't think I see it used in return types anywhere near enough to justify that conclusion.

But if the Try conversion would involve converting from ExitStatus to Result<(), ExitStatusError> or even something else seems pretty "magical" to me, and I'm not sure on the value of it -- if it's also mapping ExitStatusError to some other error type, that seems even more questionable to my eyes.

[yaahc]

IOW, I think I disagree with "ExitStatus is a well established analog to Result", since I don't think I see it used in return types anywhere near enough to justify that conclusion.

What I meant by this is that it is representing the equivalent constructs from os process error reporting. Status codes and signals are the error reporting mechanism for processes and represent both the control flow (success / fail) and the explanation of the failure. I feel like it's fairly intuitive to imagine ExitStatus as equivalent to Result<(), ExitStatusError>.

[Mark-Simulacrum]

I think we sort of agree on that point -- ExitStatus does resemble a Result in some ways at the process level. ExitStatusError is a pretty poor error in terms of being able to interpret it (really wants some amount of context attached), but the same is true of io::Error, if to a slightly lesser extent.

I'm somewhat ambivalent now on which I prefer -- in general, I think I still lean towards exit_ok, but without any meaningful justification.

There's a slight feeling in my mind that some explicit declaration of 0 = Ok, other = Err is valuable, since not all programs respect that convention -- for example, if running grep, it has zero and one as both really "Ok" return types and 2 as error exit code. (-q changes that to squash errors on successful match, I guess, but still). But that's a weak feeling.

I was going to nominate this for libs-api to make a decision here but I'll leave whether to do that in your hands as the assigned reviewer - the alternative being to fcp merge this, presumably, implicitly closing/removing exit_ok + ExitStatusError?

[Mark-Simulacrum]

If I follow the docs on Try correctly, I think this isn't quite right.

Try::from_output(x).branch() --> ControlFlow::Continue(x) to me implies that type Output = ExitStatus would be correct here.

I think the current implementation is forcing a synthesis of ExitStatus from (), whereas you actually "want" to just pass along the ExitStatus you're taking in branch.

[ratmice]

This is definitely the least straightforward part of this patch, and definitely deserves scrutiny.
I believed though that it does meet the Try laws, because branch() --> ControlFlow::Continue(x) iff self.code() == 0.

And I think type Output = ExitStatus seems weird to me, because ExitStatusError is a subset of ExitStatus such that the values intersect, so I felt it was weird in the way that a Result<ExitStatus, ExitStatusError> would be strange, since it has values which would be valid in both the Ok, and the Err branches.

Anyhow, that is the justification for why I did it this way at least.

[ratmice]

I also noticed this in the documentation for the Residual associated type:

This represents the possible values of the Self type which are not represented by the Output type.

https://doc.rust-lang.org/stable/std/ops/trait.Try.html#associatedtype.Residual

Which type Output = ExitStatus would run afoul of.

Edit: I think if we want to change the type Output here away from unit, we should try and introduce a new type to complement ExitStatusError. Such as ExitStatusSuccess or some bikeshed to that extent.

Edit 2: This would also would allow exit_status???? because success would be identity which implements Try.

[ijackson]

I may be misunderstanding things, but Try seems too abstract for ExitStatus, and it also feels weird to have Try where the residual is an existing other Try type (in this case, Result). I didn't think Try was intended to supplant the usual error conversion (or indeed, very much, to supplement it). And we're definitely talking about errors here.

[ratmice]

Whether Result is actually the right thing here or not I'm not certain, In the docs for the Residual type, they all have a generic type parameter which gets filled with infallible, where ExitStatusError has no generic types.

And it seemed like defining a new type specifically for this, that it would end up being an ad-hoc implementation of Result.
Whether than was the right choice, i'm not sure though.

[bors]

☔ The latest upstream changes (presumably #101077) made this pull request unmergeable. Please resolve the merge conflicts.

[dtolnay]

Reassigning yaahc's reviews as she has stepped down from the review rotation.

r? rust-lang/libs-api

[Amanieu]

The libs-api-nominated label seems to have not been properly applied, so this was never discussed in a meeting. FWIW I agree with @Mark-Simulacrum here: I'm hesitant to add more Try implementations in general: we're currently having some regrets to adding this to Future since it ended up making code harder to follow in practice.

[ratmice]

Alright, well I figure I should summarize the open questions probably:

• Should we implement Try for this at all?
• If so then what should the Output/Residual types be?
  • In the patch current I used ()/Result, which seems controversial.
  • To avoid Result I think we need a new type like ExitStatusSuccess for the Output to complement ExitStatusError in a way that doesn't implement Try.

[ijackson]

I have just proposed this RFC rust-lang/rfcs#3362 for new methods on Command and a new error type. This seems related, and is in some sense an alternative to implementing Try for ExitStatus.

[m-ou-se]

We discussed this in last week's libs-api meeting. We agreed with the comments above, that it doesn't seem like a great idea to implement Try for this type. The end result can get confusing, in part because of types like Result<ExitStatus, _> (double question marks?).

We prefer something more explicit, like the currently unstable ExitStatus::exit_ok(). We should find a better name for that method, though. (It starts with exit, but it doesn't exit anything. Maybe it should be to_result or something that includes success (because we already have .success()).)

[ratmice]

Alright, I'm going to close this then and leave a comment in #54889 that it should be closed too