Generate QueryInterface-like helper function for callbacks

[MarijnS95]

It'd be great to have helper functions for callbacks with nicer signatures in the windows crate, just like "normal function" wrappers. For starters, when doing this to Query (or QueryOptional) callbacks, code can be written as follows:

let lib = unsafe { Library::new(dxcompiler_lib_name()) }.unwrap();
let create: Symbol<DxcCreateInstanceProc> = unsafe { lib.get(b"DxcCreateInstance\0") }.unwrap();

let compiler: IDxcCompiler2 = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcCompiler) }?;
let library: IDxcLibrary = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcLibrary) }?;

Without this we'd have to replace the last two lines with this atrocity (using transmute for lack of a non-verbose way to put together all the pointer casts):

let mut compiler = None::<IDxcCompiler2>;
let mut library = None::<IDxcLibrary>;

unsafe { (create.unwrap())(&CLSID_DxcCompiler, &IDxcCompiler2::IID, std::mem::transmute(&mut compiler)).ok()? };
unsafe { (create.unwrap())(&CLSID_DxcLibrary, &IDxcLibrary::IID, std::mem::transmute(&mut library)).ok()? };

let compiler = compiler.unwrap();
let library = library.unwrap();

There are currently 517 hits for pub type PFN_ and there must be some that behave like QueryInterface (i.e. D3D10/11/12 CREATE_DEVICE). I'd like to get #747 reopened for this to make it in, but one major blocker was not having any (test) code that actively practices this system.

Reasons to not do this

1. There currently doesn't seem to be any use of this "external symbol loading" for Windows, only when loading an external library like DXC (which can supposedly also be linked from "windows" directly if #[link = "windows"] on DxcCreateInstance is to be believed);
2. Since writing this code/example, windows-rs is now more strict in requiring _COM_Outptr_ SAL before marking functions/callbacks as SignatureKind::Query. This has been worked around for DxcCreateInstance(2) in Add ComOutPtr attribute to additional params win32metadata#890, but not for the *Proc function types (this will be PR'd to DXC soon™).

---

Originally posted by @MarijnS95 in #747 (comment)

[kennykerr]

Here's a quick query looking for callbacks with query-like signatures:

fn main() {
    use windows_metadata::reader::*;

    let files = vec![
        File::new("/git/windows-rs/crates/libs/metadata/default/Windows.Win32.winmd").unwrap(),
    ];

    let reader = &Reader::new(&files);
    let root = reader.tree("Windows.Win32", &[]).unwrap();

    for tree in root.flatten() {
        for def in reader.namespace_types(tree.namespace) {
            if reader.type_def_kind(def) == TypeKind::Delegate
                && !reader.type_def_flags(def).winrt()
            {
                let method = reader.type_def_invoke_method(def);
                let signature = reader.method_def_signature(method, &[]);
                let kind = reader.signature_kind(&signature);

                if matches!(
                    kind,
                    SignatureKind::Query(_) | SignatureKind::QueryOptional(_)
                ) {
                    println!("{}", reader.type_def_name(def));
                }
            }
        }
    }
}

It only returns three results:

PFN_D3D12_CREATE_DEVICE
PFN_D3D12_GET_DEBUG_INTERFACE
PFN_D3D12_GET_INTERFACE

[MarijnS95]

@kennykerr That matches my point 2., I'm concerned that this might not be very useful if there aren't that many functions, or if most of them lack the _COM_Outptr_ annotation.

Adjusting that test by removing && self.param_is_com_out_ptr(param.def) from fn signature_param_is_query_object() to "back out of #1088" shows more function types that may quack like a Query but could simply be lacking _COM_Outptr_ annotations:

DxcCreateInstance2Proc
DxcCreateInstanceProc
PFN_D3D12_CREATE_DEVICE
PFN_D3D12_CREATE_ROOT_SIGNATURE_DESERIALIZER
PFN_D3D12_CREATE_VERSIONED_ROOT_SIGNATURE_DESERIALIZER
PFN_D3D12_GET_DEBUG_INTERFACE
PFN_D3D12_GET_INTERFACE
IWABOBJECT_QueryInterface_METHOD
LPFNGETCLASSOBJECT
DTC_GET_TRANSACTION_MANAGER
DTC_GET_TRANSACTION_MANAGER_EX_A
DTC_GET_TRANSACTION_MANAGER_EX_W
LPFNACCESSIBLEOBJECTFROMWINDOW
LPFNCREATESTDACCESSIBLEOBJECT
LPFNOBJECTFROMLRESULT
fpCreateIFELanguageInstanceType

[kennykerr]

That's a pretty short list - we should be able to get those fixed up by the win32 metadata project. The reason I went for the stricter test is that there are some false positives where functions have a trailing GUID*, void** signature but where the void** has nothing to do with COM.

[MarijnS95]

I totally agree with stricter tests - it just seems like we have some work to do fixing up the headers and/or temporary overrides in win32metadata.
EDIT: Won't be surprised if some of these have nothing to do with COM either - we can only find out by checking every single one and adding them to win32metadata?

Do you still think it's worth pursuing this helper then? Are there other kinds of syntactic sugar that may be beneficial for function typedefs?

[kennykerr]

I think callbacks could be a lot better - not just in this one area - but in general could offer a more appealing calling experience if they could offer a thin wrapper around the function pointer. Ideally, I would like them to behave more like regular function wrappers in the windows crate. Much like how WinRT delegates are wrappers around a COM callback.

[MarijnS95]

One thing I found annoying was them being wrapped inside Option. If I remember correctly I couldn't take them out of their libloading::Symbol<TheFnType> owner, leading to unwrap()s on a borrow everywhere.

(ALAS, nothing has changed since we last spoke; I still use Linux and unguarded #[link(name = "windows")] in sys/ prevent compilation of metadata and the snippet above, yet it runs without linker errors after removal as if the -lwindows directive isn't culled properly. You also have to explain how to tune rust-analyzer to not be stuck for minutes on end writing multiple gigabytes to target/, every time I make a change outside the large crates too.)

How do you want to tackle this? I can possibly look into the Dxc/D3D methods and submit overrides to win32metadata, but unsure about the rest. With that done, should we reopen and revive #747?

[kennykerr]

While this would be convenient for experimenting, I don't think there is enough demand for such a capability to warrant including all the extra code gen needed to make it happen.

[MarijnS95]

Also here, wrong close reason. You should close it as "not planned" etc.

[kennykerr]

Ah, I never noticed the close drop-down before - thanks!

[kennykerr]

Also in my defense, I used this drop-down to bulk close the issues tagged as questions. 😁

[MarijnS95]

Ah, I never noticed the close drop-down before - thanks!

It's "new" (±2 months old IIRC). Feel free to reopen and close with the right status.

Also in my defense, I used this drop-down to bulk close the issues tagged as questions.

I see, questions have no place on this repo?

[kennykerr]

I'm happy to try and answer questions but they're not actionable and thus get closed pretty quickly. If there's valuable information in the Q&A then we can create a PR to capture that information in the FAQ.

[MarijnS95]

I should apply that technique to my repos 😈

[MarijnS95]

Hi @kennykerr! This still only applies to the original 3 delegates found in #1835 (comment), but I do have a few custom metadata bindings where these helpers are relevant too. And they'll be even more relevant when (if?) we can get the SAL annotation fixes for DirectXShaderCompiler working: microsoft/DirectXShaderCompiler#4524. Should we re-discuss whether it is worth adding this helper again?

I still have a branch open and it mostly works: https://github.com/MarijnS95/windows-rs/compare/callback-query-interface-2, there's just one incompatibility. Currently parameters for the type alias are generated as follows:

windows-rs/crates/libs/bindgen/src/delegates.rs

Lines 25 to 29 in 2e7f99d

	let params = signature.params.iter().map(|p| {
	let name = gen.param_name(p.def);
	let tokens = gen.type_default_name(&p.ty);
	quote! { #name: #tokens }
	});

Watch what happens to pouter:

pub type PFNMagic = ::core::option::Option<
    unsafe extern "system" fn(
        pouter: ::core::option::Option<::windows_core::IUnknown>,
        // pouter: *mut ::core::ffi::c_void,
        riid: *const ::windows_core::GUID,
        ppvobject: *mut *mut ::core::ffi::c_void,
    ) -> ::windows_core::HRESULT,
>;

The function call arguments generated by win32_args() and win32_params() expect to be compatible with a *mut c_void type instead:

windows-rs/crates/libs/bindgen/src/functions.rs

Lines 198 to 206 in 2e7f99d

	let params = signature.params.iter().map(|p| {
	let name = gen.param_name(p.def);
	let tokens = if p.kind == SignatureParamKind::ValueType {
	gen.type_default_name(&p.ty)
	} else {
	gen.type_abi_name(&p.ty)
	};
	quote! { #name: #tokens }
	});

pub unsafe fn PFNMagic<P0, T>(
    func: &PFNMagic,
    pouter: P0,
) -> ::windows::core::Result<T>
where
    P0: ::windows_core::IntoParam<::windows_core::IUnknown>,
    T: ::windows::core::ComInterface,
{
    let mut result__ = ::std::ptr::null_mut();
    (func.unwrap())(
        pouter.into_param().abi(), // <------------- Boom. This *mut c_void is not compatible with Option<IUnknown>.
        &<T as ::windows_core::ComInterface>::IID,
        &mut result__,
    )
    .from_abi(result__)
}

What is the desired solution for this? Custom argument/input handling to match the target type of a delegate type alias, or adjust the argument type of that delegate type alias? And what if the metadata didn't annotate the pointer with _In_opt_?

---

On a closely related note (perhaps worth creating a new issue for) the IntoParam<IUnknown> argument is rather useless as it cannot be used to pass any COM object to the function. ComInterface would be a better trait as that guarantees (see as_unknown()) it is always castable to IUnknown (though, borrows vs owned moves have to be considered!). We could solve this even more generically by more broadly exposing CanInto (currently behind some #[doc(hidden)]) for "free" upcasts to known COM object interfaces in the hierarchy. I ran into the necessity for such a cast/hack in a few more places, but would have to look up where exactly to give more concrete use-case examples.