Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

impl Try for ExitStatus #93565

Closed
wants to merge 2 commits into from
Closed

impl Try for ExitStatus #93565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94e31c9
impl Try for ExitStatus
ratmice Feb 1, 2022
f64d808
windows implementation of impl Try for ExitStatus.
ratmice Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@
#![feature(thin_box)]
#![feature(toowned_clone_into)]
#![feature(try_reserve_kind)]
#![feature(try_trait_v2)]
#![feature(vec_into_raw_parts)]
//
// Library features (unwind):
Expand Down
28 changes: 28 additions & 0 deletions library/std/src/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,12 @@ use crate::fmt;
use crate::fs;
use crate::io::{self, IoSlice, IoSliceMut};
use crate::num::NonZeroI32;
use crate::ops::{ControlFlow, FromResidual, Try};
use crate::path::Path;
use crate::str;
use crate::sys::pipe::{read2, AnonPipe};
use crate::sys::process as imp;

#[stable(feature = "command_access", since = "1.57.0")]
pub use crate::sys_common::process::CommandEnvs;
use crate::sys_common::{AsInner, AsInnerMut, FromInner, IntoInner};
Expand Down Expand Up @@ -1542,6 +1544,29 @@ impl fmt::Display for ExitStatus {
}
}

#[unstable(feature = "exit_status_error", issue = "84908")]
impl Try for ExitStatus {
type Output = ();
type Residual = Result<Infallible, ExitStatusError>;

fn branch(self) -> ControlFlow<Self::Residual, Self::Output> {
self.0.try_branch()
}

fn from_output((): ()) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ratmice ratmice Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Self(imp::ExitStatus::zero_status())
}
}

#[unstable(feature = "exit_status_error", issue = "84908")]
impl FromResidual<Result<Infallible, ExitStatusError>> for ExitStatus {
fn from_residual(residual: Result<Infallible, ExitStatusError>) -> Self {
match residual {
Err(exit_status_error) => exit_status_error.into(),
}
}
}

/// Allows extension traits within `std`.
#[unstable(feature = "sealed", issue = "none")]
impl crate::sealed::Sealed for ExitStatusError {}
Expand Down Expand Up @@ -1574,6 +1599,9 @@ pub struct ExitStatusError(imp::ExitStatusError);

#[unstable(feature = "exit_status_error", issue = "84908")]
impl ExitStatusError {
pub(crate) fn new(exit_status_error: imp::ExitStatusError) -> Self {
ExitStatusError(exit_status_error)
}
/// Reports the exit code, if applicable, from an `ExitStatusError`.
///
/// In Unix terms the return value is the **exit status**: the value passed to `exit`, if the
Expand Down
19 changes: 19 additions & 0 deletions library/std/src/sys/unix/process/process_unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use crate::convert::Infallible;
use crate::fmt;
use crate::io::{self, Error, ErrorKind};
use crate::mem;
use crate::num::NonZeroI32;
use crate::ops::ControlFlow;
use crate::ptr;
use crate::sys;
use crate::sys::cvt;
Expand Down Expand Up @@ -647,6 +649,12 @@ impl ExitStatus {
ExitStatus(status)
}

// Returns a new 0 status, to avoid hard coding zero
// in crate::process::ExitStatus.
pub fn zero_status() -> ExitStatus {
ExitStatus(0)
}

fn exited(&self) -> bool {
libc::WIFEXITED(self.0)
}
Expand All @@ -663,6 +671,17 @@ impl ExitStatus {
}
}

pub fn try_branch(
self,
) -> ControlFlow<Result<Infallible, crate::process::ExitStatusError>, ()> {
match NonZero_c_int::try_from(self.0) {
Ok(failure) => ControlFlow::Break(Err(crate::process::ExitStatusError::new(
ExitStatusError(failure),
))),
Err(_) => ControlFlow::Continue(()),
}
}

pub fn code(&self) -> Option<i32> {
self.exited().then(|| libc::WEXITSTATUS(self.0))
}
Expand Down
17 changes: 17 additions & 0 deletions library/std/src/sys/windows/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ mod tests;

use crate::cmp;
use crate::collections::BTreeMap;
use crate::convert::Infallible;
use crate::env;
use crate::env::consts::{EXE_EXTENSION, EXE_SUFFIX};
use crate::ffi::{OsStr, OsString};
use crate::fmt;
use crate::io::{self, Error, ErrorKind};
use crate::mem;
use crate::num::NonZeroI32;
use crate::ops::ControlFlow;
use crate::os::windows::ffi::{OsStrExt, OsStringExt};
use crate::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle, FromRawHandle, IntoRawHandle};
use crate::path::{Path, PathBuf};
Expand Down Expand Up @@ -675,6 +677,21 @@ impl ExitStatus {
pub fn code(&self) -> Option<i32> {
Some(self.0 as i32)
}

pub fn try_branch(
self,
) -> ControlFlow<Result<Infallible, crate::process::ExitStatusError>, ()> {
match NonZeroDWORD::try_from(self.0) {
Ok(failure) => ControlFlow::Break(Err(crate::process::ExitStatusError::new(
ExitStatusError(failure),
))),
Err(_) => ControlFlow::Continue(()),
}
}

pub fn zero_status() -> ExitStatus {
ExitStatus(0)
}
}

/// Converts a raw `c::DWORD` to a type-safe `ExitStatus` by wrapping it without copying.
Expand Down