diff --git a/.github/workflows/msrv-windows-result.yml b/.github/workflows/msrv-windows-result.yml index 9e1067cb18..602c22b9d1 100644 --- a/.github/workflows/msrv-windows-result.yml +++ b/.github/workflows/msrv-windows-result.yml @@ -27,3 +27,15 @@ 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 + - 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/result/.natvis b/crates/libs/result/.natvis index 0d373c2529..26432e7fad 100644 --- a/crates/libs/result/.natvis +++ b/crates/libs/result/.natvis @@ -1,13 +1,21 @@ - + - code - info + (HRESULT)code.__0.__0,hr + (HRESULT)code.__0.__0 + info.ptr {(HRESULT)__0} + + + ErrorInfo + + *(void**)&ptr + + diff --git a/crates/libs/result/Cargo.toml b/crates/libs/result/Cargo.toml index f553397c3c..6c65a5e8f0 100644 --- a/crates/libs/result/Cargo.toml +++ b/crates/libs/result/Cargo.toml @@ -21,6 +21,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..64d6c4c3bb 100644 --- a/crates/libs/result/src/error.rs +++ b/crates/libs/result/src/error.rs @@ -1,21 +1,88 @@ use super::*; -use core::ffi::c_void; +use core::num::NonZeroI32; -/// An error object consists of both an error code as well as detailed error information for debugging. -#[derive(Clone, PartialEq, Eq)] +#[allow(unused_imports)] +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 `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 +/// 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`. +/// +/// 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: +/// +/// * 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 +/// +/// * [`IErrorInfo`](https://learn.microsoft.com/en-us/windows/win32/api/oaidl/nn-oaidl-ierrorinfo) +#[derive(Clone)] pub struct Error { - code: HRESULT, - #[cfg(windows)] - info: Option, + /// 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 representation for enum variants (such as [`Result`]) that + /// contain instances of [`Error`]. + code: NonZeroI32, + + /// Contains details about the error, such as error text. + info: ErrorInfo, +} + +/// 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!(); + } +} + +fn nonzero_hresult(hr: HRESULT) -> NonZeroI32 { + if let Some(nz) = NonZeroI32::new(hr.0) { + nz + } else { + S_EMPTY_ERROR + } } impl Error { /// Creates an error object without any failure information. pub const fn empty() -> Self { Self { - code: HRESULT(0), - #[cfg(windows)] - info: None, + code: S_EMPTY_ERROR, + info: ErrorInfo::empty(), } } @@ -24,13 +91,11 @@ impl Error { pub fn new>(code: HRESULT, message: T) -> Self { #[cfg(windows)] { - let message: Vec<_> = message.as_ref().encode_utf16().collect(); + let message: &str = message.as_ref(); if message.is_empty() { Self::from_hresult(code) } else { - unsafe { - RoOriginateErrorW(code.0, message.len() as u32, message.as_ptr()); - } + ErrorInfo::originate_error(code, message); code.into() } } @@ -44,9 +109,8 @@ impl Error { /// 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: nonzero_hresult(code), + info: ErrorInfo::empty(), } } @@ -54,10 +118,8 @@ impl Error { pub fn from_win32() -> Self { #[cfg(windows)] { - Self { - code: HRESULT::from_win32(unsafe { GetLastError() }), - info: None, - } + let error = unsafe { GetLastError() }; + Self::from_hresult(HRESULT::from_win32(error)) } #[cfg(not(windows))] { @@ -67,94 +129,46 @@ impl Error { /// The error code describing the error. pub const fn code(&self) -> HRESULT { - self.code + 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 { - #[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.info.message() { + return message; } // Otherwise fallback to a generic error code description. - self.code.message() + self.code().message() } /// The error object describing the error. #[cfg(windows)] - pub fn as_ptr(&self) -> *mut c_void { - self.info - .as_ref() - .map_or(core::ptr::null_mut(), |info| info.as_raw()) + pub fn as_ptr(&self) -> *mut core::ffi::c_void { + self.info.as_ptr() } } #[cfg(feature = "std")] impl std::error::Error for Error {} -unsafe impl Send for Error {} -unsafe impl Sync for Error {} 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 + let code = error.code(); + error.info.into_thread(); + code } } impl From for Error { fn from(code: HRESULT) -> Self { Self { - code, - #[cfg(windows)] - info: { - let mut info = None; - unsafe { GetErrorInfo(0, &mut info as *mut _ as _) }; - info - }, + code: nonzero_hresult(code), + info: ErrorInfo::from_thread(), } } } @@ -162,7 +176,7 @@ impl From for Error { #[cfg(feature = "std")] impl From for std::io::Error { fn from(from: Error) -> Self { - Self::from_raw_os_error(from.code.0) + Self::from_raw_os_error(from.code().0) } } @@ -178,31 +192,19 @@ impl From for Error { impl From for Error { 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 { 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 { 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)) } } @@ -210,7 +212,7 @@ 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() } @@ -226,3 +228,178 @@ impl core::fmt::Display for Error { } } } + +impl core::hash::Hash for Error { + fn hash(&self, state: &mut H) { + self.code.hash(state); + // We do not hash the error info. + } +} + +// Equality tests only the HRESULT, not the error info (if any). +impl PartialEq for Error { + fn eq(&self, other: &Self) -> bool { + self.code == other.code + } +} + +impl Eq for Error {} + +impl PartialOrd for Error { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Error { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + self.code.cmp(&other.code) + } +} + +static_assertions::assert_impl_all!(Error: Send, Sync); + +use error_info::*; + +#[cfg(all(windows, not(windows_slim_errors)))] +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()); + } + } + } + + 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, not(windows_slim_errors))))] +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 EmptyErrorInfo; + + pub(crate) use EmptyErrorInfo as ErrorInfo; + + impl EmptyErrorInfo { + 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 48304abc80..68587ecf6c 100644 --- a/crates/libs/result/src/hresult.rs +++ b/crates/libs/result/src/hresult.rs @@ -2,7 +2,7 @@ use super::*; /// 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,7 +63,7 @@ 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(); @@ -109,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); } } diff --git a/crates/libs/result/src/lib.rs b/crates/libs/result/src/lib.rs index 8eb6ddbeeb..3207acfdc4 100644 --- a/crates/libs/result/src/lib.rs +++ b/crates/libs/result/src/lib.rs @@ -6,32 +6,36 @@ 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/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"]