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

For COM interfaces T and U, if T is derived from U, implement From<T> for U #2764

Closed
zopsicle opened this issue Jan 1, 2024 · 5 comments · Fixed by #2779
Closed

For COM interfaces T and U, if T is derived from U, implement From<T> for U #2764

zopsicle opened this issue Jan 1, 2024 · 5 comments · Fixed by #2779
Labels
enhancement New feature or request

Comments

@zopsicle
Copy link

zopsicle commented Jan 1, 2024

Suggestion

The interface_hierarchy macro and the CanInto trait enable upcasting of COM interfaces, but this only appears to be used for arguments to Windows API functions (through the undocumented IntoParam trait).

It would be useful to additionally support upcasting of COM interfaces directly. This is always safe and infallible, so ComInterface::cast is not suitable. Example code that would compile with this change:

use windows::Win32::Graphics::Dxgi::{IDXGISwapChain, IDXGISwapChain1};

fn f(a: IDXGISwapChain1) -> IDXGISwapChain
{
    a.into()
}

Note that this would be a zero-cost conversion; no clone or call to QueryInterface necessary.

I think the implementation would be as simple as having interface_hierarchy generate transmuting From impls: 9057d9e but maybe there is more to it that I am not aware of.

@zopsicle zopsicle added the enhancement New feature or request label Jan 1, 2024
@tim-weis
Copy link
Contributor

tim-weis commented Jan 1, 2024

maybe there is more to it that I am not aware of

There is, indeed. In COM Theory™, reference counts are strictly per interface. When transmuting from one (owning) representation into a different (owning) representation, IUnknown's impl Drop is inevitably going to cause havoc.

In practice, this frequently pans out, even if wrong. While I haven't made up my mind whether 2024 is going to be the year when I stop giving a damn, I'll proactively suggest not introducing this.

This isn't to say that ergonomics for upcasting COM interfaces couldn't be improved. It's just that—this time—the simple solution won't do. I'm confident some sharp minds will review this and procure a workable solution.

Happy New Year, regardless!

@zopsicle
Copy link
Author

zopsicle commented Jan 1, 2024

The move constructor for Microsoft’s own ComPtr type (i.e. owning COM interfaces) in C++ seems to suggest otherwise. It implements an upcast simply by copying the pointer: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.14393.0/winrt/wrl/client.h#L272

@tim-weis
Copy link
Contributor

tim-weis commented Jan 1, 2024

Yes, that's the same bug. You'd need to instantiate a COM object that implements a tear-off interface to exercise the error mode.

Raymond Chen published several blog posts on this topic:

Apparently, tear-offs are a real thing.

@kennykerr
Copy link
Collaborator

The issue with tearoffs/aggregates doesn't apply here since the IUnknown behavior isn't changed as the same vtable is in play - you just end up with a subset of the vtable.

The main reason I'd be reluctant to add the suggestion is that it would add a ton of (expanded) code to the windows crate that could impact build time. There may be a way to do it with a default From implementation for CanInto implementations.

@kennykerr
Copy link
Collaborator

This should do the trick: #2779

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.

3 participants