Remove Drop impl on VARIANT and PROPVARIANT (#3886)

Author: riverar

crates/libs/core/src/handles.rs crates/libs/core/src/resources.rscrates/libs/core/src/handles.rs renamed to crates/libs/core/src/resources.rs

@@ -1,33 +1,33 @@
-/// Custom code to free a handle.
+/// Custom code to free a resource.
 ///
-/// This is similar to the [`Drop`] trait, and may be used to implement [`Drop`], but allows handles
-/// to be dropped depending on context.
+/// This is similar to the [`Drop`] trait, and may be used to implement [`Drop`], but allows resources
+/// to be freed depending on context.
 pub trait Free : Clone {
-    /// Calls the handle's free function.
+    /// Calls the resource's free function.
     ///
     /// # Safety
-    /// The handle must be owned by the caller and safe to free.
+    /// The resource must be owned by the caller and safe to free.
     unsafe fn free(&mut self);
 }
 
-/// A wrapper to provide ownership for handles to automatically drop via the handle's [`Free`] trait.
+/// A wrapper to provide ownership for resources to automatically drop via the resource's [`Free`] trait.
 #[repr(transparent)]
 #[derive(PartialEq, Eq, Default, Debug)]
 pub struct Owned<T: Free>(T);
 
 impl<T: Free> Owned<T> {
-    /// Takes ownership of the handle.
+    /// Takes ownership of the resource.
     ///
     /// # Safety
     ///
-    /// The handle must be owned by the caller and safe to free.
+    /// The resource must be owned by the caller and safe to free.
     pub unsafe fn new(x: T) -> Self {
         Self(x)
     }
 
-    /// Consumes the `Owned<T>` and relinquishes ownership of the handle.
-    /// The caller is now responsible for freeing the returned handle.
-    #[must_use = "losing the handle will leak it"]
+    /// Consumes the `Owned<T>` and relinquishes ownership of the resource.
+    /// The caller is now responsible for freeing the returned resource.
+    #[must_use = "losing the resource will leak it"]
     pub fn into_raw(o: Self) -> T {
         let o = core::mem::ManuallyDrop::new(o);
         o.0.clone()

crates/libs/core/src/windows.rs

@@ -9,8 +9,8 @@ mod event;
 #[cfg(feature = "std")]
 pub use event::*;
 
-mod handles;
-pub use handles::*;
+mod resources;
+pub use resources::*;
 
 pub use windows_strings::*;
 

crates/libs/windows/src/extensions/Win32/System/StructuredStorage.rs

@@ -29,9 +29,9 @@ impl Clone for PROPVARIANT {
     }
 }
 
-impl Drop for PROPVARIANT {
-    fn drop(&mut self) {
-        unsafe { _ = PropVariantClear(self) };
+impl Free for PROPVARIANT {
+    unsafe fn free(&mut self) {
+        _ = PropVariantClear(self);
     }
 }
 

crates/libs/windows/src/extensions/Win32/System/Variant.rs

@@ -30,11 +30,12 @@ impl Clone for VARIANT {
     }
 }
 
-impl Drop for VARIANT {
-    fn drop(&mut self) {
-        unsafe { _ = VariantClear(self) };
+impl Free for VARIANT {
+    unsafe fn free(&mut self) {
+        _ = VariantClear(self);
     }
 }
+
 impl VARIANT {
     pub fn vt(&self) -> VARENUM {
         unsafe { self.Anonymous.Anonymous.vt }

crates/tests/misc/variant/tests/tests.rs

@@ -1,7 +1,8 @@
+use std::rc::Rc;
 use windows::Foundation::Uri;
 use windows::Win32::Foundation::{E_INVALIDARG, TYPE_E_TYPEMISMATCH};
-use windows::Win32::System::Com;
 use windows::Win32::System::Com::StructuredStorage::PROPVARIANT;
+use windows::Win32::System::Com::{self, IAgileObject, IAgileObject_Impl};
 use windows::Win32::System::Variant::*;
 use windows_core::*;
 
@@ -265,3 +266,39 @@ fn test_propvariant() -> Result<()> {
 
     Ok(())
 }
+
+#[implement(IAgileObject)]
+struct Canary {
+    _counter: Rc<()>,
+}
+impl IAgileObject_Impl for Canary_Impl {}
+
+#[test]
+fn test_owned_variant() -> Result<()> {
+    let counter = Rc::new(());
+    let unk: IUnknown = Canary {
+        _counter: counter.clone(),
+    }
+    .into();
+    assert_eq!(Rc::strong_count(&counter), 2);
+    {
+        let _owned = unsafe { Owned::new(VARIANT::from(unk)) };
+    }
+    assert_eq!(Rc::strong_count(&counter), 1);
+    Ok(())
+}
+
+#[test]
+fn test_owned_propvariant() -> Result<()> {
+    let counter = Rc::new(());
+    let unk: IUnknown = Canary {
+        _counter: counter.clone(),
+    }
+    .into();
+    assert_eq!(Rc::strong_count(&counter), 2);
+    {
+        let _owned = unsafe { Owned::new(PROPVARIANT::from(unk)) };
+    }
+    assert_eq!(Rc::strong_count(&counter), 1);
+    Ok(())
+}