From f72fd9e0e2282c9fe6908cdd76ff57721576ff7c Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Wed, 15 May 2024 15:00:38 -0700 Subject: [PATCH] PR feedback --- crates/libs/core/src/com_object.rs | 85 ++++++++++--------- crates/libs/core/src/imp/mod.rs | 1 - crates/libs/implement/src/lib.rs | 6 +- crates/tests/implement_core/src/com_object.rs | 40 +++++++-- 4 files changed, 84 insertions(+), 48 deletions(-) diff --git a/crates/libs/core/src/com_object.rs b/crates/libs/core/src/com_object.rs index e6c878c37e..e924e02c55 100644 --- a/crates/libs/core/src/com_object.rs +++ b/crates/libs/core/src/com_object.rs @@ -1,8 +1,9 @@ use crate::{AsImpl, IUnknown, IUnknownImpl, Interface, InterfaceRef}; +use core::borrow::Borrow; +use core::ops::Deref; use core::ptr::NonNull; -use std::borrow::Borrow; -/// Identifies types that can be placed in `ComObject`. +/// Identifies types that can be placed in [`ComObject`]. /// /// This trait links types that can be placed in `ComObject` with the types generated by the /// `#[implement]` macro. The `#[implement]` macro generates implementations of this trait. @@ -11,20 +12,24 @@ use std::borrow::Borrow; /// /// This trait is an implementation detail of the Windows crates. /// User code should not deal directly with this trait. -pub trait ComImpl { +/// +/// This trait is sort of the reverse of [`IUnknownImpl`]. This trait allows user code to use +/// `ComObject] instead of `ComObject`. +pub trait ComObjectInner { /// The generated `_Impl` type (aka the "boxed" type or "outer" type). type Outer: IUnknownImpl; } -/// Describes the COM interfaces that a specific ComObject implements. -/// This trait is implemented by ComObject implementation object (e.g. `MyApp_Impl`). +/// Describes the COM interfaces implemented by a specific COM object. /// -/// The `#[implement]` macro generates implementations of this trait. +/// The `#[implement]` macro generates implementations of this trait. Implementations are attached +/// to the "outer" types generated by `#[implemented]`, e.g. the `MyApp_Impl` type. Each +/// implementation knows how to locate the interface-specific field within `MyApp_Impl`. /// /// This trait is an implementation detail of the Windows crates. /// User code should not deal directly with this trait. pub trait ComObjectInterface { - /// Gets a borrowed interface on the ComObject. + /// Gets a borrowed interface that is implemented by `T`. fn as_interface_ref(&self) -> InterfaceRef<'_, I>; } @@ -39,15 +44,17 @@ pub trait ComObjectInterface { /// /// # Safety /// -/// The contained `ptr` field is an owned, reference-counted pointer to a _pinned_ Pin>. -/// Although this code does not currently use `Pin`, +/// The contained `ptr` field is an owned, reference-counted pointer to a _pinned_ `Pin>`. +/// Although this code does not currently use `Pin`, it takes care not to expose any unsafe semantics +/// to safe code. However, code that calls unsafe functions on [`ComObject`] must, like all unsafe code, +/// understand and preserve invariants. #[repr(transparent)] -pub struct ComObject { +pub struct ComObject { ptr: NonNull, } -impl ComObject { - /// Allocates a heap cell (box) and moves `obj` into it. Returns a counted pointer to `obj`. +impl ComObject { + /// Allocates a heap cell (box) and moves `value` into it. Returns a counted pointer to `value`. pub fn new(value: T) -> Self { unsafe { let box_ = T::Outer::new_box(value); @@ -57,8 +64,8 @@ impl ComObject { /// Gets a reference to the shared object stored in the box. /// - /// `ComObject` also implements `Deref`, so you can often deref directly into the object. - /// For those situations where using the `Deref` impl is inconvenient, you can use + /// [`ComObject`] also implements [`Deref`], so you can often deref directly into the object. + /// For those situations where using the [`Deref`] impl is inconvenient, you can use /// this method to explicitly get a reference to the contents. #[inline(always)] pub fn get(&self) -> &T { @@ -72,9 +79,9 @@ impl ComObject { } // Note that we _do not_ provide a way to get a mutable reference to the outer box. - // It's ok to return &mut T, but not &mut T::Outer. That would allow someone to replace the + // It's ok to return `&mut T`, but not `&mut T::Outer`. That would allow someone to replace the // contents of the entire object (box and reference count), which could lead to UB. - // This could maybe be solved by returning Pin<&mut T::Outer>, but that requires some + // This could maybe be solved by returning `Pin<&mut T::Outer>`, but that requires some // additional thinking. /// Gets a mutable reference to the object stored in the box, if the reference count @@ -97,13 +104,13 @@ impl ComObject { self.get_box().is_reference_count_one() } - /// If this object has only a single object reference (i.e. this `ComObject` is the only + /// If this object has only a single object reference (i.e. this [`ComObject`] is the only /// reference to the heap allocation), then this method will extract the inner `T` /// (and return it in an `Ok`) and then free the heap allocation. /// /// If there is more than one reference to this object, then this returns `Err(self)`. #[inline(always)] - pub fn try_take(self) -> Result { + pub fn take(self) -> Result { if self.is_exclusive_reference() { let outer_box: Box = unsafe { core::mem::transmute(self) }; Ok(outer_box.into_inner()) @@ -115,7 +122,7 @@ impl ComObject { /// Casts to a given interface type. /// /// This always performs a `QueryInterface`, even if `T` is known to implement `I`. - /// If you know that `T` implements `I`, then use `as_interface` or `to_interface` because + /// If you know that `T` implements `I`, then use [`Self::as_interface`] or [`Self::to_interface`] because /// those functions do not require a dynamic `QueryInterface` call. #[inline(always)] pub fn cast(&self) -> windows_core::Result @@ -126,10 +133,10 @@ impl ComObject { unknown.cast() } - /// Gets a borrowed reference to an interface that is implemented by this ComObject. + /// Gets a borrowed reference to an interface that is implemented by `T`. /// /// The returned reference does not have an additional reference count. - /// You can AddRef it by calling to_owned(). + /// You can AddRef it by calling [`Self::to_owned`]. #[inline(always)] pub fn as_interface(&self) -> InterfaceRef<'_, I> where @@ -138,7 +145,7 @@ impl ComObject { self.get_box().as_interface_ref() } - /// Gets an owned (counted) reference to an interface that is implemented by this ComObject. + /// Gets an owned (counted) reference to an interface that is implemented by this [`ComObject`]. #[inline(always)] pub fn to_interface(&self) -> I where @@ -147,7 +154,7 @@ impl ComObject { self.as_interface::().to_owned() } - /// Converts this `ComObject` into an interface that it implements. + /// Converts `self` into an interface that it implements. /// /// This does not need to adjust reference counts because `self` is consumed. #[inline(always)] @@ -163,13 +170,13 @@ impl ComObject { } } -impl Default for ComObject { +impl Default for ComObject { fn default() -> Self { Self::new(T::default()) } } -impl Drop for ComObject { +impl Drop for ComObject { fn drop(&mut self) { unsafe { T::Outer::Release(self.ptr.as_ptr()); @@ -177,7 +184,7 @@ impl Drop for ComObject { } } -impl Clone for ComObject { +impl Clone for ComObject { #[inline(always)] fn clone(&self) -> Self { unsafe { @@ -187,7 +194,7 @@ impl Clone for ComObject { } } -impl AsRef for ComObject +impl AsRef for ComObject where IUnknown: From + AsImpl, { @@ -197,7 +204,7 @@ where } } -impl core::ops::Deref for ComObject { +impl Deref for ComObject { type Target = T::Outer; #[inline(always)] @@ -211,14 +218,14 @@ impl core::ops::Deref for ComObject { // access to the contents of the object. Use get_mut() for dynamically-checked // exclusive access. -impl From for ComObject { +impl From for ComObject { fn from(value: T) -> ComObject { ComObject::new(value) } } // Delegate hashing, if implemented. -impl core::hash::Hash for ComObject { +impl core::hash::Hash for ComObject { fn hash(&self, state: &mut H) { self.get().hash(state); } @@ -226,10 +233,10 @@ impl core::hash::Hash for ComObject { // If T is Send (or Sync) then the ComObject is also Send (or Sync). // Since the actual object storage is in the heap, the object is never moved. -unsafe impl Send for ComObject {} -unsafe impl Sync for ComObject {} +unsafe impl Send for ComObject {} +unsafe impl Sync for ComObject {} -impl PartialEq for ComObject { +impl PartialEq for ComObject { fn eq(&self, other: &ComObject) -> bool { let inner_self: &T = self.get(); let other_self: &T = other.get(); @@ -237,9 +244,9 @@ impl PartialEq for ComObject { } } -impl Eq for ComObject {} +impl Eq for ComObject {} -impl PartialOrd for ComObject { +impl PartialOrd for ComObject { fn partial_cmp(&self, other: &Self) -> Option { let inner_self: &T = self.get(); let other_self: &T = other.get(); @@ -247,7 +254,7 @@ impl PartialOrd for ComObject { } } -impl Ord for ComObject { +impl Ord for ComObject { fn cmp(&self, other: &Self) -> core::cmp::Ordering { let inner_self: &T = self.get(); let other_self: &T = other.get(); @@ -255,19 +262,19 @@ impl Ord for ComObject { } } -impl core::fmt::Debug for ComObject { +impl core::fmt::Debug for ComObject { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { ::fmt(self.get(), f) } } -impl core::fmt::Display for ComObject { +impl core::fmt::Display for ComObject { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { ::fmt(self.get(), f) } } -impl Borrow for ComObject { +impl Borrow for ComObject { fn borrow(&self) -> &T { self.get() } diff --git a/crates/libs/core/src/imp/mod.rs b/crates/libs/core/src/imp/mod.rs index 4ee91dce9f..7f7ca49d3e 100644 --- a/crates/libs/core/src/imp/mod.rs +++ b/crates/libs/core/src/imp/mod.rs @@ -10,7 +10,6 @@ mod sha1; mod waiter; mod weak_ref_count; -pub use crate::com_object::{ComImpl, ComObjectInterface}; pub use bindings::*; pub use can_into::*; pub use com_bindings::*; diff --git a/crates/libs/implement/src/lib.rs b/crates/libs/implement/src/lib.rs index a20cd9d9f8..df82baea89 100644 --- a/crates/libs/implement/src/lib.rs +++ b/crates/libs/implement/src/lib.rs @@ -103,7 +103,7 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: } } - impl #generics ::windows_core::imp::ComObjectInterface<#interface_ident> for #impl_ident::#generics where #constraints { + impl #generics ::windows_core::ComObjectInterface<#interface_ident> for #impl_ident::#generics where #constraints { #[inline(always)] fn as_interface_ref(&self) -> ::windows_core::InterfaceRef<'_, #interface_ident> { unsafe { @@ -149,7 +149,7 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: } } - impl #generics ::windows_core::imp::ComImpl for #original_ident::#generics where #constraints { + impl #generics ::windows_core::ComObjectInner for #original_ident::#generics where #constraints { type Outer = #impl_ident::#generics; } @@ -282,7 +282,7 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: } } - impl #generics ::windows_core::imp::ComObjectInterface<::windows_core::IUnknown> for #impl_ident::#generics where #constraints { + impl #generics ::windows_core::ComObjectInterface<::windows_core::IUnknown> for #impl_ident::#generics where #constraints { #[inline(always)] fn as_interface_ref(&self) -> ::windows_core::InterfaceRef<'_, ::windows_core::IUnknown> { unsafe { diff --git a/crates/tests/implement_core/src/com_object.rs b/crates/tests/implement_core/src/com_object.rs index 7a9c4e9d8f..a5d0eb2be9 100644 --- a/crates/tests/implement_core/src/com_object.rs +++ b/crates/tests/implement_core/src/com_object.rs @@ -200,7 +200,7 @@ fn get_mut() { } #[test] -fn try_take() { +fn take() { let app: ComObject = MyApp::new(42); let tombstone = app.tombstone.clone(); // refcount = 1 @@ -208,8 +208,8 @@ fn try_take() { let app2 = app.clone(); // refcount = 2 - let app2_rejected: ComObject = match app2.try_take() { - Ok(_unexpected) => panic!("try_take should have failed"), + let app2_rejected: ComObject = match app2.take() { + Ok(_unexpected) => panic!("take() should have failed"), Err(e) => e, }; // refcount = 1 @@ -217,7 +217,7 @@ fn try_take() { drop(app2_rejected); // refcount = 1 - match app.try_take() { + match app.take() { Ok(unwrapped_app) => { // box destroyed assert!(!tombstone.is_dead()); @@ -227,7 +227,7 @@ fn try_take() { } Err(_unexpected) => { - panic!("try_take should have succeeded"); + panic!("take() should have succeeded"); } } } @@ -332,3 +332,33 @@ fn from_inner_ref() { let ibar: IBar = unsafe { ifoo.get_self_as_bar() }; unsafe { ibar.say_hello() }; } + +// This tests that we can place a type that is not Send in a ComObject. +// Compilation is sufficient to test. +#[implement(IBar)] +struct UnsendableThing { + cell: core::cell::Cell, +} + +impl IBar_Impl for UnsendableThing { + unsafe fn say_hello(&self) { + println!("{}", self.cell.get()); + } +} + +static_assertions::assert_not_impl_all!(UnsendableThing: Send, Sync); +static_assertions::assert_not_impl_all!(ComObject: Send, Sync); + +#[implement(IBar)] +struct SendableThing { + arc: std::sync::Arc, +} + +impl IBar_Impl for SendableThing { + unsafe fn say_hello(&self) { + println!("{}", *self.arc); + } +} + +static_assertions::assert_impl_all!(SendableThing: Send, Sync); +static_assertions::assert_impl_all!(ComObject: Send, Sync);