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

Add interface macro to define COM interfaces locally #1486

Closed
kennykerr opened this issue Feb 1, 2022 · 8 comments · Fixed by #1540
Closed

Add interface macro to define COM interfaces locally #1486

kennykerr opened this issue Feb 1, 2022 · 8 comments · Fixed by #1540
Labels
enhancement New feature or request

Comments

@kennykerr
Copy link
Collaborator

kennykerr commented Feb 1, 2022

While I'm working on #1093 and #1094 for complete API and component authoring support, some projects need to implement COM interfaces that are not necessarily described by Windows metadata and don't necessarily warrant public API definitions. A project might just want to implement some locally defined COM interface for interop with another private component.

I recently overhauled the windows crate's implement macro to rely on traits rather than metadata (#1345). What that means is that we could conceivably define interfaces directly in Rust, via a new interface macro, and then have the implement macro implement them and never actually touch any metadata from start to finish. At that point the windows crate could do everything the com crate currently does.

  • The implement proc macro can define the necessary types that the implement macro could simply consume and implement (as if it had come from the pre-generated bindings).
  • There should be no material difference between the code that the windows-bindgen produces and the implement macro produces. The implement macro should not care or be able to tell the difference.
  • It can further be limited to COM interfaces and avoid the complexities of describing WinRT interfaces.
  • Support for WinRT would be left to Support metadata generation #1093 and Simplify WinRT and COM class authoring #1094.

Perhaps something like this:

#[interface("0000010c-0000-0000-C000-000000000046")]
trait IPersist : IUnknown {
    fn GetClassID(clsid: *mut GUID) -> HRESULT;
}
@kennykerr
Copy link
Collaborator Author

kennykerr commented Feb 16, 2022

@rylev's PR (#1540) adds the basic interface macro. There are still some outstanding issues and more testing is needed but we can address those in subsequent PRs.

@kennykerr
Copy link
Collaborator Author

Reopening as this issue still needs a lot of work.

@kennykerr kennykerr reopened this Mar 2, 2022
@kennykerr
Copy link
Collaborator Author

A few initial issues with the interface macro that I'm working through:

  • The interface macro only lets you write methods with a single parameter. That's obviously an issue. 😉
  • The interface macro will fail if the implement feature is not enabled - it makes invalid cross-feature dependencies. It should not need the implement feature at first glance, but it may be simpler to combine them...
  • It doesn't preserve the method's return type and blindly assumes HRESULT.
  • It blindly wants everything to be unsafe. Bug: _Impl trait should use unsafe fn if any of its parameters are pointers #1506 (comment)
  • It uses the quote crate rather than the windows-tokens crate used by the implement macro.
  • The proc-macro2 crate dependency is also not necessary.

@kennykerr
Copy link
Collaborator Author

#1595 fixes the return type issue

#1594 fixes the single parameter issue

@rylev
Copy link
Contributor

rylev commented Mar 14, 2022

@kennykerr I can work on fixing those issues. There's only one I'm unsure about:

The reason I opted for quote instead of windows-tokens is that while windows-tokens is much faster when you have to generate a lot of code, quote provides much better error messages when the user does something wrong. In other words quote is directly optimized for the experience that the interface macro provides: direct user input generates a little bit of code.

What's more is that we already bring quote in as a dependency since our usage of syn relies on it. In fact, you can see that the implement crate also brings in quote already:

cargo tree
windows-implement v0.33.0 (proc-macro) (C:\Users\ryanl\Code\windows-rs\crates\libs\implement)
├── syn v1.0.60
│   ├── proc-macro2 v1.0.24
│   │   └── unicode-xid v0.2.1
│   ├── quote v1.0.8 // < HERE IT IS
│   │   └── proc-macro2 v1.0.24 (*)
│   └── unicode-xid v0.2.1
└── windows-tokens v0.33.0 (C:\Users\ryanl\Code\windows-rs\crates\libs\tokens)

So, I think instead we should consider using quote in implement rather than using windows-tokens in interface. What do you think?

I also think we should consider using trybuild to test the diagnostics we're providing to users to ensure a good developer experience.

@kennykerr
Copy link
Collaborator Author

Sounds great - there's also less reason to use windows-tokens now that we have pre-packaged bindings. My main concern is consistency, so I like what you're suggesting. I just wish quote were faster so we didn't need the fork at all. And trybuild tests would be great.

@kennykerr
Copy link
Collaborator Author

kennykerr commented Mar 16, 2022

I also noticed that the interface macro fails to compile if the trait is not public. Seems like it should allow local (private) declarations.

@rylev
Copy link
Contributor

rylev commented Mar 17, 2022

@kennykerr I believe this issue can be closed now that the interface macro works fairly well. For newly discovered issues, it's probably easier to just open up single issues for them. Based on discussion here, I think we should open issues for the following:

  • Make the usage of windows-tokens and quote consistent between implement and interface. Most likely the best thing for us to do here is just use quote in the implement macro.
  • Test UX/dev experience of the proc macros with the trybuild crate.
  • Be smarter about the requirement for unsafe keyword when declaring interface methods

Note: there was a point above about not needing to use proc_macro2 as a dependency of the interface crate. While we could get rid of this as a direct dependency, proc_macro2 is still used by syn which we'll still want to use as a dependency, so we're stuck with it no matter what. Having it as a direct dependency doesn't incur any additional costs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants