From 0869c275c5fc6419f595938d5d130561a60c916c Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 12:56:04 -0700 Subject: [PATCH 01/15] snapshot --- crates/libs/result/Cargo.toml | 6 +- crates/libs/result/src/bstr.rs | 50 +++ crates/libs/result/src/error.rs | 398 ++++++++++++++------ crates/libs/result/src/hresult.rs | 173 ++++++++- crates/libs/result/src/lib.rs | 18 +- crates/libs/result/src/strings.rs | 49 --- crates/libs/result/src/tests.rs | 18 + crates/tests/implement/tests/data_object.rs | 2 +- crates/tests/result/tests/error.rs | 23 +- 9 files changed, 538 insertions(+), 199 deletions(-) create mode 100644 crates/libs/result/src/bstr.rs create mode 100644 crates/libs/result/src/tests.rs diff --git a/crates/libs/result/Cargo.toml b/crates/libs/result/Cargo.toml index f553397c3c..971d7d89b1 100644 --- a/crates/libs/result/Cargo.toml +++ b/crates/libs/result/Cargo.toml @@ -11,8 +11,9 @@ readme = "readme.md" categories = ["os::windows-apis"] [features] -default = ["std"] +default = ["std", "error-info"] std = [] +error-info = [] [lints] workspace = true @@ -21,6 +22,9 @@ workspace = true default-target = "x86_64-pc-windows-msvc" targets = [] +[dependencies] +static_assertions = "1.0" + [dependencies.windows-targets] version = "0.52.5" path = "../targets" diff --git a/crates/libs/result/src/bstr.rs b/crates/libs/result/src/bstr.rs new file mode 100644 index 0000000000..700e4d7cb8 --- /dev/null +++ b/crates/libs/result/src/bstr.rs @@ -0,0 +1,50 @@ +use super::*; + +#[repr(transparent)] +pub struct BasicString(*const u16); + +impl BasicString { + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + pub fn len(&self) -> usize { + if self.0.is_null() { + 0 + } else { + unsafe { SysStringLen(self.0) as usize } + } + } + + pub fn as_wide(&self) -> &[u16] { + let len = self.len(); + if len != 0 { + unsafe { core::slice::from_raw_parts(self.as_ptr(), len) } + } else { + &[] + } + } + + pub fn as_ptr(&self) -> *const u16 { + if !self.is_empty() { + self.0 + } else { + const EMPTY: [u16; 1] = [0]; + EMPTY.as_ptr() + } + } +} + +impl Default for BasicString { + fn default() -> Self { + Self(core::ptr::null_mut()) + } +} + +impl Drop for BasicString { + fn drop(&mut self) { + if !self.0.is_null() { + unsafe { SysFreeString(self.0) } + } + } +} diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index 88a2c697a5..e2d76bcbda 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -1,52 +1,245 @@ use super::*; -use core::ffi::c_void; +#[allow(unused_imports)] +use core::mem::size_of; /// An error object consists of both an error code as well as detailed error information for debugging. -#[derive(Clone, PartialEq, Eq)] -pub struct Error { - code: HRESULT, - #[cfg(windows)] - info: Option, +/// +/// # Extended error info and the `error-info` feature +/// +/// The `Error` contains an `HRESULT` value that describes the error, as well as an optional +/// `IErrorInfo` object. The `IErrorInfo` object is a COM object that can provide detailed information +/// about an error, such as a text string, a `ProgID` of the originator, etc. If the error object +/// was originated in an WinRT component, then additional information such as a stack track may be +/// captured. +/// +/// However, many systems based on COM do not use `IErrorInfo`. For these systems, the optional error +/// info within `Error` has no benefits, but has substantial costs because it increases the size of +/// the `Error` object, which also increases the size of `Result`. +/// +/// The `windows-result` crate provides the `error-info` feature, which gives developers control over +/// the costs of using `Error`. +#[derive(Clone)] +pub struct ErrorT +where + Detail: ErrorDetail, +{ + code: NonZeroHRESULT, + + /// Contains details about the error, such as error text. + /// + /// Note; We cannot use `Option` here, because that would increase the size of `Error` + /// even when `Detail` contains no information at all (a ZST). The `Detail` type itself must + /// be a ZST for `Error` to be a slim type. + detail: Detail, } -impl Error { - /// Creates an error object without any failure information. - pub const fn empty() -> Self { - Self { - code: HRESULT(0), - #[cfg(windows)] - info: None, +/// An error object consists of both an error code as well as detailed error information for debugging. +/// +/// # Extended error info and the `error-info` feature +/// +/// The `Error` contains an `HRESULT` value that describes the error, as well as an optional +/// `IErrorInfo` object. The `IErrorInfo` object is a COM object that can provide detailed information +/// about an error, such as a text string, a `ProgID` of the originator, etc. If the error object +/// was originated in an WinRT component, then additional information such as a stack track may be +/// captured. +/// +/// However, many systems based on COM do not use `IErrorInfo`. For these systems, the optional error +/// info within `Error` has no benefits, but has substantial costs because it increases the size of +/// the `Error` object, which also increases the size of `Result`. +/// +/// The `windows-result` crate provides the `error-info` feature, which gives developers control over +/// the costs of using `Error`. +pub type Error = ErrorT; + +/// Specifies the default error detail for the `Error` type. +#[cfg(all(windows, feature = "error-info"))] +pub type DefaultDetail = ErrorInfo; + +/// Specifies the default error detail for the `Error` type. +#[cfg(not(all(windows, feature = "error-info")))] +pub type DefaultDetail = NoDetail; + +/// This type implements `ErrorDetail`, by containing no error information at all. This type allows +/// for "slim" `Error` objects, which have the same size as `HRESULT`. +#[derive(Default, Clone)] +pub struct NoDetail; + +/// Defines the behavior of error detail objects, which are stored within `Error`. +pub trait ErrorDetail: Sized { + /// Returns an empty error detail. + fn empty() -> Self; + + /// If this implementation stores error detail in ambient (thread-local) storage, then this + /// function transfers the error detail from ambient storage and returns it. + fn from_ambient() -> Self; + + /// If this implementation stores error detail in ambient (thread-local) storage, then this + /// function tranfers `self` into that ambient storage. + /// + /// If this implementation does not store error detail in ambient storage, then this function + /// discards the error detail. + fn into_ambient(self); + + /// If this implementation stores error detail in ambient (thread-local) storage, then this + /// function creates a new error detail object, given the `HRESULT` and a message string, + /// and stores the new error detail object in the ambient storage. + fn originate_error(code: HRESULT, message: &str); + + /// Returns a message describing the error, if available. + fn message(&self) -> Option; +} + +impl ErrorDetail for NoDetail { + fn empty() -> Self { + Self + } + fn from_ambient() -> Self { + Self + } + fn into_ambient(self) {} + fn originate_error(_code: HRESULT, _message: &str) {} + fn message(&self) -> Option { + None + } +} + +#[cfg(windows)] +pub use win_impl::ErrorInfo; + +#[cfg(windows)] +mod win_impl { + use super::*; + use crate::bstr::BasicString; + use crate::com::ComPtr; + + /// This type stores error detail, represented by a COM `IErrorInfo` object. + /// + /// # References + /// + /// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) + #[derive(Clone, Default)] + pub struct ErrorInfo { + pub(super) ptr: Option, + } + + unsafe impl Send for ErrorInfo {} + unsafe impl Sync for ErrorInfo {} + + impl ErrorInfo { + /// Gets a raw pointer to the `IErrorInfo` COM object stored in this `ErrorInfo`. + pub fn as_ptr(&self) -> *mut core::ffi::c_void { + self.ptr + .as_ref() + .map_or(core::ptr::null_mut(), |info| info.as_raw()) } } - /// Creates a new error object, capturing the stack and other information about the - /// point of failure. - pub fn new>(code: HRESULT, message: T) -> Self { - #[cfg(windows)] - { - let message: Vec<_> = message.as_ref().encode_utf16().collect(); + impl ErrorDetail for ErrorInfo { + fn empty() -> Self { + Self { ptr: None } + } + + fn from_ambient() -> Self { + unsafe { + let mut ptr = None; + GetErrorInfo(0, &mut ptr as *mut _ as _); + Self { ptr } + } + } + + fn into_ambient(self) { + if let Some(ptr) = self.ptr { + unsafe { + SetErrorInfo(0, ptr.as_raw()); + } + } + } + + fn originate_error(code: HRESULT, message: &str) { + if message.is_empty() { + return; + } + + let mut wide_message: Vec = Vec::with_capacity(message.encode_utf16().count() + 1); + wide_message.extend(message.encode_utf16()); + let wide_message_len = wide_message.len(); // does not count NUL + wide_message.push(0); + + unsafe { + RoOriginateErrorW(code.0, wide_message_len as u32, wide_message.as_ptr()); + } + } + + fn message(&self) -> Option { + let info = self.ptr.as_ref()?; + + let mut message = BasicString::default(); + + // First attempt to retrieve the restricted error information. + if let Some(info) = info.cast(&IID_IRestrictedErrorInfo) { + let mut fallback = BasicString::default(); + let mut code = 0; + + unsafe { + com_call!( + IRestrictedErrorInfo_Vtbl, + info.GetErrorDetails( + &mut fallback as *mut _ as _, + &mut code, + &mut message as *mut _ as _, + &mut BasicString::default() as *mut _ as _ + ) + ); + } + + if message.is_empty() { + message = fallback + }; + } + + // Next attempt to retrieve the regular error information. if message.is_empty() { - Self::from_hresult(code) - } else { unsafe { - RoOriginateErrorW(code.0, message.len() as u32, message.as_ptr()); + com_call!( + IErrorInfo_Vtbl, + info.GetDescription(&mut message as *mut _ as _) + ); } - code.into() } + + if message.is_empty() { + return None; + } + + Some(String::from_utf16_lossy(crate::strings::wide_trim_end( + message.as_wide(), + ))) } - #[cfg(not(windows))] - { - let _ = message; - Self::from_hresult(code) + } +} + +impl ErrorT { + /// Creates an error object without any failure information. + pub fn empty() -> Self { + Self { + code: NonZeroHRESULT::E_FAIL, + detail: Detail::empty(), } } + /// Creates a new error object, capturing the stack and other information about the + /// point of failure. + pub fn new>(code: HRESULT, message: T) -> Self { + Detail::originate_error(code, message.as_ref()); + // This into() call will take the ambient thread-local error object, if any. + Self::from(code) + } + /// Creates a new error object with an error code, but without additional error information. pub fn from_hresult(code: HRESULT) -> Self { Self { - code, - #[cfg(windows)] - info: None, + code: NonZeroHRESULT::from_hresult_or_fail(code), + detail: Detail::empty(), } } @@ -54,10 +247,8 @@ impl Error { pub fn from_win32() -> Self { #[cfg(windows)] { - Self { - code: HRESULT::from_win32(unsafe { GetLastError() }), - info: None, - } + let hr = HRESULT::from_win32(unsafe { GetLastError() }); + Self::from_hresult(hr) } #[cfg(not(windows))] { @@ -67,107 +258,64 @@ impl Error { /// The error code describing the error. pub const fn code(&self) -> HRESULT { - self.code + self.code.to_hresult() } /// The error message describing the error. pub fn message(&self) -> String { - #[cfg(windows)] - { - if let Some(info) = &self.info { - let mut message = BasicString::default(); - - // First attempt to retrieve the restricted error information. - if let Some(info) = info.cast(&IID_IRestrictedErrorInfo) { - let mut fallback = BasicString::default(); - let mut code = 0; - - unsafe { - com_call!( - IRestrictedErrorInfo_Vtbl, - info.GetErrorDetails( - &mut fallback as *mut _ as _, - &mut code, - &mut message as *mut _ as _, - &mut BasicString::default() as *mut _ as _ - ) - ); - } - - if message.is_empty() { - message = fallback - }; - } - - // Next attempt to retrieve the regular error information. - if message.is_empty() { - unsafe { - com_call!( - IErrorInfo_Vtbl, - info.GetDescription(&mut message as *mut _ as _) - ); - } - } - - return String::from_utf16_lossy(wide_trim_end(message.as_wide())); - } + if let Some(message) = self.detail.message() { + return message; } // Otherwise fallback to a generic error code description. self.code.message() } + /// Gets access to the error detail stored in this `Error`. + pub fn detail(&self) -> &Detail { + &self.detail + } + + /* /// The error object describing the error. - #[cfg(windows)] - pub fn as_ptr(&self) -> *mut c_void { + #[cfg(all(windows, feature = "error-info"))] + pub fn as_ptr(&self) -> *mut core::ffi::c_void { self.info + .ptr .as_ref() .map_or(core::ptr::null_mut(), |info| info.as_raw()) } + */ } #[cfg(feature = "std")] -impl std::error::Error for Error {} -unsafe impl Send for Error {} -unsafe impl Sync for Error {} +impl std::error::Error for ErrorT {} -impl From for HRESULT { - fn from(error: Error) -> Self { - #[cfg(windows)] - { - if let Some(info) = error.info { - unsafe { - SetErrorInfo(0, info.as_raw()); - } - } - } - error.code +impl From> for HRESULT { + fn from(error: ErrorT) -> Self { + error.detail.into_ambient(); + error.code.into() } } -impl From for Error { +impl From for ErrorT { fn from(code: HRESULT) -> Self { Self { - code, - #[cfg(windows)] - info: { - let mut info = None; - unsafe { GetErrorInfo(0, &mut info as *mut _ as _) }; - info - }, + code: NonZeroHRESULT::from_hresult_or_fail(code), + detail: Detail::from_ambient(), } } } #[cfg(feature = "std")] -impl From for std::io::Error { - fn from(from: Error) -> Self { - Self::from_raw_os_error(from.code.0) +impl From> for std::io::Error { + fn from(from: ErrorT) -> Self { + Self::from_raw_os_error(from.code.0.get()) } } #[cfg(feature = "std")] -impl From for Error { +impl From for ErrorT { fn from(from: std::io::Error) -> Self { match from.raw_os_error() { Some(status) => HRESULT::from_win32(status as u32).into(), @@ -176,37 +324,25 @@ impl From for Error { } } -impl From for Error { +impl From for ErrorT { fn from(_: alloc::string::FromUtf16Error) -> Self { - Self { - code: HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION), - #[cfg(windows)] - info: None, - } + Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for Error { +impl From for ErrorT { fn from(_: alloc::string::FromUtf8Error) -> Self { - Self { - code: HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION), - #[cfg(windows)] - info: None, - } + Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for Error { +impl From for ErrorT { fn from(_: core::num::TryFromIntError) -> Self { - Self { - code: HRESULT::from_win32(ERROR_INVALID_DATA), - #[cfg(windows)] - info: None, - } + Self::from_hresult(HRESULT::from_win32(ERROR_INVALID_DATA)) } } -impl core::fmt::Debug for Error { +impl core::fmt::Debug for ErrorT { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let mut debug = fmt.debug_struct("Error"); debug @@ -216,7 +352,7 @@ impl core::fmt::Debug for Error { } } -impl core::fmt::Display for Error { +impl core::fmt::Display for ErrorT { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let message = self.message(); if message.is_empty() { @@ -226,3 +362,21 @@ impl core::fmt::Display for Error { } } } + +// Equality tests only the HRESULT, not the error info (if any). +impl PartialEq for ErrorT { + fn eq(&self, other: &Self) -> bool { + self.code == other.code + } +} + +impl Eq for ErrorT {} + +// If we are using "slim" Error objects, then we can rely on Result<()> +// having a representation that is equivalent to HRESULT. +static_assertions::const_assert_eq!( + size_of::>>(), + size_of::() +); + +static_assertions::assert_impl_all!(Error: Send, Sync); diff --git a/crates/libs/result/src/hresult.rs b/crates/libs/result/src/hresult.rs index 48304abc80..d1368f0963 100644 --- a/crates/libs/result/src/hresult.rs +++ b/crates/libs/result/src/hresult.rs @@ -1,8 +1,11 @@ use super::*; +use core::mem::size_of; +use core::num::NonZeroI32; +use static_assertions::const_assert_eq; /// An error code value returned by most COM functions. #[repr(transparent)] -#[derive(Copy, Clone, Default, Eq, PartialEq)] +#[derive(Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd, Hash)] #[must_use] #[allow(non_camel_case_types)] pub struct HRESULT(pub i32); @@ -63,10 +66,10 @@ impl HRESULT { } /// The error message describing the error. - pub fn message(&self) -> String { + pub fn message(self) -> String { #[cfg(windows)] { - let mut message = HeapString::default(); + let mut message = crate::strings::HeapString::default(); let mut code = self.0; let mut module = core::ptr::null_mut(); @@ -97,10 +100,8 @@ impl HRESULT { ); if !message.0.is_null() && size > 0 { - String::from_utf16_lossy(wide_trim_end(core::slice::from_raw_parts( - message.0, - size as usize, - ))) + let message_slice = core::slice::from_raw_parts(message.0, size as usize); + String::from_utf16_lossy(crate::strings::wide_trim_end(message_slice)) } else { String::default() } @@ -152,3 +153,161 @@ impl core::fmt::Debug for HRESULT { f.write_fmt(format_args!("HRESULT({})", self)) } } + +impl core::fmt::Display for NonZeroHRESULT { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + ::fmt(&HRESULT::from(*self), f) + } +} + +impl core::fmt::Debug for NonZeroHRESULT { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + ::fmt(&HRESULT::from(*self), f) + } +} + +/// An error code value returned by most COM functions. +/// +/// This type cannot represent `S_OK`. This type is intended for use with either +/// `Option` or `Result` so that the unused 0 bit pattern can be used as a "niche", +/// which allows `Option` and `Result` to use the bit pattern and conserve space. +#[repr(transparent)] +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +#[must_use] +#[allow(non_camel_case_types)] +pub struct NonZeroHRESULT(pub NonZeroI32); + +const_assert_eq!(size_of::(), size_of::()); + +// Check the Option niche optimization. +const_assert_eq!(size_of::>(), size_of::()); + +// Check the Result niche optimization. +const_assert_eq!( + size_of::>(), + size_of::() +); + +impl NonZeroHRESULT { + /// Returns [`true`] if `self` is a success code. + /// + /// Although `NonZeroHRESULT` cannot represent `S_OK`, it can represent other success values + /// such as `S_FALSE`. + #[inline] + pub const fn is_ok(self) -> bool { + self.0.get() >= 0 + } + + /// Returns [`true`] if `self` is a failure code. + #[inline] + pub const fn is_err(self) -> bool { + self.0.get() < 0 + } + + /// Asserts that `self` is a success code. + /// + /// This will invoke the [`panic!`] macro if `self` is a failure code and display + /// the [`HRESULT`] value for diagnostics. + #[inline] + #[track_caller] + pub fn unwrap(self) { + assert!(self.is_ok(), "HRESULT 0x{:X}", self.0.get()); + } + + /// Converts the [`HRESULT`] to [`Result<()>`][Result<_>]. + #[inline] + pub fn ok(self) -> Result<()> { + // SAFETY: We use a static assertion to check that the size of these two types are identical. + // Since there is only one possible niche, that niche must be used for the Ok(()) value. + #[cfg(not(all(windows, feature = "error-info")))] + unsafe { + core::mem::transmute(self) + } + + // If we are not using slim Error, then we take the slow path. + #[cfg(all(windows, feature = "error-info"))] + HRESULT::from(self).ok() + } + + /// Calls `op` if `self` is a success code, otherwise returns [`HRESULT`] + /// converted to [`Result`]. + #[inline] + pub fn map(self, op: F) -> Result + where + F: FnOnce() -> T, + { + self.ok()?; + Ok(op()) + } + + /// Calls `op` if `self` is a success code, otherwise returns [`HRESULT`] + /// converted to [`Result`]. + #[inline] + pub fn and_then(self, op: F) -> Result + where + F: FnOnce() -> Result, + { + self.ok()?; + op() + } + + /// The error message describing the error. + pub fn message(self) -> String { + HRESULT::from(self).message() + } + + /// Converts `hr` to `NonZeroHRESULT`. If `hr` is `S_OK` (zero), then this returns `None`. + pub const fn from_hresult_opt(hr: HRESULT) -> Option { + // SAFETY: The niche optimization is guaranteed for Option. This transmute + // exploits the same niche optimization, only for an HRESULT that wraps an i32. + unsafe { core::mem::transmute(hr) } + } + + /// Converts `HRESULT` to `NonZeroHRESULT`. + /// + /// This is intended only for use in constant declarations. This will panic if `error` is 0. + pub const fn from_hresult_unwrap(hr: HRESULT) -> Self { + if let Some(nz) = NonZeroI32::new(hr.0) { + Self(nz) + } else { + panic!("`hr` cannot be S_OK (zero)"); + } + } + + /// The well-known `E_FAIL` constant. + pub const E_FAIL: NonZeroHRESULT = Self::from_hresult_unwrap(HRESULT(0x80004005_u32 as i32)); + + /// Converts `hr` to `NonZeroHRESULT`. If `hr` is `S_OK`, then this returns `E_FAIL`. + pub const fn from_hresult_or_fail(hr: HRESULT) -> Self { + if let Some(nz) = NonZeroI32::new(hr.0) { + Self(nz) + } else { + Self::E_FAIL + } + } + + /// Maps a Win32 error code to a NonZeroHRESULT value. If `error` is 0, then this will + /// return `E_FAIL`. + pub const fn from_win32(error: u32) -> Self { + Self::from_hresult_or_fail(HRESULT::from_win32(error)) + } + + /// Maps an NT error code to an HRESULT value. + /// + /// This is intended only for use in constant declarations. If `error` is 0, then this will + /// return `E_FAIL`. + pub const fn from_nt(error: i32) -> Self { + Self::from_hresult_or_fail(HRESULT::from_nt(error)) + } + + /// Converts this `NonZeroHRESULT` to an `HRESULT`. This is usable in `const fn`. + pub const fn to_hresult(self) -> HRESULT { + HRESULT(self.0.get()) + } +} + +impl From for HRESULT { + fn from(hr: NonZeroHRESULT) -> Self { + Self(hr.0.get()) + } +} diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index 8eb6ddbeeb..a8ff718968 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -11,27 +11,29 @@ Learn more about Rust for Windows here: = core::result::Result; + +#[cfg(test)] +mod tests; diff --git a/crates/libs/result/src/strings.rs b/crates/libs/result/src/strings.rs index ee861dc35c..ce77f1f717 100644 --- a/crates/libs/result/src/strings.rs +++ b/crates/libs/result/src/strings.rs @@ -18,55 +18,6 @@ impl Drop for HeapString { } } -#[repr(transparent)] -pub struct BasicString(*const u16); - -impl BasicString { - pub fn is_empty(&self) -> bool { - self.len() == 0 - } - - pub fn len(&self) -> usize { - if self.0.is_null() { - 0 - } else { - unsafe { SysStringLen(self.0) as usize } - } - } - - pub fn as_wide(&self) -> &[u16] { - let len = self.len(); - if len != 0 { - unsafe { core::slice::from_raw_parts(self.as_ptr(), len) } - } else { - &[] - } - } - - pub fn as_ptr(&self) -> *const u16 { - if !self.is_empty() { - self.0 - } else { - const EMPTY: [u16; 1] = [0]; - EMPTY.as_ptr() - } - } -} - -impl Default for BasicString { - fn default() -> Self { - Self(core::ptr::null_mut()) - } -} - -impl Drop for BasicString { - fn drop(&mut self) { - if !self.0.is_null() { - unsafe { SysFreeString(self.0) } - } - } -} - pub fn wide_trim_end(mut wide: &[u16]) -> &[u16] { while let Some(last) = wide.last() { match last { diff --git a/crates/libs/result/src/tests.rs b/crates/libs/result/src/tests.rs new file mode 100644 index 0000000000..7e38f3b24b --- /dev/null +++ b/crates/libs/result/src/tests.rs @@ -0,0 +1,18 @@ +use crate::*; + +#[test] +fn show_sizes() { + use core::mem::size_of; + + macro_rules! show_size { + ($t:ty) => { + println!("size_of {} = {}", stringify!($t), size_of::<$t>()); + }; + } + + println!("sizes:"); + show_size!(Error); + show_size!(Result<()>); + show_size!(Result); + show_size!(Result); +} diff --git a/crates/tests/implement/tests/data_object.rs b/crates/tests/implement/tests/data_object.rs index a53787e290..a8bab9b37d 100644 --- a/crates/tests/implement/tests/data_object.rs +++ b/crates/tests/implement/tests/data_object.rs @@ -100,7 +100,7 @@ fn test() -> Result<()> { assert!(r.is_err()); let e = r.unwrap_err(); assert!(e.code() == S_OK); - assert!(e.as_ptr().is_null()); + assert!(e.detail().as_ptr().is_null()); d.DAdvise(&Default::default(), 0, None)?; diff --git a/crates/tests/result/tests/error.rs b/crates/tests/result/tests/error.rs index 43b5b20af5..75a742a999 100644 --- a/crates/tests/result/tests/error.rs +++ b/crates/tests/result/tests/error.rs @@ -1,7 +1,7 @@ use windows_result::*; -const S_OK: HRESULT = HRESULT(0); const E_INVALIDARG: HRESULT = HRESULT(-2147024809i32); +const E_FAIL: HRESULT = HRESULT(0x80004005_u32 as i32); const ERROR_CANCELLED: u32 = 1223; const ERROR_INVALID_DATA: u32 = 13; const E_CANCELLED: HRESULT = HRESULT::from_win32(ERROR_CANCELLED); @@ -10,21 +10,22 @@ windows_targets::link!("kernel32.dll" "system" fn SetLastError(code: u32)); #[test] fn empty() { - const EMPTY: Error = Error::empty(); - assert_eq!(EMPTY.code(), S_OK); - assert!(EMPTY.as_ptr().is_null()); - assert_eq!(EMPTY.message(), "The operation completed successfully."); + let e: Error = Error::empty(); + assert_eq!(e.code(), E_FAIL); + assert!(e.detail().as_ptr().is_null()); + assert_eq!(e.message(), "Unspecified error"); } #[test] fn new() { let e = Error::new(E_INVALIDARG, ""); assert_eq!(e.code(), E_INVALIDARG); - assert!(e.as_ptr().is_null()); + assert!(e.detail().as_ptr().is_null()); + assert_eq!(e.message(), "The parameter is incorrect."); let e = Error::new(E_INVALIDARG, "test message"); assert_eq!(e.code(), E_INVALIDARG); - assert!(!e.as_ptr().is_null()); + assert!(!e.detail().as_ptr().is_null()); assert_eq!(e.message(), "test message"); } @@ -32,7 +33,7 @@ fn new() { fn from_hresult() { let e = Error::from_hresult(E_INVALIDARG); assert_eq!(e.code(), E_INVALIDARG); - assert!(e.as_ptr().is_null()); + assert!(e.detail().as_ptr().is_null()); assert_eq!(e.message(), "The parameter is incorrect."); } @@ -41,13 +42,13 @@ fn from_win32() { unsafe { SetLastError(0) }; let e = Error::from_win32(); - assert_eq!(e.code(), S_OK); - assert!(e.as_ptr().is_null()); + assert_eq!(e.code(), E_FAIL); + assert!(e.detail().as_ptr().is_null()); unsafe { SetLastError(ERROR_CANCELLED) }; let e = Error::from_win32(); - assert!(e.as_ptr().is_null()); + assert!(e.detail().as_ptr().is_null()); assert_eq!(e.code(), E_CANCELLED); } From b6c3d77760bf0789bf777282006a4d986a0af4b5 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 14:43:10 -0700 Subject: [PATCH 02/15] switch to slim-errors --- crates/libs/core/Cargo.toml | 1 + crates/libs/result/Cargo.toml | 5 +- crates/libs/result/src/error.rs | 84 ++++++++++++--------- crates/libs/result/src/hresult.rs | 4 +- crates/libs/result/src/lib.rs | 2 +- crates/libs/windows/Cargo.toml | 1 + crates/tests/core/tests/error.rs | 2 +- crates/tests/implement/tests/data_object.rs | 2 +- crates/tests/implement/tests/vector.rs | 2 +- 9 files changed, 58 insertions(+), 45 deletions(-) diff --git a/crates/libs/core/Cargo.toml b/crates/libs/core/Cargo.toml index f250d93a10..b9a36eab6e 100644 --- a/crates/libs/core/Cargo.toml +++ b/crates/libs/core/Cargo.toml @@ -32,3 +32,4 @@ windows-interface = { path = "../interface", version = "0.57.0" } [features] default = ["std"] std = [] +slim-errors = ["windows-result/slim-errors"] diff --git a/crates/libs/result/Cargo.toml b/crates/libs/result/Cargo.toml index 971d7d89b1..8ac2113f59 100644 --- a/crates/libs/result/Cargo.toml +++ b/crates/libs/result/Cargo.toml @@ -11,9 +11,10 @@ readme = "readme.md" categories = ["os::windows-apis"] [features] -default = ["std", "error-info"] +default = ["std"] std = [] -error-info = [] +# slim-errors reduces the size of Error (and Result<()>) to that of a single HRESULT +slim-errors = [] [lints] workspace = true diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index e2d76bcbda..cefbda16e8 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -2,9 +2,17 @@ use super::*; #[allow(unused_imports)] use core::mem::size_of; -/// An error object consists of both an error code as well as detailed error information for debugging. +// We define ErrorT and Error separately because it allows type inference to work correctly +// in some common situations. If there were no separate type alias, then expressions like +// `let e = Error::from(...)` and `let e = Error::new(...)` would have a free (unspecified) type +// parameter, and so are ambiguous and fail to compile. +// +// Most code wants to use the default type parameter, so we give the type alias the `Error` name. +// For the code that wants to deal directly non-default error details, it can use `ErrorT<...>`. + +/// An error object consists of both an error code and optional detailed error information for debugging. /// -/// # Extended error info and the `error-info` feature +/// # Extended error info and the `slim-errors` feature /// /// The `Error` contains an `HRESULT` value that describes the error, as well as an optional /// `IErrorInfo` object. The `IErrorInfo` object is a COM object that can provide detailed information @@ -16,13 +24,15 @@ use core::mem::size_of; /// info within `Error` has no benefits, but has substantial costs because it increases the size of /// the `Error` object, which also increases the size of `Result`. /// -/// The `windows-result` crate provides the `error-info` feature, which gives developers control over -/// the costs of using `Error`. +/// The `slim-errors` Cargo feature allows you to _statically_ disable error detail in `Error`. +/// This allows the `Error` and even the `Result<(), Error>` types to have a very small size; they +/// have the same size and alignment as `HRESULT` itself. Because of this, they can be returned +/// directly in a machine register, which gives better performance than writing the (relatively +/// large) `Error` object (with full error detail) to the stack on every function return. #[derive(Clone)] -pub struct ErrorT -where - Detail: ErrorDetail, -{ +pub struct ErrorT { + /// The error code. Because this uses `NonZeroHRESULT`, this provides a "niche" to the Rust + /// compiler. This allows the compiler to use more compact representations in some sitatutions. code: NonZeroHRESULT, /// Contains details about the error, such as error text. @@ -35,30 +45,22 @@ where /// An error object consists of both an error code as well as detailed error information for debugging. /// -/// # Extended error info and the `error-info` feature -/// -/// The `Error` contains an `HRESULT` value that describes the error, as well as an optional -/// `IErrorInfo` object. The `IErrorInfo` object is a COM object that can provide detailed information -/// about an error, such as a text string, a `ProgID` of the originator, etc. If the error object -/// was originated in an WinRT component, then additional information such as a stack track may be -/// captured. -/// -/// However, many systems based on COM do not use `IErrorInfo`. For these systems, the optional error -/// info within `Error` has no benefits, but has substantial costs because it increases the size of -/// the `Error` object, which also increases the size of `Result`. -/// -/// The `windows-result` crate provides the `error-info` feature, which gives developers control over -/// the costs of using `Error`. +/// This is a type alias for the [`ErrorT`] type. pub type Error = ErrorT; /// Specifies the default error detail for the `Error` type. -#[cfg(all(windows, feature = "error-info"))] +#[cfg(not(feature = "slim-errors"))] pub type DefaultDetail = ErrorInfo; /// Specifies the default error detail for the `Error` type. -#[cfg(not(all(windows, feature = "error-info")))] +#[cfg(feature = "slim-errors")] pub type DefaultDetail = NoDetail; +/// On non-Windows platforms, `ErrorInfo` is mapped to `NoDetail` because non-Windows platforms +/// do not support `IErrorInfo`. +#[cfg(not(windows))] +pub type ErrorInfo = NoDetail; + /// This type implements `ErrorDetail`, by containing no error information at all. This type allows /// for "slim" `Error` objects, which have the same size as `HRESULT`. #[derive(Default, Clone)] @@ -275,17 +277,6 @@ impl ErrorT { pub fn detail(&self) -> &Detail { &self.detail } - - /* - /// The error object describing the error. - #[cfg(all(windows, feature = "error-info"))] - pub fn as_ptr(&self) -> *mut core::ffi::c_void { - self.info - .ptr - .as_ref() - .map_or(core::ptr::null_mut(), |info| info.as_raw()) - } - */ } #[cfg(feature = "std")] @@ -363,14 +354,33 @@ impl core::fmt::Display for ErrorT { } } +impl core::hash::Hash for ErrorT { + fn hash(&self, state: &mut H) { + self.code.hash(state); + // We do not hash the detail. + } +} + // Equality tests only the HRESULT, not the error info (if any). -impl PartialEq for ErrorT { +impl PartialEq for ErrorT { fn eq(&self, other: &Self) -> bool { self.code == other.code } } -impl Eq for ErrorT {} +impl Eq for ErrorT {} + +impl PartialOrd for ErrorT { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for ErrorT { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + self.code.cmp(&other.code) + } +} // If we are using "slim" Error objects, then we can rely on Result<()> // having a representation that is equivalent to HRESULT. diff --git a/crates/libs/result/src/hresult.rs b/crates/libs/result/src/hresult.rs index d1368f0963..39f0e841fd 100644 --- a/crates/libs/result/src/hresult.rs +++ b/crates/libs/result/src/hresult.rs @@ -219,13 +219,13 @@ impl NonZeroHRESULT { pub fn ok(self) -> Result<()> { // SAFETY: We use a static assertion to check that the size of these two types are identical. // Since there is only one possible niche, that niche must be used for the Ok(()) value. - #[cfg(not(all(windows, feature = "error-info")))] + #[cfg(any(feature = "slim-error", not(windows)))] unsafe { core::mem::transmute(self) } // If we are not using slim Error, then we take the slow path. - #[cfg(all(windows, feature = "error-info"))] + #[cfg(not(any(feature = "slim-error", not(windows))))] HRESULT::from(self).ok() } diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index a8ff718968..e18fdcff73 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -23,7 +23,7 @@ mod com; #[cfg(windows)] mod strings; -#[cfg(all(windows, feature = "error-info"))] +#[cfg(windows)] mod bstr; mod error; diff --git a/crates/libs/windows/Cargo.toml b/crates/libs/windows/Cargo.toml index cec03168ca..102324e8bf 100644 --- a/crates/libs/windows/Cargo.toml +++ b/crates/libs/windows/Cargo.toml @@ -36,6 +36,7 @@ docs = [] deprecated = [] implement = [] std = ["windows-core/std"] +slim-errors = ["windows-core/slim-errors"] # generated features AI = ["Foundation"] AI_MachineLearning = ["AI"] diff --git a/crates/tests/core/tests/error.rs b/crates/tests/core/tests/error.rs index f1cb505b17..82b162bbc5 100644 --- a/crates/tests/core/tests/error.rs +++ b/crates/tests/core/tests/error.rs @@ -67,5 +67,5 @@ fn suppressed_error_info() -> Result<()> { fn just_hresult() { let e: Error = E_NOTIMPL.into(); assert!(e.code() == E_NOTIMPL); - assert!(e.as_ptr().is_null()); + assert!(e.detail().as_ptr().is_null()); } diff --git a/crates/tests/implement/tests/data_object.rs b/crates/tests/implement/tests/data_object.rs index a8bab9b37d..474f8c2da2 100644 --- a/crates/tests/implement/tests/data_object.rs +++ b/crates/tests/implement/tests/data_object.rs @@ -99,7 +99,7 @@ fn test() -> Result<()> { let r = d.EnumFormatEtc(0); assert!(r.is_err()); let e = r.unwrap_err(); - assert!(e.code() == S_OK); + assert!(e.code() == E_FAIL); assert!(e.detail().as_ptr().is_null()); d.DAdvise(&Default::default(), 0, None)?; diff --git a/crates/tests/implement/tests/vector.rs b/crates/tests/implement/tests/vector.rs index 9c3c2e03a5..de2cd63a00 100644 --- a/crates/tests/implement/tests/vector.rs +++ b/crates/tests/implement/tests/vector.rs @@ -184,7 +184,7 @@ fn GetAt() -> Result<()> { ]) .into(); assert_eq!(v.GetAt(0)?.ToString()?, "http://test/"); - assert_eq!(v.GetAt(1).unwrap_err().code(), S_OK); + assert_eq!(v.GetAt(1).unwrap_err().code(), E_FAIL); Ok(()) } From 60e243cbe07176e365fdefcf2482a87e83623c5d Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 14:46:07 -0700 Subject: [PATCH 03/15] update workflow --- .github/workflows/msrv-windows-result.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/msrv-windows-result.yml b/.github/workflows/msrv-windows-result.yml index 9e1067cb18..baef56a15a 100644 --- a/.github/workflows/msrv-windows-result.yml +++ b/.github/workflows/msrv-windows-result.yml @@ -27,3 +27,5 @@ jobs: run: rustup update --no-self-update ${{ matrix.rust }} && rustup default ${{ matrix.rust }} - name: Check run: cargo check -p windows-result --all-features + - name: Check Default Features + run: cargo check -p windows-result From d30af76b37f2c2ef7dd4d308d468fcec2af3235f Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 15:13:07 -0700 Subject: [PATCH 04/15] fix linux tests --- crates/libs/result/src/error.rs | 9 +++++++++ crates/tests/linux/tests/hresult.rs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index cefbda16e8..ecc6373118 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -231,6 +231,9 @@ impl ErrorT { /// Creates a new error object, capturing the stack and other information about the /// point of failure. + /// + /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot + /// store an `S_OK` value. pub fn new>(code: HRESULT, message: T) -> Self { Detail::originate_error(code, message.as_ref()); // This into() call will take the ambient thread-local error object, if any. @@ -238,6 +241,9 @@ impl ErrorT { } /// Creates a new error object with an error code, but without additional error information. + /// + /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot + /// store an `S_OK` value. pub fn from_hresult(code: HRESULT) -> Self { Self { code: NonZeroHRESULT::from_hresult_or_fail(code), @@ -246,6 +252,9 @@ impl ErrorT { } /// Creates a new `Error` from the Win32 error code returned by `GetLastError()`. + /// + /// If `GetLastError()` returns `ERROR_SUCCESS` then this is remapped to `E_FAIL`. The `Error` + /// object cannot store an `S_OK` value. pub fn from_win32() -> Self { #[cfg(windows)] { diff --git a/crates/tests/linux/tests/hresult.rs b/crates/tests/linux/tests/hresult.rs index 9eef4b7f32..de64280ab1 100644 --- a/crates/tests/linux/tests/hresult.rs +++ b/crates/tests/linux/tests/hresult.rs @@ -17,7 +17,7 @@ fn basic_hresult() { fn error_message_is_not_supported() { let e = Error::new(S_OK, "this gets ignored"); let message = e.message(); - assert_eq!(message, "0x00000000"); + assert_eq!(message, "0x80004005"); } #[test] From b5c861aca80cee1581d47b226b2afea25f717e02 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 15:34:26 -0700 Subject: [PATCH 05/15] fix cfg --- crates/libs/result/src/hresult.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/libs/result/src/hresult.rs b/crates/libs/result/src/hresult.rs index 39f0e841fd..0d5f0aa367 100644 --- a/crates/libs/result/src/hresult.rs +++ b/crates/libs/result/src/hresult.rs @@ -219,13 +219,13 @@ impl NonZeroHRESULT { pub fn ok(self) -> Result<()> { // SAFETY: We use a static assertion to check that the size of these two types are identical. // Since there is only one possible niche, that niche must be used for the Ok(()) value. - #[cfg(any(feature = "slim-error", not(windows)))] + #[cfg(any(feature = "slim-errors", not(windows)))] unsafe { core::mem::transmute(self) } // If we are not using slim Error, then we take the slow path. - #[cfg(not(any(feature = "slim-error", not(windows))))] + #[cfg(not(any(feature = "slim-errors", not(windows))))] HRESULT::from(self).ok() } From db9c87d61546d997794939c31d63a206b69ecb63 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 16:12:06 -0700 Subject: [PATCH 06/15] fix natvis --- crates/libs/result/.natvis | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/libs/result/.natvis b/crates/libs/result/.natvis index 0d373c2529..cc5da51de3 100644 --- a/crates/libs/result/.natvis +++ b/crates/libs/result/.natvis @@ -1,13 +1,17 @@ - + code - info + detail - {(HRESULT)__0} + {(::HRESULT)__0} + + + + {(::HRESULT)__0.__0.__0} From 4f3196addcc9702e774e3e7455aad08ac3b039b8 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 16:15:32 -0700 Subject: [PATCH 07/15] fix msrv-related issue --- crates/libs/result/src/error.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index ecc6373118..5560b927c9 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -267,11 +267,6 @@ impl ErrorT { } } - /// The error code describing the error. - pub const fn code(&self) -> HRESULT { - self.code.to_hresult() - } - /// The error message describing the error. pub fn message(&self) -> String { if let Some(message) = self.detail.message() { @@ -288,6 +283,15 @@ impl ErrorT { } } +// This is a separate impl block because Rust 1.60.0 (our MSRV) rejects const fns that have +// trait bounds on them, so we place it in a separate impl without any bounds. +impl ErrorT { + /// The error code describing the error. + pub const fn code(&self) -> HRESULT { + self.code.to_hresult() + } +} + #[cfg(feature = "std")] impl std::error::Error for ErrorT {} From 88d911cf608d57f752e8eaad9d42d433e595c207 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 17:12:06 -0700 Subject: [PATCH 08/15] PR feedback; default; Error::empty --- crates/libs/core/src/imp/factory_cache.rs | 4 +- crates/libs/core/src/type.rs | 8 ++- crates/libs/result/src/error.rs | 60 +++++++++------------ crates/tests/implement/tests/data_object.rs | 4 +- crates/tests/implement/tests/vector.rs | 2 +- crates/tests/noexcept/tests/test.rs | 2 +- crates/tests/result/tests/error.rs | 8 --- crates/tests/winrt/tests/collisions.rs | 2 +- 8 files changed, 39 insertions(+), 51 deletions(-) diff --git a/crates/libs/core/src/imp/factory_cache.rs b/crates/libs/core/src/imp/factory_cache.rs index e2b03d0b79..f7aa3f786f 100644 --- a/crates/libs/core/src/imp/factory_cache.rs +++ b/crates/libs/core/src/imp/factory_cache.rs @@ -172,7 +172,7 @@ mod tests { if unsafe { library.as_bytes() } == &b"A.dll"[..] { Ok(42) } else { - Err(crate::Error::empty()) + Err(crate::Error::fail()) } }); assert!(matches!(end_result, Some(42))); @@ -182,7 +182,7 @@ mod tests { let mut results = Vec::new(); let end_result = search_path(path, |library| { results.push(unsafe { library.to_string().unwrap() }); - crate::Result::<()>::Err(crate::Error::empty()) + crate::Result::<()>::Err(crate::Error::fail()) }); assert!(end_result.is_none()); assert_eq!(results, vec!["A.B.dll", "A.dll"]); diff --git a/crates/libs/core/src/type.rs b/crates/libs/core/src/type.rs index 215c20135d..58cc458a4b 100644 --- a/crates/libs/core/src/type.rs +++ b/crates/libs/core/src/type.rs @@ -1,4 +1,5 @@ use super::*; +use crate::imp::E_POINTER; #[doc(hidden)] pub trait TypeKind { @@ -35,12 +36,15 @@ where if !abi.is_null() { Ok(core::mem::transmute_copy(&abi)) } else { - Err(Error::empty()) + Err(Error::from_hresult(E_POINTER)) } } fn from_default(default: &Self::Default) -> Result { - default.as_ref().cloned().ok_or(Error::empty()) + default + .as_ref() + .cloned() + .ok_or(Error::from_hresult(E_POINTER)) } } diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index 5560b927c9..a80aa04ae3 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -67,10 +67,7 @@ pub type ErrorInfo = NoDetail; pub struct NoDetail; /// Defines the behavior of error detail objects, which are stored within `Error`. -pub trait ErrorDetail: Sized { - /// Returns an empty error detail. - fn empty() -> Self; - +pub trait ErrorDetail: Sized + Default { /// If this implementation stores error detail in ambient (thread-local) storage, then this /// function transfers the error detail from ambient storage and returns it. fn from_ambient() -> Self; @@ -92,9 +89,6 @@ pub trait ErrorDetail: Sized { } impl ErrorDetail for NoDetail { - fn empty() -> Self { - Self - } fn from_ambient() -> Self { Self } @@ -137,10 +131,6 @@ mod win_impl { } impl ErrorDetail for ErrorInfo { - fn empty() -> Self { - Self { ptr: None } - } - fn from_ambient() -> Self { unsafe { let mut ptr = None; @@ -220,26 +210,17 @@ mod win_impl { } } -impl ErrorT { - /// Creates an error object without any failure information. - pub fn empty() -> Self { +impl ErrorT { + /// Creates an error object without any specific failure information. + /// + /// The `code()` for this error is `E_FAIL`. + pub fn fail() -> Self { Self { code: NonZeroHRESULT::E_FAIL, - detail: Detail::empty(), + detail: Detail::default(), } } - /// Creates a new error object, capturing the stack and other information about the - /// point of failure. - /// - /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot - /// store an `S_OK` value. - pub fn new>(code: HRESULT, message: T) -> Self { - Detail::originate_error(code, message.as_ref()); - // This into() call will take the ambient thread-local error object, if any. - Self::from(code) - } - /// Creates a new error object with an error code, but without additional error information. /// /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot @@ -247,7 +228,7 @@ impl ErrorT { pub fn from_hresult(code: HRESULT) -> Self { Self { code: NonZeroHRESULT::from_hresult_or_fail(code), - detail: Detail::empty(), + detail: Detail::default(), } } @@ -266,6 +247,19 @@ impl ErrorT { unimplemented!() } } +} + +impl ErrorT { + /// Creates a new error object, capturing the stack and other information about the + /// point of failure. + /// + /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot + /// store an `S_OK` value. + pub fn new>(code: HRESULT, message: T) -> Self { + Detail::originate_error(code, message.as_ref()); + // This into() call will take the ambient thread-local error object, if any. + Self::from(code) + } /// The error message describing the error. pub fn message(&self) -> String { @@ -276,16 +270,14 @@ impl ErrorT { // Otherwise fallback to a generic error code description. self.code.message() } +} +impl ErrorT { /// Gets access to the error detail stored in this `Error`. pub fn detail(&self) -> &Detail { &self.detail } -} -// This is a separate impl block because Rust 1.60.0 (our MSRV) rejects const fns that have -// trait bounds on them, so we place it in a separate impl without any bounds. -impl ErrorT { /// The error code describing the error. pub const fn code(&self) -> HRESULT { self.code.to_hresult() @@ -328,19 +320,19 @@ impl From for ErrorT { } } -impl From for ErrorT { +impl From for ErrorT { fn from(_: alloc::string::FromUtf16Error) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for ErrorT { +impl From for ErrorT { fn from(_: alloc::string::FromUtf8Error) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for ErrorT { +impl From for ErrorT { fn from(_: core::num::TryFromIntError) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_INVALID_DATA)) } diff --git a/crates/tests/implement/tests/data_object.rs b/crates/tests/implement/tests/data_object.rs index 474f8c2da2..361ad3741a 100644 --- a/crates/tests/implement/tests/data_object.rs +++ b/crates/tests/implement/tests/data_object.rs @@ -58,7 +58,7 @@ impl IDataObject_Impl for Test_Impl { fn EnumFormatEtc(&self, _: u32) -> Result { unsafe { (*self.0.get()).EnumFormatEtc = true; - Err(Error::empty()) + Err(Error::fail()) } } @@ -79,7 +79,7 @@ impl IDataObject_Impl for Test_Impl { fn EnumDAdvise(&self) -> Result { unsafe { (*self.0.get()).EnumDAdvise = true; - Err(Error::empty()) + Err(Error::fail()) } } } diff --git a/crates/tests/implement/tests/vector.rs b/crates/tests/implement/tests/vector.rs index de2cd63a00..da00164180 100644 --- a/crates/tests/implement/tests/vector.rs +++ b/crates/tests/implement/tests/vector.rs @@ -184,7 +184,7 @@ fn GetAt() -> Result<()> { ]) .into(); assert_eq!(v.GetAt(0)?.ToString()?, "http://test/"); - assert_eq!(v.GetAt(1).unwrap_err().code(), E_FAIL); + assert_eq!(v.GetAt(1).unwrap_err().code(), E_POINTER); Ok(()) } diff --git a/crates/tests/noexcept/tests/test.rs b/crates/tests/noexcept/tests/test.rs index 5e6e6f54d5..5ee47476f3 100644 --- a/crates/tests/noexcept/tests/test.rs +++ b/crates/tests/noexcept/tests/test.rs @@ -42,7 +42,7 @@ impl ITest_Impl for Test_Impl { } fn Test(&self) -> Result { let this = self.0.read().unwrap(); - this.2.clone().ok_or_else(Error::empty) + this.2.clone().ok_or_else(Error::fail) } fn SetTest(&self, value: Option<&ITest>) -> Result<()> { let mut this = self.0.write().unwrap(); diff --git a/crates/tests/result/tests/error.rs b/crates/tests/result/tests/error.rs index 75a742a999..e5dac491c5 100644 --- a/crates/tests/result/tests/error.rs +++ b/crates/tests/result/tests/error.rs @@ -8,14 +8,6 @@ const E_CANCELLED: HRESULT = HRESULT::from_win32(ERROR_CANCELLED); windows_targets::link!("kernel32.dll" "system" fn SetLastError(code: u32)); -#[test] -fn empty() { - let e: Error = Error::empty(); - assert_eq!(e.code(), E_FAIL); - assert!(e.detail().as_ptr().is_null()); - assert_eq!(e.message(), "Unspecified error"); -} - #[test] fn new() { let e = Error::new(E_INVALIDARG, ""); diff --git a/crates/tests/winrt/tests/collisions.rs b/crates/tests/winrt/tests/collisions.rs index 9c7c64e5d2..23b27794a0 100644 --- a/crates/tests/winrt/tests/collisions.rs +++ b/crates/tests/winrt/tests/collisions.rs @@ -16,7 +16,7 @@ fn wifi() -> windows::core::Result<()> { assert!(!a.is_empty()); // from_id_async from IWiFiDirectDeviceStatics - assert!(WiFiDirectDevice::FromIdAsync(&a)?.get() == Err(windows::core::Error::empty())); + assert!(WiFiDirectDevice::FromIdAsync(&a)?.get() == Err(windows::core::Error::fail())); // get_device_selector overload from IWiFiDirectDeviceStatics2 is renamed to get_device_selector2 let c = WiFiDirectDevice::GetDeviceSelector2(WiFiDirectDeviceSelectorType::DeviceInterface)?; From de1c33a788d26abfe61eec61851bcae522185f69 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 25 Jun 2024 17:32:40 -0700 Subject: [PATCH 09/15] fix error in test case --- crates/tests/winrt/tests/collisions.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/tests/winrt/tests/collisions.rs b/crates/tests/winrt/tests/collisions.rs index 23b27794a0..482fadabf9 100644 --- a/crates/tests/winrt/tests/collisions.rs +++ b/crates/tests/winrt/tests/collisions.rs @@ -1,5 +1,5 @@ use windows::{ - core::HSTRING, + core::{imp::E_POINTER, HSTRING}, ApplicationModel::Email::EmailAttachment, Devices::WiFiDirect::{ WiFiDirectConnectionParameters, WiFiDirectDevice, WiFiDirectDeviceSelectorType, @@ -16,7 +16,10 @@ fn wifi() -> windows::core::Result<()> { assert!(!a.is_empty()); // from_id_async from IWiFiDirectDeviceStatics - assert!(WiFiDirectDevice::FromIdAsync(&a)?.get() == Err(windows::core::Error::fail())); + assert!( + WiFiDirectDevice::FromIdAsync(&a)?.get() + == Err(windows::core::Error::from_hresult(E_POINTER)) + ); // get_device_selector overload from IWiFiDirectDeviceStatics2 is renamed to get_device_selector2 let c = WiFiDirectDevice::GetDeviceSelector2(WiFiDirectDeviceSelectorType::DeviceInterface)?; From 95bb3732f8b5d197e5603de600ea6449eab67e20 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 09:05:54 -0700 Subject: [PATCH 10/15] move back to features, PR feedback --- crates/libs/core/Cargo.toml | 5 +- crates/libs/core/src/imp/factory_cache.rs | 4 +- crates/libs/core/src/type.rs | 8 +- crates/libs/result/Cargo.toml | 11 +- crates/libs/result/src/error.rs | 491 ++++++++++---------- crates/libs/result/src/hresult.rs | 171 +------ crates/libs/result/src/lib.rs | 8 +- crates/libs/strings/Cargo.toml | 1 + crates/libs/windows/Cargo.toml | 2 +- crates/tests/core/tests/error.rs | 2 +- crates/tests/implement/tests/data_object.rs | 8 +- crates/tests/implement/tests/vector.rs | 2 +- crates/tests/linux/tests/result.rs | 2 +- crates/tests/noexcept/tests/test.rs | 2 +- crates/tests/result/tests/error.rs | 23 +- crates/tests/winrt/tests/collisions.rs | 7 +- 16 files changed, 295 insertions(+), 452 deletions(-) diff --git a/crates/libs/core/Cargo.toml b/crates/libs/core/Cargo.toml index 9cd6c8b55d..4113fe1320 100644 --- a/crates/libs/core/Cargo.toml +++ b/crates/libs/core/Cargo.toml @@ -24,6 +24,7 @@ path = "../targets" [dependencies.windows-result] version = "0.1.1" path = "../result" +default-features = false [dependencies.windows-strings] version = "0.1.0" @@ -35,5 +36,5 @@ windows-interface = { path = "../interface", version = "0.57.0" } [features] default = ["std"] -std = [] -slim-errors = ["windows-result/slim-errors"] +std = ["windows-result/std"] +error-info = ["windows-result/error-info"] diff --git a/crates/libs/core/src/imp/factory_cache.rs b/crates/libs/core/src/imp/factory_cache.rs index f7aa3f786f..e2b03d0b79 100644 --- a/crates/libs/core/src/imp/factory_cache.rs +++ b/crates/libs/core/src/imp/factory_cache.rs @@ -172,7 +172,7 @@ mod tests { if unsafe { library.as_bytes() } == &b"A.dll"[..] { Ok(42) } else { - Err(crate::Error::fail()) + Err(crate::Error::empty()) } }); assert!(matches!(end_result, Some(42))); @@ -182,7 +182,7 @@ mod tests { let mut results = Vec::new(); let end_result = search_path(path, |library| { results.push(unsafe { library.to_string().unwrap() }); - crate::Result::<()>::Err(crate::Error::fail()) + crate::Result::<()>::Err(crate::Error::empty()) }); assert!(end_result.is_none()); assert_eq!(results, vec!["A.B.dll", "A.dll"]); diff --git a/crates/libs/core/src/type.rs b/crates/libs/core/src/type.rs index 58cc458a4b..215c20135d 100644 --- a/crates/libs/core/src/type.rs +++ b/crates/libs/core/src/type.rs @@ -1,5 +1,4 @@ use super::*; -use crate::imp::E_POINTER; #[doc(hidden)] pub trait TypeKind { @@ -36,15 +35,12 @@ where if !abi.is_null() { Ok(core::mem::transmute_copy(&abi)) } else { - Err(Error::from_hresult(E_POINTER)) + Err(Error::empty()) } } fn from_default(default: &Self::Default) -> Result { - default - .as_ref() - .cloned() - .ok_or(Error::from_hresult(E_POINTER)) + default.as_ref().cloned().ok_or(Error::empty()) } } diff --git a/crates/libs/result/Cargo.toml b/crates/libs/result/Cargo.toml index 8ac2113f59..bbb7bd33e5 100644 --- a/crates/libs/result/Cargo.toml +++ b/crates/libs/result/Cargo.toml @@ -11,10 +11,15 @@ readme = "readme.md" categories = ["os::windows-apis"] [features] -default = ["std"] +default = ["std", "error-info"] std = [] -# slim-errors reduces the size of Error (and Result<()>) to that of a single HRESULT -slim-errors = [] +error-info = [] + +# The disable-error-info feature forcibly disables error-info (and overrides it). +# This should _not_ be the preferred way to disable error info, but is necessary +# in case a third-party crate has a dependency on Windows crates but does not +# provide any way to control its own feature selection. +disable-error-info = [] [lints] workspace = true diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index a80aa04ae3..04a9f9b3ad 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -1,21 +1,15 @@ use super::*; +use core::num::NonZeroI32; + #[allow(unused_imports)] use core::mem::size_of; -// We define ErrorT and Error separately because it allows type inference to work correctly -// in some common situations. If there were no separate type alias, then expressions like -// `let e = Error::from(...)` and `let e = Error::new(...)` would have a free (unspecified) type -// parameter, and so are ambiguous and fail to compile. -// -// Most code wants to use the default type parameter, so we give the type alias the `Error` name. -// For the code that wants to deal directly non-default error details, it can use `ErrorT<...>`. - /// An error object consists of both an error code and optional detailed error information for debugging. /// -/// # Extended error info and the `slim-errors` feature +/// # Extended error info and the `error-info` feature /// -/// The `Error` contains an `HRESULT` value that describes the error, as well as an optional -/// `IErrorInfo` object. The `IErrorInfo` object is a COM object that can provide detailed information +/// `Error` contains an [`HRESULT`] value that describes the error, as well as an optional +/// `IErrorInfo` COM object. The `IErrorInfo` object is a COM object that can provide detailed information /// about an error, such as a text string, a `ProgID` of the originator, etc. If the error object /// was originated in an WinRT component, then additional information such as a stack track may be /// captured. @@ -24,294 +18,158 @@ use core::mem::size_of; /// info within `Error` has no benefits, but has substantial costs because it increases the size of /// the `Error` object, which also increases the size of `Result`. /// -/// The `slim-errors` Cargo feature allows you to _statically_ disable error detail in `Error`. +/// The `error-info` Cargo feature allows you to control whether `IErrorInfo` support is enabled +/// at compile time. The `error-info` feature is _enabled_ by default, so if you want to disable it +/// you will need to disable default features. +/// +/// When the `error-info` feature is disabled, /// This allows the `Error` and even the `Result<(), Error>` types to have a very small size; they /// have the same size and alignment as `HRESULT` itself. Because of this, they can be returned /// directly in a machine register, which gives better performance than writing the (relatively /// large) `Error` object (with full error detail) to the stack on every function return. +/// +/// # References +/// +/// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) #[derive(Clone)] -pub struct ErrorT { - /// The error code. Because this uses `NonZeroHRESULT`, this provides a "niche" to the Rust - /// compiler. This allows the compiler to use more compact representations in some sitatutions. - code: NonZeroHRESULT, +pub struct Error { + /// The `HRESULT` error code, but represented using `NonZeroI32`. The `NonZeroHRESULT` provides + /// a "niche" to the Rust compiler, which is a space-saving optimization. This allows the + /// compiler to use more compact representations in some sitatutions. + code: NonZeroI32, /// Contains details about the error, such as error text. - /// - /// Note; We cannot use `Option` here, because that would increase the size of `Error` - /// even when `Detail` contains no information at all (a ZST). The `Detail` type itself must - /// be a ZST for `Error` to be a slim type. - detail: Detail, + info: ErrorInfo, } -/// An error object consists of both an error code as well as detailed error information for debugging. -/// -/// This is a type alias for the [`ErrorT`] type. -pub type Error = ErrorT; - -/// Specifies the default error detail for the `Error` type. -#[cfg(not(feature = "slim-errors"))] -pub type DefaultDetail = ErrorInfo; - -/// Specifies the default error detail for the `Error` type. -#[cfg(feature = "slim-errors")] -pub type DefaultDetail = NoDetail; - -/// On non-Windows platforms, `ErrorInfo` is mapped to `NoDetail` because non-Windows platforms -/// do not support `IErrorInfo`. -#[cfg(not(windows))] -pub type ErrorInfo = NoDetail; - -/// This type implements `ErrorDetail`, by containing no error information at all. This type allows -/// for "slim" `Error` objects, which have the same size as `HRESULT`. -#[derive(Default, Clone)] -pub struct NoDetail; - -/// Defines the behavior of error detail objects, which are stored within `Error`. -pub trait ErrorDetail: Sized + Default { - /// If this implementation stores error detail in ambient (thread-local) storage, then this - /// function transfers the error detail from ambient storage and returns it. - fn from_ambient() -> Self; - - /// If this implementation stores error detail in ambient (thread-local) storage, then this - /// function tranfers `self` into that ambient storage. - /// - /// If this implementation does not store error detail in ambient storage, then this function - /// discards the error detail. - fn into_ambient(self); - - /// If this implementation stores error detail in ambient (thread-local) storage, then this - /// function creates a new error detail object, given the `HRESULT` and a message string, - /// and stores the new error detail object in the ambient storage. - fn originate_error(code: HRESULT, message: &str); - - /// Returns a message describing the error, if available. - fn message(&self) -> Option; -} - -impl ErrorDetail for NoDetail { - fn from_ambient() -> Self { - Self - } - fn into_ambient(self) {} - fn originate_error(_code: HRESULT, _message: &str) {} - fn message(&self) -> Option { - None +/// We remap S_OK to this error because the S_OK representation (zero) is reserved for niche +/// optimizations. +const S_EMPTY_ERROR: NonZeroI32 = const_nonzero_i32(u32::from_be_bytes(*b"S_OK") as i32); + +/// Converts an HRESULT into a NonZeroI32. If the input is S_OK (zero), then this is converted to +/// S_EMPTY_ERROR. This is necessary because NonZeroI32, as the name implies, cannot represent the +/// value zero. So we remap it to a value no one should be using, during storage. +const fn const_nonzero_i32(i: i32) -> NonZeroI32 { + if let Some(nz) = NonZeroI32::new(i) { + nz + } else { + panic!(); } } -#[cfg(windows)] -pub use win_impl::ErrorInfo; - -#[cfg(windows)] -mod win_impl { - use super::*; - use crate::bstr::BasicString; - use crate::com::ComPtr; - - /// This type stores error detail, represented by a COM `IErrorInfo` object. - /// - /// # References - /// - /// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) - #[derive(Clone, Default)] - pub struct ErrorInfo { - pub(super) ptr: Option, +fn nonzero_hresult(hr: HRESULT) -> NonZeroI32 { + if let Some(nz) = NonZeroI32::new(hr.0) { + nz + } else { + S_EMPTY_ERROR } +} - unsafe impl Send for ErrorInfo {} - unsafe impl Sync for ErrorInfo {} - - impl ErrorInfo { - /// Gets a raw pointer to the `IErrorInfo` COM object stored in this `ErrorInfo`. - pub fn as_ptr(&self) -> *mut core::ffi::c_void { - self.ptr - .as_ref() - .map_or(core::ptr::null_mut(), |info| info.as_raw()) +impl Error { + /// Creates an error object without any failure information. + pub const fn empty() -> Self { + Self { + code: S_EMPTY_ERROR, + info: ErrorInfo::empty(), } } - impl ErrorDetail for ErrorInfo { - fn from_ambient() -> Self { - unsafe { - let mut ptr = None; - GetErrorInfo(0, &mut ptr as *mut _ as _); - Self { ptr } - } - } - - fn into_ambient(self) { - if let Some(ptr) = self.ptr { - unsafe { - SetErrorInfo(0, ptr.as_raw()); - } - } - } - - fn originate_error(code: HRESULT, message: &str) { - if message.is_empty() { - return; - } - - let mut wide_message: Vec = Vec::with_capacity(message.encode_utf16().count() + 1); - wide_message.extend(message.encode_utf16()); - let wide_message_len = wide_message.len(); // does not count NUL - wide_message.push(0); - - unsafe { - RoOriginateErrorW(code.0, wide_message_len as u32, wide_message.as_ptr()); - } - } - - fn message(&self) -> Option { - let info = self.ptr.as_ref()?; - - let mut message = BasicString::default(); - - // First attempt to retrieve the restricted error information. - if let Some(info) = info.cast(&IID_IRestrictedErrorInfo) { - let mut fallback = BasicString::default(); - let mut code = 0; - - unsafe { - com_call!( - IRestrictedErrorInfo_Vtbl, - info.GetErrorDetails( - &mut fallback as *mut _ as _, - &mut code, - &mut message as *mut _ as _, - &mut BasicString::default() as *mut _ as _ - ) - ); - } - - if message.is_empty() { - message = fallback - }; - } - - // Next attempt to retrieve the regular error information. - if message.is_empty() { - unsafe { - com_call!( - IErrorInfo_Vtbl, - info.GetDescription(&mut message as *mut _ as _) - ); - } - } - + /// Creates a new error object, capturing the stack and other information about the + /// point of failure. + pub fn new>(code: HRESULT, message: T) -> Self { + #[cfg(windows)] + { + let message: &str = message.as_ref(); if message.is_empty() { - return None; + Self::from_hresult(code) + } else { + ErrorInfo::originate_error(code, message); + code.into() } - - Some(String::from_utf16_lossy(crate::strings::wide_trim_end( - message.as_wide(), - ))) } - } -} - -impl ErrorT { - /// Creates an error object without any specific failure information. - /// - /// The `code()` for this error is `E_FAIL`. - pub fn fail() -> Self { - Self { - code: NonZeroHRESULT::E_FAIL, - detail: Detail::default(), + #[cfg(not(windows))] + { + let _ = message; + Self::from_hresult(code) } } /// Creates a new error object with an error code, but without additional error information. - /// - /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot - /// store an `S_OK` value. pub fn from_hresult(code: HRESULT) -> Self { Self { - code: NonZeroHRESULT::from_hresult_or_fail(code), - detail: Detail::default(), + code: nonzero_hresult(code), + info: ErrorInfo::empty(), } } /// Creates a new `Error` from the Win32 error code returned by `GetLastError()`. - /// - /// If `GetLastError()` returns `ERROR_SUCCESS` then this is remapped to `E_FAIL`. The `Error` - /// object cannot store an `S_OK` value. pub fn from_win32() -> Self { #[cfg(windows)] { - let hr = HRESULT::from_win32(unsafe { GetLastError() }); - Self::from_hresult(hr) + let error = unsafe { GetLastError() }; + Self::from_hresult(HRESULT::from_win32(error)) } #[cfg(not(windows))] { unimplemented!() } } -} -impl ErrorT { - /// Creates a new error object, capturing the stack and other information about the - /// point of failure. - /// - /// If `code` is `S_OK`, then this is remapped to `E_FAIL`. The `Error` object cannot - /// store an `S_OK` value. - pub fn new>(code: HRESULT, message: T) -> Self { - Detail::originate_error(code, message.as_ref()); - // This into() call will take the ambient thread-local error object, if any. - Self::from(code) + /// The error code describing the error. + pub const fn code(&self) -> HRESULT { + if self.code.get() == S_EMPTY_ERROR.get() { + HRESULT(0) + } else { + HRESULT(self.code.get()) + } } /// The error message describing the error. pub fn message(&self) -> String { - if let Some(message) = self.detail.message() { + if let Some(message) = self.info.message() { return message; } // Otherwise fallback to a generic error code description. - self.code.message() + self.code().message() } -} -impl ErrorT { - /// Gets access to the error detail stored in this `Error`. - pub fn detail(&self) -> &Detail { - &self.detail - } - - /// The error code describing the error. - pub const fn code(&self) -> HRESULT { - self.code.to_hresult() + /// The error object describing the error. + #[cfg(windows)] + pub fn as_ptr(&self) -> *mut core::ffi::c_void { + self.info.as_ptr() } } #[cfg(feature = "std")] -impl std::error::Error for ErrorT {} +impl std::error::Error for Error {} -impl From> for HRESULT { - fn from(error: ErrorT) -> Self { - error.detail.into_ambient(); - error.code.into() +impl From for HRESULT { + fn from(error: Error) -> Self { + let code = error.code(); + error.info.into_thread(); + code } } -impl From for ErrorT { +impl From for Error { fn from(code: HRESULT) -> Self { Self { - code: NonZeroHRESULT::from_hresult_or_fail(code), - detail: Detail::from_ambient(), + code: nonzero_hresult(code), + info: ErrorInfo::from_thread(), } } } #[cfg(feature = "std")] -impl From> for std::io::Error { - fn from(from: ErrorT) -> Self { - Self::from_raw_os_error(from.code.0.get()) +impl From for std::io::Error { + fn from(from: Error) -> Self { + Self::from_raw_os_error(from.code().0) } } #[cfg(feature = "std")] -impl From for ErrorT { +impl From for Error { fn from(from: std::io::Error) -> Self { match from.raw_os_error() { Some(status) => HRESULT::from_win32(status as u32).into(), @@ -320,35 +178,35 @@ impl From for ErrorT { } } -impl From for ErrorT { +impl From for Error { fn from(_: alloc::string::FromUtf16Error) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for ErrorT { +impl From for Error { fn from(_: alloc::string::FromUtf8Error) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_NO_UNICODE_TRANSLATION)) } } -impl From for ErrorT { +impl From for Error { fn from(_: core::num::TryFromIntError) -> Self { Self::from_hresult(HRESULT::from_win32(ERROR_INVALID_DATA)) } } -impl core::fmt::Debug for ErrorT { +impl core::fmt::Debug for Error { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let mut debug = fmt.debug_struct("Error"); debug - .field("code", &self.code) + .field("code", &self.code()) .field("message", &self.message()) .finish() } } -impl core::fmt::Display for ErrorT { +impl core::fmt::Display for Error { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let message = self.message(); if message.is_empty() { @@ -359,39 +217,174 @@ impl core::fmt::Display for ErrorT { } } -impl core::hash::Hash for ErrorT { +impl core::hash::Hash for Error { fn hash(&self, state: &mut H) { self.code.hash(state); - // We do not hash the detail. + // We do not hash the error info. } } // Equality tests only the HRESULT, not the error info (if any). -impl PartialEq for ErrorT { +impl PartialEq for Error { fn eq(&self, other: &Self) -> bool { self.code == other.code } } -impl Eq for ErrorT {} +impl Eq for Error {} -impl PartialOrd for ErrorT { +impl PartialOrd for Error { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -impl Ord for ErrorT { +impl Ord for Error { fn cmp(&self, other: &Self) -> core::cmp::Ordering { self.code.cmp(&other.code) } } -// If we are using "slim" Error objects, then we can rely on Result<()> -// having a representation that is equivalent to HRESULT. -static_assertions::const_assert_eq!( - size_of::>>(), - size_of::() -); - static_assertions::assert_impl_all!(Error: Send, Sync); + +use error_info::*; + +#[cfg(all(windows, feature = "error-info", not(feature = "disable-error-info")))] +mod error_info { + use super::*; + use crate::com::ComPtr; + + /// This type stores error detail, represented by a COM `IErrorInfo` object. + /// + /// # References + /// + /// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) + #[derive(Clone, Default)] + pub(crate) struct ErrorInfo { + pub(super) ptr: Option, + } + + impl ErrorInfo { + pub(crate) const fn empty() -> Self { + Self { ptr: None } + } + + pub(crate) fn from_thread() -> Self { + unsafe { + let mut ptr = core::mem::MaybeUninit::zeroed(); + crate::bindings::GetErrorInfo(0, ptr.as_mut_ptr() as *mut _); + Self { + ptr: ptr.assume_init(), + } + } + } + + pub(crate) fn into_thread(self) { + if let Some(ptr) = self.ptr { + unsafe { + crate::bindings::SetErrorInfo(0, ptr.as_raw()); + } + } + } + + #[cfg(windows)] + pub(crate) fn originate_error(code: HRESULT, message: &str) { + let message: Vec<_> = message.encode_utf16().collect(); + unsafe { + RoOriginateErrorW(code.0, message.len() as u32, message.as_ptr()); + } + } + + pub(crate) fn message(&self) -> Option { + use crate::bstr::BasicString; + + let ptr = self.ptr.as_ref()?; + + let mut message = BasicString::default(); + + // First attempt to retrieve the restricted error information. + if let Some(info) = ptr.cast(&IID_IRestrictedErrorInfo) { + let mut fallback = BasicString::default(); + let mut code = 0; + + unsafe { + com_call!( + IRestrictedErrorInfo_Vtbl, + info.GetErrorDetails( + &mut fallback as *mut _ as _, + &mut code, + &mut message as *mut _ as _, + &mut BasicString::default() as *mut _ as _ + ) + ); + } + + if message.is_empty() { + message = fallback + }; + } + + // Next attempt to retrieve the regular error information. + if message.is_empty() { + unsafe { + com_call!( + IErrorInfo_Vtbl, + ptr.GetDescription(&mut message as *mut _ as _) + ); + } + } + + Some(String::from_utf16_lossy(wide_trim_end(message.as_wide()))) + } + + pub(crate) fn as_ptr(&self) -> *mut core::ffi::c_void { + if let Some(info) = self.ptr.as_ref() { + info.as_raw() + } else { + core::ptr::null_mut() + } + } + } + + unsafe impl Send for ErrorInfo {} + unsafe impl Sync for ErrorInfo {} +} + +#[cfg(not(all(windows, feature = "error-info", not(feature = "disable-error-info"))))] +mod error_info { + use super::*; + + #[derive(Clone, Default)] + pub(crate) struct ErrorInfo; + + impl ErrorInfo { + pub(crate) const fn empty() -> Self { + Self + } + + pub(crate) fn from_thread() -> Self { + Self + } + + pub(crate) fn into_thread(self) {} + + #[cfg(windows)] + pub(crate) fn originate_error(_code: HRESULT, _message: &str) {} + + pub(crate) fn message(&self) -> Option { + None + } + + #[cfg(windows)] + pub(crate) fn as_ptr(&self) -> *mut core::ffi::c_void { + core::ptr::null_mut() + } + } + + // If we are using "slim" Error objects, then we can rely on Result<()> + // having a representation that is equivalent to HRESULT. + static_assertions::const_assert_eq!( + size_of::>(), + size_of::() + ); +} diff --git a/crates/libs/result/src/hresult.rs b/crates/libs/result/src/hresult.rs index 0d5f0aa367..68587ecf6c 100644 --- a/crates/libs/result/src/hresult.rs +++ b/crates/libs/result/src/hresult.rs @@ -1,7 +1,4 @@ use super::*; -use core::mem::size_of; -use core::num::NonZeroI32; -use static_assertions::const_assert_eq; /// An error code value returned by most COM functions. #[repr(transparent)] @@ -69,7 +66,7 @@ impl HRESULT { pub fn message(self) -> String { #[cfg(windows)] { - let mut message = crate::strings::HeapString::default(); + let mut message = HeapString::default(); let mut code = self.0; let mut module = core::ptr::null_mut(); @@ -100,8 +97,10 @@ impl HRESULT { ); if !message.0.is_null() && size > 0 { - let message_slice = core::slice::from_raw_parts(message.0, size as usize); - String::from_utf16_lossy(crate::strings::wide_trim_end(message_slice)) + String::from_utf16_lossy(wide_trim_end(core::slice::from_raw_parts( + message.0, + size as usize, + ))) } else { String::default() } @@ -110,7 +109,7 @@ impl HRESULT { #[cfg(not(windows))] { - return format!("0x{:08x}", self.0 as u32); + return alloc::format!("0x{:08x}", self.0 as u32); } } @@ -153,161 +152,3 @@ impl core::fmt::Debug for HRESULT { f.write_fmt(format_args!("HRESULT({})", self)) } } - -impl core::fmt::Display for NonZeroHRESULT { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - ::fmt(&HRESULT::from(*self), f) - } -} - -impl core::fmt::Debug for NonZeroHRESULT { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - ::fmt(&HRESULT::from(*self), f) - } -} - -/// An error code value returned by most COM functions. -/// -/// This type cannot represent `S_OK`. This type is intended for use with either -/// `Option` or `Result` so that the unused 0 bit pattern can be used as a "niche", -/// which allows `Option` and `Result` to use the bit pattern and conserve space. -#[repr(transparent)] -#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -#[must_use] -#[allow(non_camel_case_types)] -pub struct NonZeroHRESULT(pub NonZeroI32); - -const_assert_eq!(size_of::(), size_of::()); - -// Check the Option niche optimization. -const_assert_eq!(size_of::>(), size_of::()); - -// Check the Result niche optimization. -const_assert_eq!( - size_of::>(), - size_of::() -); - -impl NonZeroHRESULT { - /// Returns [`true`] if `self` is a success code. - /// - /// Although `NonZeroHRESULT` cannot represent `S_OK`, it can represent other success values - /// such as `S_FALSE`. - #[inline] - pub const fn is_ok(self) -> bool { - self.0.get() >= 0 - } - - /// Returns [`true`] if `self` is a failure code. - #[inline] - pub const fn is_err(self) -> bool { - self.0.get() < 0 - } - - /// Asserts that `self` is a success code. - /// - /// This will invoke the [`panic!`] macro if `self` is a failure code and display - /// the [`HRESULT`] value for diagnostics. - #[inline] - #[track_caller] - pub fn unwrap(self) { - assert!(self.is_ok(), "HRESULT 0x{:X}", self.0.get()); - } - - /// Converts the [`HRESULT`] to [`Result<()>`][Result<_>]. - #[inline] - pub fn ok(self) -> Result<()> { - // SAFETY: We use a static assertion to check that the size of these two types are identical. - // Since there is only one possible niche, that niche must be used for the Ok(()) value. - #[cfg(any(feature = "slim-errors", not(windows)))] - unsafe { - core::mem::transmute(self) - } - - // If we are not using slim Error, then we take the slow path. - #[cfg(not(any(feature = "slim-errors", not(windows))))] - HRESULT::from(self).ok() - } - - /// Calls `op` if `self` is a success code, otherwise returns [`HRESULT`] - /// converted to [`Result`]. - #[inline] - pub fn map(self, op: F) -> Result - where - F: FnOnce() -> T, - { - self.ok()?; - Ok(op()) - } - - /// Calls `op` if `self` is a success code, otherwise returns [`HRESULT`] - /// converted to [`Result`]. - #[inline] - pub fn and_then(self, op: F) -> Result - where - F: FnOnce() -> Result, - { - self.ok()?; - op() - } - - /// The error message describing the error. - pub fn message(self) -> String { - HRESULT::from(self).message() - } - - /// Converts `hr` to `NonZeroHRESULT`. If `hr` is `S_OK` (zero), then this returns `None`. - pub const fn from_hresult_opt(hr: HRESULT) -> Option { - // SAFETY: The niche optimization is guaranteed for Option. This transmute - // exploits the same niche optimization, only for an HRESULT that wraps an i32. - unsafe { core::mem::transmute(hr) } - } - - /// Converts `HRESULT` to `NonZeroHRESULT`. - /// - /// This is intended only for use in constant declarations. This will panic if `error` is 0. - pub const fn from_hresult_unwrap(hr: HRESULT) -> Self { - if let Some(nz) = NonZeroI32::new(hr.0) { - Self(nz) - } else { - panic!("`hr` cannot be S_OK (zero)"); - } - } - - /// The well-known `E_FAIL` constant. - pub const E_FAIL: NonZeroHRESULT = Self::from_hresult_unwrap(HRESULT(0x80004005_u32 as i32)); - - /// Converts `hr` to `NonZeroHRESULT`. If `hr` is `S_OK`, then this returns `E_FAIL`. - pub const fn from_hresult_or_fail(hr: HRESULT) -> Self { - if let Some(nz) = NonZeroI32::new(hr.0) { - Self(nz) - } else { - Self::E_FAIL - } - } - - /// Maps a Win32 error code to a NonZeroHRESULT value. If `error` is 0, then this will - /// return `E_FAIL`. - pub const fn from_win32(error: u32) -> Self { - Self::from_hresult_or_fail(HRESULT::from_win32(error)) - } - - /// Maps an NT error code to an HRESULT value. - /// - /// This is intended only for use in constant declarations. If `error` is 0, then this will - /// return `E_FAIL`. - pub const fn from_nt(error: i32) -> Self { - Self::from_hresult_or_fail(HRESULT::from_nt(error)) - } - - /// Converts this `NonZeroHRESULT` to an `HRESULT`. This is usable in `const fn`. - pub const fn to_hresult(self) -> HRESULT { - HRESULT(self.0.get()) - } -} - -impl From for HRESULT { - fn from(hr: NonZeroHRESULT) -> Self { - Self(hr.0.get()) - } -} diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index e18fdcff73..30babbfb1e 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -6,7 +6,7 @@ Learn more about Rust for Windows here: = core::result::Result; diff --git a/crates/libs/strings/Cargo.toml b/crates/libs/strings/Cargo.toml index 46643e0ced..8aff1cf960 100644 --- a/crates/libs/strings/Cargo.toml +++ b/crates/libs/strings/Cargo.toml @@ -24,6 +24,7 @@ path = "../targets" [dependencies.windows-result] version = "0.1.1" path = "../result" +default-features = false [features] default = ["std"] diff --git a/crates/libs/windows/Cargo.toml b/crates/libs/windows/Cargo.toml index 102324e8bf..eb050f84ce 100644 --- a/crates/libs/windows/Cargo.toml +++ b/crates/libs/windows/Cargo.toml @@ -36,7 +36,7 @@ docs = [] deprecated = [] implement = [] std = ["windows-core/std"] -slim-errors = ["windows-core/slim-errors"] +error-info = ["windows-core/error-info"] # generated features AI = ["Foundation"] AI_MachineLearning = ["AI"] diff --git a/crates/tests/core/tests/error.rs b/crates/tests/core/tests/error.rs index 82b162bbc5..f1cb505b17 100644 --- a/crates/tests/core/tests/error.rs +++ b/crates/tests/core/tests/error.rs @@ -67,5 +67,5 @@ fn suppressed_error_info() -> Result<()> { fn just_hresult() { let e: Error = E_NOTIMPL.into(); assert!(e.code() == E_NOTIMPL); - assert!(e.detail().as_ptr().is_null()); + assert!(e.as_ptr().is_null()); } diff --git a/crates/tests/implement/tests/data_object.rs b/crates/tests/implement/tests/data_object.rs index 361ad3741a..a53787e290 100644 --- a/crates/tests/implement/tests/data_object.rs +++ b/crates/tests/implement/tests/data_object.rs @@ -58,7 +58,7 @@ impl IDataObject_Impl for Test_Impl { fn EnumFormatEtc(&self, _: u32) -> Result { unsafe { (*self.0.get()).EnumFormatEtc = true; - Err(Error::fail()) + Err(Error::empty()) } } @@ -79,7 +79,7 @@ impl IDataObject_Impl for Test_Impl { fn EnumDAdvise(&self) -> Result { unsafe { (*self.0.get()).EnumDAdvise = true; - Err(Error::fail()) + Err(Error::empty()) } } } @@ -99,8 +99,8 @@ fn test() -> Result<()> { let r = d.EnumFormatEtc(0); assert!(r.is_err()); let e = r.unwrap_err(); - assert!(e.code() == E_FAIL); - assert!(e.detail().as_ptr().is_null()); + assert!(e.code() == S_OK); + assert!(e.as_ptr().is_null()); d.DAdvise(&Default::default(), 0, None)?; diff --git a/crates/tests/implement/tests/vector.rs b/crates/tests/implement/tests/vector.rs index da00164180..9c3c2e03a5 100644 --- a/crates/tests/implement/tests/vector.rs +++ b/crates/tests/implement/tests/vector.rs @@ -184,7 +184,7 @@ fn GetAt() -> Result<()> { ]) .into(); assert_eq!(v.GetAt(0)?.ToString()?, "http://test/"); - assert_eq!(v.GetAt(1).unwrap_err().code(), E_POINTER); + assert_eq!(v.GetAt(1).unwrap_err().code(), S_OK); Ok(()) } diff --git a/crates/tests/linux/tests/result.rs b/crates/tests/linux/tests/result.rs index 824bf0e942..3414132583 100644 --- a/crates/tests/linux/tests/result.rs +++ b/crates/tests/linux/tests/result.rs @@ -18,7 +18,7 @@ fn basic_hresult() { fn error_message_is_not_supported() { let e = Error::new(S_OK, "this gets ignored"); let message = e.message(); - assert_eq!(message, "0x80004005"); + assert_eq!(message, "0x00000000"); } #[test] diff --git a/crates/tests/noexcept/tests/test.rs b/crates/tests/noexcept/tests/test.rs index 5ee47476f3..5e6e6f54d5 100644 --- a/crates/tests/noexcept/tests/test.rs +++ b/crates/tests/noexcept/tests/test.rs @@ -42,7 +42,7 @@ impl ITest_Impl for Test_Impl { } fn Test(&self) -> Result { let this = self.0.read().unwrap(); - this.2.clone().ok_or_else(Error::fail) + this.2.clone().ok_or_else(Error::empty) } fn SetTest(&self, value: Option<&ITest>) -> Result<()> { let mut this = self.0.write().unwrap(); diff --git a/crates/tests/result/tests/error.rs b/crates/tests/result/tests/error.rs index e5dac491c5..43b5b20af5 100644 --- a/crates/tests/result/tests/error.rs +++ b/crates/tests/result/tests/error.rs @@ -1,23 +1,30 @@ use windows_result::*; +const S_OK: HRESULT = HRESULT(0); const E_INVALIDARG: HRESULT = HRESULT(-2147024809i32); -const E_FAIL: HRESULT = HRESULT(0x80004005_u32 as i32); const ERROR_CANCELLED: u32 = 1223; const ERROR_INVALID_DATA: u32 = 13; const E_CANCELLED: HRESULT = HRESULT::from_win32(ERROR_CANCELLED); windows_targets::link!("kernel32.dll" "system" fn SetLastError(code: u32)); +#[test] +fn empty() { + const EMPTY: Error = Error::empty(); + assert_eq!(EMPTY.code(), S_OK); + assert!(EMPTY.as_ptr().is_null()); + assert_eq!(EMPTY.message(), "The operation completed successfully."); +} + #[test] fn new() { let e = Error::new(E_INVALIDARG, ""); assert_eq!(e.code(), E_INVALIDARG); - assert!(e.detail().as_ptr().is_null()); - assert_eq!(e.message(), "The parameter is incorrect."); + assert!(e.as_ptr().is_null()); let e = Error::new(E_INVALIDARG, "test message"); assert_eq!(e.code(), E_INVALIDARG); - assert!(!e.detail().as_ptr().is_null()); + assert!(!e.as_ptr().is_null()); assert_eq!(e.message(), "test message"); } @@ -25,7 +32,7 @@ fn new() { fn from_hresult() { let e = Error::from_hresult(E_INVALIDARG); assert_eq!(e.code(), E_INVALIDARG); - assert!(e.detail().as_ptr().is_null()); + assert!(e.as_ptr().is_null()); assert_eq!(e.message(), "The parameter is incorrect."); } @@ -34,13 +41,13 @@ fn from_win32() { unsafe { SetLastError(0) }; let e = Error::from_win32(); - assert_eq!(e.code(), E_FAIL); - assert!(e.detail().as_ptr().is_null()); + assert_eq!(e.code(), S_OK); + assert!(e.as_ptr().is_null()); unsafe { SetLastError(ERROR_CANCELLED) }; let e = Error::from_win32(); - assert!(e.detail().as_ptr().is_null()); + assert!(e.as_ptr().is_null()); assert_eq!(e.code(), E_CANCELLED); } diff --git a/crates/tests/winrt/tests/collisions.rs b/crates/tests/winrt/tests/collisions.rs index 482fadabf9..9c7c64e5d2 100644 --- a/crates/tests/winrt/tests/collisions.rs +++ b/crates/tests/winrt/tests/collisions.rs @@ -1,5 +1,5 @@ use windows::{ - core::{imp::E_POINTER, HSTRING}, + core::HSTRING, ApplicationModel::Email::EmailAttachment, Devices::WiFiDirect::{ WiFiDirectConnectionParameters, WiFiDirectDevice, WiFiDirectDeviceSelectorType, @@ -16,10 +16,7 @@ fn wifi() -> windows::core::Result<()> { assert!(!a.is_empty()); // from_id_async from IWiFiDirectDeviceStatics - assert!( - WiFiDirectDevice::FromIdAsync(&a)?.get() - == Err(windows::core::Error::from_hresult(E_POINTER)) - ); + assert!(WiFiDirectDevice::FromIdAsync(&a)?.get() == Err(windows::core::Error::empty())); // get_device_selector overload from IWiFiDirectDeviceStatics2 is renamed to get_device_selector2 let c = WiFiDirectDevice::GetDeviceSelector2(WiFiDirectDeviceSelectorType::DeviceInterface)?; From cf1079229ef4647a17b1c9a51b091a249ce2a444 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 09:51:02 -0700 Subject: [PATCH 11/15] fix dead code warning in ComPtr, update natvis --- crates/libs/result/.natvis | 13 +++++++++---- crates/libs/result/src/error.rs | 8 ++++++-- crates/libs/result/src/lib.rs | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/crates/libs/result/.natvis b/crates/libs/result/.natvis index cc5da51de3..036d0c86de 100644 --- a/crates/libs/result/.natvis +++ b/crates/libs/result/.natvis @@ -1,17 +1,22 @@  + {(HRESULT)code.__0.__0} code - detail + (HRESULT)code.__0.__0 + info - {(::HRESULT)__0} + {(HRESULT)__0} - - {(::HRESULT)__0.__0.__0} + + ErrorInfo + + *(void**)&ptr + diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index 04a9f9b3ad..645c7284c5 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -354,10 +354,14 @@ mod error_info { mod error_info { use super::*; + // We use this name so that the NatVis element for ErrorInfo does *not* match this type. + // This prevents the NatVis description from failing to load. #[derive(Clone, Default)] - pub(crate) struct ErrorInfo; + pub(crate) struct EmptyErrorInfo; - impl ErrorInfo { + pub(crate) use EmptyErrorInfo as ErrorInfo; + + impl EmptyErrorInfo { pub(crate) const fn empty() -> Self { Self } diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index 30babbfb1e..95fcf98851 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -17,7 +17,7 @@ use alloc::{string::String, vec::Vec}; mod bindings; use bindings::*; -#[cfg(windows)] +#[cfg(all(windows, feature = "error-info", not(feature = "disable-error-info")))] mod com; #[cfg(windows)] From 9f5c73d1cc8aeed1d030fb637ca2e0eb9059cd8c Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 10:46:11 -0700 Subject: [PATCH 12/15] specify features in a sample --- crates/libs/windows/Cargo.toml | 2 +- crates/samples/components/json_validator/Cargo.toml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/libs/windows/Cargo.toml b/crates/libs/windows/Cargo.toml index eb050f84ce..59649b2a2b 100644 --- a/crates/libs/windows/Cargo.toml +++ b/crates/libs/windows/Cargo.toml @@ -27,7 +27,7 @@ targets = [] rustdoc-args = ["--cfg", "docsrs"] [dependencies] -windows-core = { path = "../core", version = "0.57.0" } +windows-core = { path = "../core", version = "0.57.0", default-features = false } windows-targets = { path = "../targets", version = "0.52.5" } [features] diff --git a/crates/samples/components/json_validator/Cargo.toml b/crates/samples/components/json_validator/Cargo.toml index 1b820a3337..8cdbf11a07 100644 --- a/crates/samples/components/json_validator/Cargo.toml +++ b/crates/samples/components/json_validator/Cargo.toml @@ -16,4 +16,5 @@ path = "../../../libs/windows" features = [ "Win32_Foundation", "Win32_System_Com", + "error-info", ] From 00d73453ad97807484b9562301616a4fd5cc5d4c Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 11:18:50 -0700 Subject: [PATCH 13/15] switch to --cfg --- .github/workflows/msrv-windows-result.yml | 10 ++++++ Cargo.toml | 2 +- crates/libs/core/Cargo.toml | 4 +-- crates/libs/result/Cargo.toml | 9 +---- crates/libs/result/src/error.rs | 33 ++++++++++++------- crates/libs/result/src/lib.rs | 4 +-- crates/libs/windows/Cargo.toml | 1 - .../components/json_validator/Cargo.toml | 1 - 8 files changed, 37 insertions(+), 27 deletions(-) diff --git a/.github/workflows/msrv-windows-result.yml b/.github/workflows/msrv-windows-result.yml index baef56a15a..602c22b9d1 100644 --- a/.github/workflows/msrv-windows-result.yml +++ b/.github/workflows/msrv-windows-result.yml @@ -29,3 +29,13 @@ jobs: run: cargo check -p windows-result --all-features - name: Check Default Features run: cargo check -p windows-result + - name: Check Slim Errors + shell: pwsh + run: | + $ErrorActionPreference = 'Stop' + $env:RUSTFLAGS = '--cfg=windows_slim_errors' + + # This will show the size of Error, which lets us confirm that RUSTFLAGS was set. + cargo test -p windows-result --lib -- --nocapture --test-threads=1 + + cargo check -p windows-result diff --git a/Cargo.toml b/Cargo.toml index 00ab9b32b3..ec929dcdb0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,4 +16,4 @@ exclude = [ [workspace.lints.rust] rust_2018_idioms = { level = "warn", priority = -1 } missing_docs = "warn" -unexpected_cfgs = { level = "warn", check-cfg = ['cfg(windows_raw_dylib, windows_debugger_visualizer)'] } +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(windows_raw_dylib, windows_debugger_visualizer, windows_slim_errors)'] } diff --git a/crates/libs/core/Cargo.toml b/crates/libs/core/Cargo.toml index 4113fe1320..52cb1fdf2a 100644 --- a/crates/libs/core/Cargo.toml +++ b/crates/libs/core/Cargo.toml @@ -24,7 +24,6 @@ path = "../targets" [dependencies.windows-result] version = "0.1.1" path = "../result" -default-features = false [dependencies.windows-strings] version = "0.1.0" @@ -36,5 +35,4 @@ windows-interface = { path = "../interface", version = "0.57.0" } [features] default = ["std"] -std = ["windows-result/std"] -error-info = ["windows-result/error-info"] +std = [] diff --git a/crates/libs/result/Cargo.toml b/crates/libs/result/Cargo.toml index bbb7bd33e5..6c65a5e8f0 100644 --- a/crates/libs/result/Cargo.toml +++ b/crates/libs/result/Cargo.toml @@ -11,15 +11,8 @@ readme = "readme.md" categories = ["os::windows-apis"] [features] -default = ["std", "error-info"] +default = ["std"] std = [] -error-info = [] - -# The disable-error-info feature forcibly disables error-info (and overrides it). -# This should _not_ be the preferred way to disable error info, but is necessary -# in case a third-party crate has a dependency on Windows crates but does not -# provide any way to control its own feature selection. -disable-error-info = [] [lints] workspace = true diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index 645c7284c5..e387b3838f 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -6,7 +6,7 @@ use core::mem::size_of; /// An error object consists of both an error code and optional detailed error information for debugging. /// -/// # Extended error info and the `error-info` feature +/// # Extended error info and the `windows_slim_errors` configuration option /// /// `Error` contains an [`HRESULT`] value that describes the error, as well as an optional /// `IErrorInfo` COM object. The `IErrorInfo` object is a COM object that can provide detailed information @@ -18,15 +18,26 @@ use core::mem::size_of; /// info within `Error` has no benefits, but has substantial costs because it increases the size of /// the `Error` object, which also increases the size of `Result`. /// -/// The `error-info` Cargo feature allows you to control whether `IErrorInfo` support is enabled -/// at compile time. The `error-info` feature is _enabled_ by default, so if you want to disable it -/// you will need to disable default features. +/// This error information can be disabled at compile time by setting `RUSTFLAGS=--cfg=windows_slim_errors`. +/// This removes the `IErrorInfo` support within the [`Error`] type, which has these benefits: /// -/// When the `error-info` feature is disabled, -/// This allows the `Error` and even the `Result<(), Error>` types to have a very small size; they -/// have the same size and alignment as `HRESULT` itself. Because of this, they can be returned -/// directly in a machine register, which gives better performance than writing the (relatively -/// large) `Error` object (with full error detail) to the stack on every function return. +/// * It reduces the size of [`Error`] to 4 bytes (the size of [`HRESULT`]). +/// +/// * It reduces the size of `Result<(), Error>` to 4 bytes, allowing it to be returned in a single +/// machine register. +/// +/// * The `Error` (and `Result`) types no longer have a [`Drop`] impl. This removes the need +/// for lifetime checking and running drop code when [`Error`] and [`Result`] go out of scope. This +/// significantly reduces code size for codebase that make extensive use of [`Error`]. +/// +/// Of course, these benefits come with a cost; you lose extended error information for those +/// COM objects that support it. +/// +/// This is controlled by a `--cfg` option rather than a Cargo feature because this compilation +/// option sets a policy that applies to an entire graph of crates. Individual crates that take a +/// dependency on the `windows-result` crate are not in a good position to decide whether they want +/// slim errors or full errors. Cargo features are meant to be additive, but specifying the size +/// and contents of `Error` is not a feature so much as a whole-program policy decision. /// /// # References /// @@ -249,7 +260,7 @@ static_assertions::assert_impl_all!(Error: Send, Sync); use error_info::*; -#[cfg(all(windows, feature = "error-info", not(feature = "disable-error-info")))] +#[cfg(all(windows, not(windows_slim_errors)))] mod error_info { use super::*; use crate::com::ComPtr; @@ -350,7 +361,7 @@ mod error_info { unsafe impl Sync for ErrorInfo {} } -#[cfg(not(all(windows, feature = "error-info", not(feature = "disable-error-info"))))] +#[cfg(not(all(windows, not(windows_slim_errors))))] mod error_info { use super::*; diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index 95fcf98851..3207acfdc4 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -17,7 +17,7 @@ use alloc::{string::String, vec::Vec}; mod bindings; use bindings::*; -#[cfg(all(windows, feature = "error-info", not(feature = "disable-error-info")))] +#[cfg(all(windows, not(windows_slim_errors)))] mod com; #[cfg(windows)] @@ -25,7 +25,7 @@ mod strings; #[cfg(windows)] use strings::*; -#[cfg(all(windows, feature = "error-info", not(feature = "disable-error-info")))] +#[cfg(all(windows, not(windows_slim_errors)))] mod bstr; mod error; diff --git a/crates/libs/windows/Cargo.toml b/crates/libs/windows/Cargo.toml index 59649b2a2b..8552d4ec47 100644 --- a/crates/libs/windows/Cargo.toml +++ b/crates/libs/windows/Cargo.toml @@ -36,7 +36,6 @@ docs = [] deprecated = [] implement = [] std = ["windows-core/std"] -error-info = ["windows-core/error-info"] # generated features AI = ["Foundation"] AI_MachineLearning = ["AI"] diff --git a/crates/samples/components/json_validator/Cargo.toml b/crates/samples/components/json_validator/Cargo.toml index 8cdbf11a07..1b820a3337 100644 --- a/crates/samples/components/json_validator/Cargo.toml +++ b/crates/samples/components/json_validator/Cargo.toml @@ -16,5 +16,4 @@ path = "../../../libs/windows" features = [ "Win32_Foundation", "Win32_System_Com", - "error-info", ] From f04a94943812222d972fb80aee06c48bb475ec77 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 11:35:10 -0700 Subject: [PATCH 14/15] natvis --- crates/libs/result/.natvis | 5 ++--- crates/libs/result/src/error.rs | 5 +++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/libs/result/.natvis b/crates/libs/result/.natvis index 036d0c86de..26432e7fad 100644 --- a/crates/libs/result/.natvis +++ b/crates/libs/result/.natvis @@ -1,11 +1,10 @@  - {(HRESULT)code.__0.__0} - code + (HRESULT)code.__0.__0,hr (HRESULT)code.__0.__0 - info + info.ptr diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index e387b3838f..6d41719bf9 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -44,9 +44,10 @@ use core::mem::size_of; /// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) #[derive(Clone)] pub struct Error { - /// The `HRESULT` error code, but represented using `NonZeroI32`. The `NonZeroHRESULT` provides + /// The `HRESULT` error code, but represented using [`NonZeroI32`]. [`NonZeroI32`] provides /// a "niche" to the Rust compiler, which is a space-saving optimization. This allows the - /// compiler to use more compact representations in some sitatutions. + /// compiler to use more compact representation for enum variants (such as [`Result`]) that + /// contain instances of [`Error`]. code: NonZeroI32, /// Contains details about the error, such as error text. From 7e5c760a270c2cd731b929a1bce89197118766ef Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 26 Jun 2024 11:51:53 -0700 Subject: [PATCH 15/15] pr feedback --- crates/libs/result/src/error.rs | 1 - crates/libs/windows/Cargo.toml | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/libs/result/src/error.rs b/crates/libs/result/src/error.rs index 6d41719bf9..64d6c4c3bb 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -299,7 +299,6 @@ mod error_info { } } - #[cfg(windows)] pub(crate) fn originate_error(code: HRESULT, message: &str) { let message: Vec<_> = message.encode_utf16().collect(); unsafe { diff --git a/crates/libs/windows/Cargo.toml b/crates/libs/windows/Cargo.toml index 8552d4ec47..cec03168ca 100644 --- a/crates/libs/windows/Cargo.toml +++ b/crates/libs/windows/Cargo.toml @@ -27,7 +27,7 @@ targets = [] rustdoc-args = ["--cfg", "docsrs"] [dependencies] -windows-core = { path = "../core", version = "0.57.0", default-features = false } +windows-core = { path = "../core", version = "0.57.0" } windows-targets = { path = "../targets", version = "0.52.5" } [features]