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.

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

Implement ExitCodeExt for Windows #48

Closed
AronParker opened this issue Jun 10, 2022 · 5 comments
Closed

Implement ExitCodeExt for Windows #48

AronParker opened this issue Jun 10, 2022 · 5 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@AronParker
Copy link

AronParker commented Jun 10, 2022

Proposal

Problem statement

On Windows it is common for applications to return HRESULT (i32) or DWORD (u32) values. The newly stabilized ExitCode provides an excellent fit for propagating these values, because std::process::exit exits immediately and 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 proposal 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.

Motivation, use-cases

  • COM based components as well as newer Win32 based-functions make extensive use of HRESULT (i32) values, which is a universal success and error code on Windows designed to represent every error code from multiple facilities.

  • GUI applications use a message loop to process their events. On windows, once the WM_QUIT message has been received, the application is requested to return the exit code in wParam, which was originally sent using the PostQuitMessage function. It takes an i32, which is to be returned from main. Using std::process::exit can result in error conditions such as an unhandled exception 0x80000003 inside the ExitProcess function. In contrast, using ExitCode allows running all destructors and unregistering window classes which prevent the exception.

  • Win32 errors (system error codes) received by GetLastError are also popular to return as an exit code, which do not fit inside an u8.

Solution sketches

/// 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.
    ///
    /// The exit code should not be 259, as this conflicts with the `STILL_ACTIVE`
    /// macro returned from the `GetExitCodeProcess` function to signal that the
    /// process has yet to run to completion.
    #[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))
    }
}

Links and related work

Original suggestion
Implementation

@AronParker AronParker added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 10, 2022
@yaahc yaahc added the initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 16, 2022
@yaahc
Copy link
Member

yaahc commented Jun 16, 2022

Seconded

We've previously discussed the need for APIs like this as part of the ExitCode type's expected evolution. Between now and when this stabilizes, I believe we should do some holistic reasoning about how this API and similar APIs fit into our story of platform specific support and upcoming features like the portability lint or where Platform:, but for now I think it's a safe bet that we want something like this and we should start experimenting with it on nightly once the initial comment period ends.

@joshtriplett
Copy link
Member

I do think we should experiment with this on nightly, but I'd love to see us just add a From impl guarded by where TargetOs: Windows or similar.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 7, 2022
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 7, 2022
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
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
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
@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2023

Looks like this is already implemented. Closing.

@m-ou-se m-ou-se closed this as completed May 17, 2023
@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2023

Oh, no tracking issue was opened. Please open a tracking issue and add it to the relevant #[unstable] tags, otherwise it will likely never be stabilized.

@m-ou-se m-ou-se reopened this May 17, 2023
@m-ou-se m-ou-se added waiting-on-author and removed initial-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 17, 2023
@AronParker
Copy link
Author

Oh, no tracking issue was opened. Please open a tracking issue and add it to the relevant #[unstable] tags, otherwise it will likely never be stabilized.

Ah sorry, I created one now. Thanks for the info.

@dtolnay dtolnay closed this as completed Nov 23, 2023
@dtolnay dtolnay added ACP-accepted API Change Proposal is accepted (seconded with no objections) and removed waiting-on-author labels Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants