Bug: _Impl trait should use `unsafe fn` if any of its parameters are pointers

[saschanaz]

Which crate is this about?

windows

Crate version

0.32.0

Summary

#[implement(Windows::Win32::Graphics::Imaging::IWICBitmapFrameDecode)]
pub struct Foo {}

impl IWICBitmapSource_Impl for Foo {
    fn GetSize(&mut self, puiwidth: *mut u32, puiheight: *mut u32) -> windows::core::Result<()> {
        unsafe {
            *puiwidth = 0u32;
            *puiheight = 0u32;
        }
        Ok(())
    }
    /* ... */
}

Expected behavior

It should be an unsafe fn as the implementation is expected to dereference the pointer parameters.

Actual behavior

Everything is just a safe fn.

Additional comments

cargo clippy is unsatisfied:

error: this public function might dereference a raw pointer but is not marked `unsafe`
   --> src\lib.rs:149:14
    |
149 |             *puiwidth = self.decoded.borrow().basic_info.xsize;
    |              ^^^^^^^^
    |
    = note: `#[deny(clippy::not_unsafe_ptr_arg_deref)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref

[kennykerr]

Good feedback - thanks! Will add detection for this and use unsafe fn accordingly.

[saschanaz]

BTW, can this specific function accept a tuple as a return type instead?

[kennykerr]

BTW, can this specific function accept a tuple as a return type instead?

I'm not sure what you mean. Do you have an example?

[saschanaz]

I mean, IWICBitmapSource_Impl::GetSize has two [out] parameters, and windows-rs can convert out parameters into a return type, so why not a tuple here? 👀

[kennykerr]

The challenge is figuring out whether it is appropriate just by looking at the metadata.

[kennykerr]

Regarding the original issue/question, I think I need to finally look a little closer at how we delineate what is safe and unsafe in the Windows API. Right now, all WinRT APIs are safe and the rest are not. That is somewhat arbitrary, and I'd like to do something perhaps more in line with clippy's decision making here where it depends on the signature of a particular function.

[Plecra]

Could this be implemented as an attribute within the win32metadata? "This function is sound to call with any arguments/environment, requiring only the parameters satisfy the listed types"

[kennykerr]

You could ask them. I doubt it would be simple to figure out though, given the large scope of APIs. I also think .NET has its own interpretation of safety here so it may be language specific. .NET seems to define a method as unsafe if any of its parameters include a pointer but doesn't account for value types that contain pointers. 🤷‍♂️