From 61273b7e642979ee25b669fd7dea05051ed6520d Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Sun, 27 Mar 2022 15:41:13 -0700 Subject: [PATCH 1/5] Define a dedicated error type for `HandleOrNull` and `HandleOrInvalid`. Define a `NotHandle` type, that implements `std::error::Error`, and use it as the error type in `HandleOrNull` and `HandleOrInvalid`. --- library/std/src/error.rs | 4 ++++ library/std/src/os/windows/io/handle.rs | 26 +++++++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/library/std/src/error.rs b/library/std/src/error.rs index c3cb71a5dee63..5e51dbf87549b 100644 --- a/library/std/src/error.rs +++ b/library/std/src/error.rs @@ -604,6 +604,10 @@ impl Error for alloc::collections::TryReserveError {} #[unstable(feature = "duration_checked_float", issue = "83400")] impl Error for time::FromFloatSecsError {} +#[cfg(windows)] +#[unstable(feature = "io_safety", issue = "87074")] +impl Error for crate::os::windows::io::NotHandle {} + // Copied from `any.rs`. impl dyn Error + 'static { /// Returns `true` if the inner type is the same as `T`. diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 120af9f99dd9b..4003f04731f7d 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -142,17 +142,17 @@ impl BorrowedHandle<'_> { } impl TryFrom for OwnedHandle { - type Error = (); + type Error = NotHandle; #[inline] - fn try_from(handle_or_null: HandleOrNull) -> Result { + fn try_from(handle_or_null: HandleOrNull) -> Result { let owned_handle = handle_or_null.0; if owned_handle.handle.is_null() { // Don't call `CloseHandle`; it'd be harmless, except that it could // overwrite the `GetLastError` error. forget(owned_handle); - Err(()) + Err(NotHandle(())) } else { Ok(owned_handle) } @@ -200,23 +200,37 @@ impl OwnedHandle { } impl TryFrom for OwnedHandle { - type Error = (); + type Error = NotHandle; #[inline] - fn try_from(handle_or_invalid: HandleOrInvalid) -> Result { + fn try_from(handle_or_invalid: HandleOrInvalid) -> Result { let owned_handle = handle_or_invalid.0; if owned_handle.handle == c::INVALID_HANDLE_VALUE { // Don't call `CloseHandle`; it'd be harmless, except that it could // overwrite the `GetLastError` error. forget(owned_handle); - Err(()) + Err(NotHandle(())) } else { Ok(owned_handle) } } } +/// This is the error type used by [`HandleOrInvalid`] and +/// [`HandleOrNull`] when attempting to convert into a handle, +/// to indicate that the value is not a handle. +#[unstable(feature = "io_safety", issue = "87074")] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct NotHandle(()); + +#[unstable(feature = "io_safety", issue = "87074")] +impl fmt::Display for NotHandle { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + "the return value of a Windows API call indicated an error".fmt(fmt) + } +} + impl AsRawHandle for BorrowedHandle<'_> { #[inline] fn as_raw_handle(&self) -> RawHandle { From c7e0ef5cabbdf42e54b69ec1760835ad2d611c02 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Sun, 27 Mar 2022 16:53:56 -0700 Subject: [PATCH 2/5] Fix an incorrect word in a comment. --- library/std/src/os/windows/io/handle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 4003f04731f7d..f27ea785586fa 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -75,7 +75,7 @@ pub struct OwnedHandle { /// `NULL`. This ensures that such FFI calls cannot start using the handle without /// checking for `NULL` first. /// -/// This type concerns any value other than `NULL` to be valid, including `INVALID_HANDLE_VALUE`. +/// This type considers any value other than `NULL` to be valid, including `INVALID_HANDLE_VALUE`. /// This is because APIs that use `NULL` as their sentry value don't treat `INVALID_HANDLE_VALUE` /// as special. /// @@ -95,7 +95,7 @@ pub struct HandleOrNull(OwnedHandle); /// `INVALID_HANDLE_VALUE`. This ensures that such FFI calls cannot start using the handle without /// checking for `INVALID_HANDLE_VALUE` first. /// -/// This type concerns any value other than `INVALID_HANDLE_VALUE` to be valid, including `NULL`. +/// This type considers any value other than `INVALID_HANDLE_VALUE` to be valid, including `NULL`. /// This is because APIs that use `INVALID_HANDLE_VALUE` as their sentry value may return `NULL` /// under `windows_subsystem = "windows"` or other situations where I/O devices are detached. /// From fc3b8b34ea5f68180dc1d776df99357c00fe135b Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Sun, 27 Mar 2022 16:54:58 -0700 Subject: [PATCH 3/5] Move the `Error` impl for `NotHandle` out of platform-independent code. --- library/std/src/error.rs | 4 ---- library/std/src/os/windows/io/handle.rs | 3 +++ 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/library/std/src/error.rs b/library/std/src/error.rs index 5e51dbf87549b..c3cb71a5dee63 100644 --- a/library/std/src/error.rs +++ b/library/std/src/error.rs @@ -604,10 +604,6 @@ impl Error for alloc::collections::TryReserveError {} #[unstable(feature = "duration_checked_float", issue = "83400")] impl Error for time::FromFloatSecsError {} -#[cfg(windows)] -#[unstable(feature = "io_safety", issue = "87074")] -impl Error for crate::os::windows::io::NotHandle {} - // Copied from `any.rs`. impl dyn Error + 'static { /// Returns `true` if the inner type is the same as `T`. diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index f27ea785586fa..32e454cee7edc 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -231,6 +231,9 @@ impl fmt::Display for NotHandle { } } +#[unstable(feature = "io_safety", issue = "87074")] +impl crate::error::Error for NotHandle {} + impl AsRawHandle for BorrowedHandle<'_> { #[inline] fn as_raw_handle(&self) -> RawHandle { From 0efbd34c7e0c5064e7df977189c9950b1074706b Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Mon, 28 Mar 2022 10:56:00 -0700 Subject: [PATCH 4/5] Split `NotHandle` into `NullHandleError` and `InvalidHandleError`. Also, make the display messages more specific, and remove the `Copy` implementation. --- library/std/src/os/windows/io/handle.rs | 45 +++++++++++++++++-------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 32e454cee7edc..707b8a301727c 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -142,17 +142,17 @@ impl BorrowedHandle<'_> { } impl TryFrom for OwnedHandle { - type Error = NotHandle; + type Error = NullHandleError; #[inline] - fn try_from(handle_or_null: HandleOrNull) -> Result { + fn try_from(handle_or_null: HandleOrNull) -> Result { let owned_handle = handle_or_null.0; if owned_handle.handle.is_null() { // Don't call `CloseHandle`; it'd be harmless, except that it could // overwrite the `GetLastError` error. forget(owned_handle); - Err(NotHandle(())) + Err(NullHandleError(())) } else { Ok(owned_handle) } @@ -200,39 +200,56 @@ impl OwnedHandle { } impl TryFrom for OwnedHandle { - type Error = NotHandle; + type Error = InvalidHandleError; #[inline] - fn try_from(handle_or_invalid: HandleOrInvalid) -> Result { + fn try_from(handle_or_invalid: HandleOrInvalid) -> Result { let owned_handle = handle_or_invalid.0; if owned_handle.handle == c::INVALID_HANDLE_VALUE { // Don't call `CloseHandle`; it'd be harmless, except that it could // overwrite the `GetLastError` error. forget(owned_handle); - Err(NotHandle(())) + Err(InvalidHandleError(())) } else { Ok(owned_handle) } } } -/// This is the error type used by [`HandleOrInvalid`] and -/// [`HandleOrNull`] when attempting to convert into a handle, -/// to indicate that the value is not a handle. +/// This is the error type used by [`HandleOrNull`] when attempting to convert +/// into a handle, to indicate that the value is null. #[unstable(feature = "io_safety", issue = "87074")] -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub struct NotHandle(()); +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NullHandleError(()); #[unstable(feature = "io_safety", issue = "87074")] -impl fmt::Display for NotHandle { +impl fmt::Display for NullHandleError { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { - "the return value of a Windows API call indicated an error".fmt(fmt) + "A HandleOrNull could not be converted to a handle because it was null".fmt(fmt) } } #[unstable(feature = "io_safety", issue = "87074")] -impl crate::error::Error for NotHandle {} +impl crate::error::Error for NullHandleError {} + +/// This is the error type used by [`HandleOrInvalid`] when attempting to +/// convert into a handle, to indicate that the value is +/// `INVALID_HANDLE_VALUE`. +#[unstable(feature = "io_safety", issue = "87074")] +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidHandleError(()); + +#[unstable(feature = "io_safety", issue = "87074")] +impl fmt::Display for InvalidHandleError { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + "A HandleOrInvalid could not be converted to a handle because it was INVALID_HANDLE_VALUE" + .fmt(fmt) + } +} + +#[unstable(feature = "io_safety", issue = "87074")] +impl crate::error::Error for InvalidHandleError {} impl AsRawHandle for BorrowedHandle<'_> { #[inline] From 733ef089d9f59234e93f84dbe94472f0f9f2a34b Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 13 Apr 2022 14:32:17 -0700 Subject: [PATCH 5/5] Add a comment explaining the `(())` idiom for empty structs. --- library/std/src/os/windows/io/handle.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 707b8a301727c..1408b2e83f87a 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -219,6 +219,7 @@ impl TryFrom for OwnedHandle { /// This is the error type used by [`HandleOrNull`] when attempting to convert /// into a handle, to indicate that the value is null. +// The empty field prevents constructing this, and allows extending it in the future. #[unstable(feature = "io_safety", issue = "87074")] #[derive(Debug, Clone, PartialEq, Eq)] pub struct NullHandleError(()); @@ -236,6 +237,7 @@ impl crate::error::Error for NullHandleError {} /// This is the error type used by [`HandleOrInvalid`] when attempting to /// convert into a handle, to indicate that the value is /// `INVALID_HANDLE_VALUE`. +// The empty field prevents constructing this, and allows extending it in the future. #[unstable(feature = "io_safety", issue = "87074")] #[derive(Debug, Clone, PartialEq, Eq)] pub struct InvalidHandleError(());