Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Make syn, quote and proc-macro2 optional when implementing COM interfaces. #2289

Closed
hasenbanck opened this issue Jan 17, 2023 · 11 comments
Labels
question Further information is requested

Comments

@hasenbanck
Copy link

Motivation

While porting the DirectStorage API to Rust we only used the interface proc macro as a starting point and the customized the expanded code to have more control of the generated API and provide a better user experience of the crate. This works in general fine, but the IUnknown_Vtbl:::new() function is behind the interface or implement feature, which pulls in some depdencies that the end user might now want to include (since they are not used by us).

It would be good to somehow enable external crates to implement COM objects without the need to pull in the proc macro dependencies.

Drawbacks

No response

Rationale and alternatives

No response

Additional context

No response

@hasenbanck hasenbanck added the enhancement New feature or request label Jan 17, 2023
@kennykerr
Copy link
Collaborator

Yes, this is something I'd love to fix.

@hasenbanck
Copy link
Author

Thanks for the reply. A short ping once something is available for testing would be great! It's not time critical in any way.

@kennykerr
Copy link
Collaborator

we only used the interface proc macro as a starting point and the customized the expanded code to have more control of the generated API and provide a better user experience of the crate.

I'm working on the eventual replacement for the interface macro and it would be helpful to know what's lacking that you chose to roll your own experience. Can you expand?

@kennykerr kennykerr added question Further information is requested and removed enhancement New feature or request labels Mar 6, 2023
@hasenbanck
Copy link
Author

Using the interface macro had the following problems for us:

  1. It didn't work well with some IDEs like the InteliJ Rust plugin in our experience (traits and structures got mixed up).
  2. You couldn't customize the public API signature. This was a problem, since we wanted to provide the same abstraction level as windows_rs itself:
// With the macro we had to expose this API:
pub unsafe fn CreateStatusArray(
    &self,
    capacity: u32,
    name: PCSTR,
    riid: *const GUID,
    ppv: *mut *mut c_void,
) -> HRESULT;

// But we wanted to have this API:
pub unsafe fn CreateStatusArray<T>(&self, capacity: u32, name: PCSTR) -> Result<T>
where
    T: Interface,
{...}

The argument that users might not want to pull in syn, quote and proc-macro2 for better build times still applies.

@kennykerr
Copy link
Collaborator

Thank! That's very helpful.

  1. Did you report that to them? Seems like something they should work on.

  2. That's something that I'm planning to solve with the interface macro's replacement and should also avoid the additional dependencies.

@kennykerr kennykerr added enhancement New feature or request and removed question Further information is requested labels Mar 9, 2023
@kennykerr
Copy link
Collaborator

Just checking in with this older issue. I don't really see a practical solution for removing the syn dependency in particular nor a compelling reason to do so. Feel free to chime in if you disagree, otherwise I'll close this issue.

@kennykerr kennykerr added question Further information is requested and removed enhancement New feature or request labels Nov 8, 2023
@MarijnS95
Copy link
Contributor

MarijnS95 commented Nov 8, 2023

(I'm subscribed to this issue even though I don't recall needing this, but chiming in anyway since it relates to what we found/discussed in #2694 (comment))

This works in general fine, but the IUnknown_Vtbl:::new() function is behind the interface or implement feature, which pulls in some depdencies that the end user might now want to include (since they are not used by us).

Since the split to windows-core this is no longer the case. windows-core still has IUnknown_Vtbl::new() behind a feature flag:

#[cfg(feature = "implement")]
impl IUnknown_Vtbl {
pub const fn new<T: IUnknownImpl, const OFFSET: isize>() -> Self {

But (as per the comment linked above) is no longer also including the proc-macro crates, that only happens in the main windows crate:


implement = ["windows-implement", "windows-interface", "windows-core/implement"]


That should solve your dependency question. It however seems (from the trait IUnknownImpl function signatures in that file) that raw pointers are still used over high-level (possibly ownership-transferring) types.

@hasenbanck
Copy link
Author

Hi!

I haven't been keeping track with the changes that happened between 0.42 and 0.51, so I can't comment about changes on the old requirements right now.

I have also delayed the update of the direct storage crate to wait until the development in the upstream project stablized and it seems now a good point to pull the changes from the current 1.2.1 version.

I just realized that they now provide a "Microsoft.Direct3D.DirectStorage.winmd" metadata file, so most likely I shouldn't write the bindings manually anymore and should automatically generate them now.

@MarijnS95
Copy link
Contributor

I just realized that they now provide a "Microsoft.Direct3D.DirectStorage.winmd" metadata file, so most likely I shouldn't write the bindings manually anymore and should automatically generate them now.

Definitely, that makes total sense with the windows-core crate. In case you need a minimal example, see:

https://github.com/Traverse-Research/amd-ext-d3d-rs

In short you have to create a bindings.txt that configures how to generate rust code and call the generator on it. riddle isn't yet published so that you can do cargo install riddle; riddle --etc bindings.txt, so there's the api_gen crate that depends on the published windows-bindgen crate via windows_bindgen::bindgen(["--etc", "bindings.txt"]) instead.

Hope that helps!

@MarijnS95
Copy link
Contributor

MarijnS95 commented Nov 9, 2023

I have also delayed the update of the direct storage crate

On a somewhat related note, there appear to be 3 of these crates, 2 of which are from @damyanp / @kennykerr and have been yanked, but crates.io shows those two at the top 😅

@kennykerr
Copy link
Collaborator

The folks working on those APIs are using windows-rs to generate bindings so that is a good bet.

Anyway, I'll close this issue. If there's anything else you need help with, please create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants