Implement ExitCodeExt for Windows

[AronParker]

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), Win32 errors (GetLastError), GUI applications (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.

This was also intended by the original Stabilization #93840 (comment) as pointed out by @eggyal in #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

/// 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.

EDIT: Proposal: rust-lang/libs-team#48

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

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

[thomcc]

@rustbot label +T-libs-api -T-libs

I don't really know what pitfalls may exist here, so punting the review to @ChrisDenton, who probably does.

r? @ChrisDenton

[ChrisDenton]

The idea looks reasonable to me but there is one pitfall I can think of. An exit code of 259 can be taken to mean the process has not exited yet. See GetExitCodeProcess. When checking if a process is still active it is more robust to wait on the process handle instead but you can't assume a foreign process will do that.

I'll admit to being uncertain if this is something we want to design around. On the one hand it may be a hazard in the general case. On the other hand, there's nothing wrong with returning 259 if you're in an environment where you know it's not going to be misinterpreted. At the very least I think the docs should warn about this case.

[ChrisDenton]

Also cc @yaahc.

[AronParker]

That's a good idea, I did not think of that. I added a warning to the documentation for this potential ambiguity.

I believe we should not design around it, if one wants to request the "real" exit code they can always use WaitForSingleObject in conjunction with GetExitCodeProcess so that STILL_ACTIVE / STATUS_PENDING could be used as an ordinary exit code.

[ChrisDenton]

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 think you may be mistaken here and these should be marked as unstable.

[AronParker]

I think you may be mistaken here and these should be marked as unstable.

In that case, should I create a tracking issue for this feature or just repurpose the original issue as a tracking issue?

[ChrisDenton]

Often "none" is used initially but in this case you can use the original issue number for now. You should probably wait for t-libs-api to sign off on the feature before making a tracking issue.

[thomcc]

Yes, new traits aren't insta-stable, it's only new impls of existing ones.

That said, I think you're supposed to file a proposals for new APIs in https://github.com/rust-lang/libs-team now -- see here for info on the process.

Here's a link to the template, I think for the you cover most of the important parts with the PR description, although you may have to move things around: https://github.com/rust-lang/libs-team/issues/new?assignees=&labels=api-change-proposal%2C+T-libs-api&template=api-change-proposal.md&title=%28My+API+Change+Proposal%29

(I hadn't realized we actually went forward with this, I think it just started this week, sorry that you have to be one of the guinea-pigs 😅).

[AronParker]

Yes, new traits aren't insta-stable, it's only new impls of existing ones.

Ah I see, thank you for clarifying this.

That said, I think you're supposed to file a proposals for new APIs in https://github.com/rust-lang/libs-team now -- see here for info on the process.

Here's a link to the template, I think for the you cover most of the important parts with the PR description, although you may have to move things around: https://github.com/rust-lang/libs-team/issues/new?assignees=&labels=api-change-proposal%2C+T-libs-api&template=api-change-proposal.md&title=%28My+API+Change+Proposal%29

I submitted a proposal right here which includes all original data from this request and more: rust-lang/libs-team#48

(I hadn't realized we actually went forward with this, I think it just started this week, sorry that you have to be one of the guinea-pigs 😅).

No problem! I understand that Rust is a large project that requires some organizational effort to manage its popularity. I'm thankful that you people take your time to review my request.

[ChrisDenton]

Ok, this ACP process is new but it looks like it's had the time and the libs-api team is in favour of having this in nightly. So I'm going to go ahead and approve.

[ChrisDenton]

@bors r+ rollup

[bors]

📌 Commit b13af73 has been approved by ChrisDenton