Stabilize Termination and ExitCode #93840

yaahc · 2022-02-09T23:46:45Z

This PR stabilizes the Termination trait and associated ExitCode type. It also adjusts the ExitCode feature flag to replace the placeholder flag with a more permanent name, as well as splitting off the to_i32 method behind its own permanently unstable feature flag.

This PR stabilizes the termination trait with the following signature:

pub trait Termination {
    fn report(self) -> ExitCode;
}

The existing impls of Termination are effectively already stable due to the prior stabilization of ? in main.

This PR also stabilizes the following APIs on exit code

#[derive(Clone, Copy, Debug)]
pub struct ExitCode(_);

impl ExitCode {
    pub const SUCCESS: ExitCode;
    pub const FAILURE: ExitCode;
}

impl From<u8> for ExitCode { /* ... */ }

All of the previous blockers have been resolved. The main ones that were resolved recently are:

The trait's name: We decided against changing this since none of the alternatives seemed particularly compelling. Instead we decided to end the bikeshedding and stick with the current name. (link to the discussion)
Issues around platform specific representations: We resolved this issue by changing the return type of report from i32 to the opaque type ExitCode. That way we can change the underlying representation without affecting the API, letting us offer full support for platform specific exit code APIs in the future.
Custom exit codes: We resolved this by adding From<u8> for ExitCode. We choose to only support u8 initially because it is the least common denominator between the sets of exit codes supported by our current platforms. In the future we anticipate adding platform specific extension traits to ExitCode for constructors from larger or negative numbers, as needed.

rust-highfive · 2022-02-09T23:46:49Z

r? @joshtriplett

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

yaahc · 2022-02-09T23:55:22Z

@rfcbot fcp merge

rfcbot · 2022-02-09T23:55:23Z

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Kixunil · 2022-02-10T10:12:34Z

Is there any reason why report takes self and not &self? ExitCode is Copy so I don't think people will need to move out of self.

Mark-Simulacrum · 2022-02-10T15:49:05Z

The current ExitCode is essentially write-only in the sense that once created, a consumer external to std cannot readily inspect whether it represents success or failure. If we add a trivial PartialEq impl, then consumers would be able to == against the SUCCESS or FAILURE constants, but this would either need some care to deal with other failure statuses (where FAILURE is 1, but someone has used From<u8> from 3) or unusual equality impls that consider everything nonzero equivalent.

If the intent is to leave the struct opaque indefinitely (essentially that it can only lead to process::exit in std) then that seems OK, but if the intent is to support manipulating ExitCodes then it seems less clear that the current design is the right one.

I am also a little concerned with potential confusion around ExitStatus vs. ExitCode -- they're both in std::process, with one for the result of running a Command and the other for (presumably) exiting from the current process. This distinction is somewhat subtle and I think it's worthwhile to add a paragraph to the documentation of each at least pointing at the other type.

The warning text on ExitCode should probably also be removed if we're stabilizing it ("Warning: While various forms of this were discussed in RFC #1937, it was ultimately cut from that RFC, and thus this type is more subject to change even than the usual unstable item churn.")

mathstuf · 2022-02-10T19:27:58Z

I am also a little concerned with potential confusion around ExitStatus vs. ExitCode

The key difference is that ExitStatus contains cases not under the process' direct control such as "killed by a signal" that a process cannot choose for itself. I think making that detail explicit should help clarify why there are two types involved (if not why they are so similar).

If we add a trivial PartialEq impl, then consumers would be able to == against the SUCCESS or FAILURE constants

How about just having a fn success() -> bool method? Maybe .is_success(), but .success() matches ExitStatus (for better or worse).

Kixunil · 2022-02-10T20:41:30Z

Agree with having a method but since in my mind ExitCode is very different from ExitStatus I think is_success() would be better (no need to use same name, is_ is idiomatic).

yaahc · 2022-02-18T20:46:18Z

r? @Mark-Simulacrum

I've gone ahead and pushed a new commit to make the recommended docs updates.

If the intent is to leave the struct opaque indefinitely (essentially that it can only lead to process::exit in std) then that seems OK, but if the intent is to support manipulating ExitCodes then it seems less clear that the current design is the right one.

I think my preference would be to keep the struct opaque indefinitely. I can't think of any reason why someone would want to react to specific ExitCodes, given that they can only be used in Termination::report, so there should be few to no situations where a user works with an ExitCode that they did not create themselves.

library/std/src/process.rs

Mark-Simulacrum · 2022-02-22T20:28:30Z

r=me modulo last small nit and FCP completing; would also be good to squash commits.

rfcbot · 2022-03-29T12:24:42Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

joshtriplett · 2022-03-29T17:30:15Z

@bors r+

bors · 2022-03-29T17:30:17Z

📌 Commit 97c58e8 has been approved by joshtriplett

Rollup of 4 pull requests Successful merges: - rust-lang#93840 (Stabilize Termination and ExitCode) - rust-lang#95256 (Ensure io::Error's bitpacked repr doesn't accidentally impl UnwindSafe) - rust-lang#95386 (Suggest wrapping patterns in enum variants) - rust-lang#95437 (diagnostics: regression test for derive bounds) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

ehuss · 2022-03-29T23:54:32Z

The stabilization version is listed as 1.60, but I believe it should be 1.61, correct?

yaahc · 2022-03-30T00:08:15Z

The stabilization version is listed as 1.60, but I believe it should be 1.61, correct?

shit, yes. I'll fix that right now

ehuss · 2022-03-30T00:18:15Z

@yaahc Would you be willing to help update the documentation for this? Some places I can think of:

Reference main return type: https://github.com/rust-lang/reference/blob/master/src/crates-and-source-files.md#main-functions
Reference #[test] return type: https://github.com/rust-lang/reference/blob/master/src/attributes/testing.md#the-test-attribute

Rustc testing chapter:

rust/src/doc/rustc/src/tests/index.md

Lines 17 to 18 in 9c06e1b

    
           Tests "pass" if they return without an error. They "fail" if they [panic], or 
        
           return a [`Result`] with an error.

Book: https://github.com/rust-lang/book/blob/ea90bbaf53ba64ef4e2da9ac2352b298aec6bec8/src/ch09-02-recoverable-errors-with-result.md?plain=1#L516-L520
- (Note: They may prefer just opening an issue versus a PR.)

I can do the latter two, but not the former two (me being the reviewer there).

…n, r=ehuss fix since field version for termination stabilization fixes incorrect version fields in stabilization of rust-lang#93840 r? `@ehuss`

docs: Update tests chapter for Termination stabilization A small update for the docs of `#[test]` functions as a result of the `Termination` stabilization in rust-lang#93840.

…r=<try> ExitCode::exit_process() method cc `@yaahc` / rust-lang#93840 (eeek, hit ctrl-enter before I meant to and right after realizing the branch name was wrong. oh, well) I feel like it makes sense to have the `exit(ExitCode)` function as a method or at least associated function on ExitCode, but maybe that would hurt discoverability? Probably not as much if it's at the top of the `process::exit()` documentation or something, but idk. Also very unsure about the name, I'd like something that communicates that you are exiting with *this* ExitCode, but with a method name being postfix it doesn't seem to flow. `code.exit_process_with()` ? `.exit_process_with_self()` ? Blech. Maybe it doesn't matter, since ideally just `code.exit()` or something would be clear simply by the name and single parameter but 🤷 Also I'd like to touch up the `ExitCode` docs (which I did a bit here), but that would probably be good in a separate PR, right? Since I think the beta deadline is coming up.

Implement ExitCodeExt for Windows Fixes rust-lang#97914 ### Motivation: On Windows it is common for applications to return `HRESULT` (`i32`) or `DWORD` (`u32`) values. These stem from COM based components ([HRESULTS](https://docs.microsoft.com/en-us/windows/win32/api/objbase/nf-objbase-coinitialize)), Win32 errors ([GetLastError](https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror)), GUI applications ([WM_QUIT](https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-quit)) and more. The newly stabilized `ExitCode` provides an excellent fit for propagating these values, because `std::process::exit` does not run deconstructors which can result in errors. However, `ExitCode` currently only implements `From<u8> for ExitCode`, which disallows the full range of `i32`/`u32` values. This pull requests attempts to address that shortcoming by providing windows specific extensions that accept a `u32` value (which covers all possible `HRESULTS` and Win32 errors) analog to [ExitStatusExt::from_raw](https://doc.rust-lang.org/std/os/windows/process/trait.ExitStatusExt.html#tymethod.from_raw). This was also intended by the original Stabilization rust-lang#93840 (comment) as pointed out by `@eggyal` in rust-lang#97914 (comment): > Issues around platform specific representations: We resolved this issue by changing the return type of report from i32 to the opaque type ExitCode. __That way we can change the underlying representation without affecting the API, letting us offer full support for platform specific exit code APIs in the future.__ [Emphasis added] ### API ```rust /// Windows-specific extensions to [`process::ExitCode`]. /// /// This trait is sealed: it cannot be implemented outside the standard library. /// This is so that future additional methods are not breaking changes. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] pub trait ExitCodeExt: Sealed { /// Creates a new `ExitCode` from the raw underlying `u32` return value of /// a process. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] fn from_raw(raw: u32) -> Self; } #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] impl ExitCodeExt for process::ExitCode { fn from_raw(raw: u32) -> Self { process::ExitCode::from_inner(From::from(raw)) } } ``` ### Misc I apologize in advance if I misplaced any attributes regarding stabilzation, as far as I learned traits are insta-stable so I chose to make them stable. If this is an error, please let me know and I'll correct it. I also added some additional machinery to make it work, analog to [ExitStatus](https://doc.rust-lang.org/std/process/struct.ExitStatus.html#). EDIT: Proposal: rust-lang/libs-team#48

Implement ExitCodeExt for Windows Fixes rust-lang#97914 ### Motivation: On Windows it is common for applications to return `HRESULT` (`i32`) or `DWORD` (`u32`) values. These stem from COM based components ([HRESULTS](https://docs.microsoft.com/en-us/windows/win32/api/objbase/nf-objbase-coinitialize)), Win32 errors ([GetLastError](https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror)), GUI applications ([WM_QUIT](https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-quit)) and more. The newly stabilized `ExitCode` provides an excellent fit for propagating these values, because `std::process::exit` does not run deconstructors which can result in errors. However, `ExitCode` currently only implements `From<u8> for ExitCode`, which disallows the full range of `i32`/`u32` values. This pull requests attempts to address that shortcoming by providing windows specific extensions that accept a `u32` value (which covers all possible `HRESULTS` and Win32 errors) analog to [ExitStatusExt::from_raw](https://doc.rust-lang.org/std/os/windows/process/trait.ExitStatusExt.html#tymethod.from_raw). This was also intended by the original Stabilization rust-lang#93840 (comment) as pointed out by ``@eggyal`` in rust-lang#97914 (comment): > Issues around platform specific representations: We resolved this issue by changing the return type of report from i32 to the opaque type ExitCode. __That way we can change the underlying representation without affecting the API, letting us offer full support for platform specific exit code APIs in the future.__ [Emphasis added] ### API ```rust /// Windows-specific extensions to [`process::ExitCode`]. /// /// This trait is sealed: it cannot be implemented outside the standard library. /// This is so that future additional methods are not breaking changes. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] pub trait ExitCodeExt: Sealed { /// Creates a new `ExitCode` from the raw underlying `u32` return value of /// a process. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] fn from_raw(raw: u32) -> Self; } #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] impl ExitCodeExt for process::ExitCode { fn from_raw(raw: u32) -> Self { process::ExitCode::from_inner(From::from(raw)) } } ``` ### Misc I apologize in advance if I misplaced any attributes regarding stabilzation, as far as I learned traits are insta-stable so I chose to make them stable. If this is an error, please let me know and I'll correct it. I also added some additional machinery to make it work, analog to [ExitStatus](https://doc.rust-lang.org/std/process/struct.ExitStatus.html#). EDIT: Proposal: rust-lang/libs-team#48

Implement ExitCodeExt for Windows Fixes #97914 ### Motivation: On Windows it is common for applications to return `HRESULT` (`i32`) or `DWORD` (`u32`) values. These stem from COM based components ([HRESULTS](https://docs.microsoft.com/en-us/windows/win32/api/objbase/nf-objbase-coinitialize)), Win32 errors ([GetLastError](https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror)), GUI applications ([WM_QUIT](https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-quit)) and more. The newly stabilized `ExitCode` provides an excellent fit for propagating these values, because `std::process::exit` does not run deconstructors which can result in errors. However, `ExitCode` currently only implements `From<u8> for ExitCode`, which disallows the full range of `i32`/`u32` values. This pull requests attempts to address that shortcoming by providing windows specific extensions that accept a `u32` value (which covers all possible `HRESULTS` and Win32 errors) analog to [ExitStatusExt::from_raw](https://doc.rust-lang.org/std/os/windows/process/trait.ExitStatusExt.html#tymethod.from_raw). This was also intended by the original Stabilization rust-lang/rust#93840 (comment) as pointed out by ``@eggyal`` in rust-lang/rust#97914 (comment): > Issues around platform specific representations: We resolved this issue by changing the return type of report from i32 to the opaque type ExitCode. __That way we can change the underlying representation without affecting the API, letting us offer full support for platform specific exit code APIs in the future.__ [Emphasis added] ### API ```rust /// Windows-specific extensions to [`process::ExitCode`]. /// /// This trait is sealed: it cannot be implemented outside the standard library. /// This is so that future additional methods are not breaking changes. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] pub trait ExitCodeExt: Sealed { /// Creates a new `ExitCode` from the raw underlying `u32` return value of /// a process. #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] fn from_raw(raw: u32) -> Self; } #[stable(feature = "windows_process_exit_code_from", since = "1.63.0")] impl ExitCodeExt for process::ExitCode { fn from_raw(raw: u32) -> Self { process::ExitCode::from_inner(From::from(raw)) } } ``` ### Misc I apologize in advance if I misplaced any attributes regarding stabilzation, as far as I learned traits are insta-stable so I chose to make them stable. If this is an error, please let me know and I'll correct it. I also added some additional machinery to make it work, analog to [ExitStatus](https://doc.rust-lang.org/std/process/struct.ExitStatus.html#). EDIT: Proposal: rust-lang/libs-team#48

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 9, 2022

rust-highfive assigned joshtriplett Feb 9, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2022

yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 9, 2022

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 9, 2022

yaahc mentioned this pull request Feb 9, 2022

Tracking issue for RFC 1937: ? in main #43301

Closed

12 tasks

rust-highfive assigned Mark-Simulacrum and unassigned joshtriplett Feb 18, 2022

Mark-Simulacrum reviewed Feb 21, 2022

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

library/std/src/process.rs Show resolved Hide resolved

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2022

yaahc added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2022

Mark-Simulacrum reviewed Feb 22, 2022

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

Stabilize Termination and ExitCode

7bdad89

yaahc force-pushed the termination-stabilization-celebration-station branch from b5ea6c7 to 7bdad89 Compare February 22, 2022 20:40

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 29, 2022

Dylan-DPC mentioned this pull request Mar 29, 2022

Rollup of 4 pull requests #95448

Merged

bors merged commit bba2a64 into rust-lang:master Mar 29, 2022

rustbot added this to the 1.61.0 milestone Mar 29, 2022

yaahc mentioned this pull request Mar 30, 2022

fix since field version for termination stabilization #95452

Merged

jplatte mentioned this pull request Mar 30, 2022

Tracking issue for std::process::ExitCode (feature process_exitcode_placeholder) #48711

Closed

1 task

apiraino removed the to-announce Announce this issue on triage meeting label Mar 31, 2022

ehuss mentioned this pull request Apr 14, 2022

docs: Update tests chapter for Termination stabilization #96062

Merged

eggyal mentioned this pull request Jun 9, 2022

Suggestion: Add ExitCodeExt for Windows #97914

Closed

AronParker mentioned this pull request Jun 9, 2022

Implement ExitCodeExt for Windows #97917

Merged

AronParker mentioned this pull request May 17, 2023

Tracking Issue for ExitCodeExt on Windows #111688

Open

5 tasks

ehuss mentioned this pull request May 16, 2024

std: make ExitCode Eq #125182

Closed

workingjubilee mentioned this pull request Jul 12, 2024

[library/std/src/process.rs] PartialEq for ExitCode #127633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize Termination and ExitCode #93840

Stabilize Termination and ExitCode #93840

yaahc commented Feb 9, 2022

rust-highfive commented Feb 9, 2022

yaahc commented Feb 9, 2022

rfcbot commented Feb 9, 2022 •

edited by BurntSushi

Loading

Kixunil commented Feb 10, 2022

Mark-Simulacrum commented Feb 10, 2022

mathstuf commented Feb 10, 2022

Kixunil commented Feb 10, 2022

yaahc commented Feb 18, 2022 •

edited

Loading

Mark-Simulacrum commented Feb 22, 2022 •

edited

Loading

rfcbot commented Mar 29, 2022

joshtriplett commented Mar 29, 2022

bors commented Mar 29, 2022

ehuss commented Mar 29, 2022

yaahc commented Mar 30, 2022

ehuss commented Mar 30, 2022

Stabilize Termination and ExitCode #93840

Stabilize Termination and ExitCode #93840

Conversation

yaahc commented Feb 9, 2022

rust-highfive commented Feb 9, 2022

yaahc commented Feb 9, 2022

rfcbot commented Feb 9, 2022 • edited by BurntSushi Loading

Kixunil commented Feb 10, 2022

Mark-Simulacrum commented Feb 10, 2022

mathstuf commented Feb 10, 2022

Kixunil commented Feb 10, 2022

yaahc commented Feb 18, 2022 • edited Loading

Mark-Simulacrum commented Feb 22, 2022 • edited Loading

rfcbot commented Mar 29, 2022

joshtriplett commented Mar 29, 2022

bors commented Mar 29, 2022

ehuss commented Mar 29, 2022

yaahc commented Mar 30, 2022

ehuss commented Mar 30, 2022

rfcbot commented Feb 9, 2022 •

edited by BurntSushi

Loading

yaahc commented Feb 18, 2022 •

edited

Loading

Mark-Simulacrum commented Feb 22, 2022 •

edited

Loading