register_com_server: Fix incorrect CoRegisterClassObject rclsid argument

[Fredrik Orderud (forderud)]

The implementation is currently calling CoRegisterClassObject with the default interface ID GUID as rclsid argument. It should instead be passing the COM class ID GUID (ClassID) as rclsid argument.

Also, update affected test code to stay in sync.

[Jon Wiswall (jonwis)]

Neat! Is there some improvement to winrt::guid_of<> to make it work with these types? https://github.com/microsoft/cppwinrt/blob/4c9e03fe1e3d7cbfbf742ec237e9af5b30add68a/strings/base_meta.h#L135-L140 is where it's implemented.

[Fredrik Orderud (forderud)]

Neat! Is there some improvement to winrt::guid_of<> to make it work with these types? https://github.com/microsoft/cppwinrt/blob/4c9e03fe1e3d7cbfbf742ec237e9af5b30add68a/strings/base_meta.h#L135-L140 is where it's implemented.

winrt::guid_of<> is unfortunately poorly documented, but it seems to return the IID of the first interface passed as template argument to the winrt::implements<..> base-class. This is different from __uuidof(T) which will give you the UUID of the class itself. My impression is therefore that winrt::guid_of<> is correct, but just not the right function to call if one wants the Class ID.

[Charles Milette (sylveon)]

The docs say:

A helper function template that retrieves the GUID of a runtime class, coclass, or interface.

So it does appear that the C++/WinRT function is broken! It should return the class ID in this case. I think what trips it up is that C++/WinRT doesn't normally expect you to implement COM classes with winrt::implements, so it assumes that the presence of winrt::implements means you're implementing a WinRT runtime object, in which case it returns the default interface for that object.

I think a fix could be to instead try __uuidof first (via SFINAE?) and then fallback to the default interface if that fails. But unfortunately any fix this might lead to subtle runtime changes (as it changes the return value of guid_of without compilation break), so it's a bit risky.

[Charles Milette (sylveon)]

Either way, we can definitely merge this fix in IMO, because this is for COM servers, not WinRT servers (as shown by it not calling RoRegisterActivationFactories), so one should expect COM classes to have __declspec(uuid) and straight __uuidof should always be correct.

[Charles Milette (sylveon)]

Kenny Kerr (@kennykerr) probably knows best w.r.t the guid_of and implements machinery. Should definitely file an upstream issue at the very least.

[Fredrik Orderud (forderud)]

Kenny Kerr (@kennykerr) probably knows best w.r.t the guid_of and implements machinery. Should definitely file an upstream issue at the very least.

I just created microsoft/cppwinrt#1494 to request either a winrt::guid_of<> documentation update or behavior change.

[Kenny Kerr (kennykerr)]

Sorry for the delay. I commented on this here: microsoft/cppwinrt#1494 (comment)

[roxk]

Either way, we can definitely merge this fix in IMO, because this is for COM servers, not WinRT servers (as shown by it not calling RoRegisterActivationFactories), so one should expect COM classes to have __declspec(uuid) and straight __uuidof should always be correct.

While these helpers are indeed for COM server, the classes provided by a COM server doesn't necessarily have to be COM classes. Implementing a COM server with WinRT types is valid and should be supported, so I'd object to merging this PR.

As a person who pushed for using WinRT for everything in windows, only to get disappointed by the fact that widget API needs to support windows 10 so it cannot use WinRT server due to MSIX limitation...These helpers represent my last ditch effort to reduce friction of implementing everything in WinRT. Merging this PR defeats the purpose of these helpers. I want developers using wil use more WinRT, not more COM.

COM support is welcome, but WinRT support is a must. It's the baseline of my contribution.

TLDR: IMO the correct fix is to use __uuidof for COM class, and then winrt::guid_of for the rest. We can do this in wil instead of cppwinrt to avoid affecting everyone (only developers using register_com_servers are affected).

[roxk]

Please keep WinRT support when adding COM feature, thanks!

[roxk]

Please do not remove support for winrt::guid_of, thanks!

[Fredrik Orderud (forderud)]

Please do not remove support for winrt::guid_of, thanks!

The WIL register_com_server function in question registers COM servers based on CLSID. I do then believe that __uuidof(T) would be more appropriate than winrt::guid_of<T>() for getting the CLSID for the COM class.

Do you have a concrete example of a COM class where it would be more appropriate to instead use winrt::guid_of<T>() for retrieving the CLSID, so that I can better understand your comment?

[roxk]

These are meant to be winrt type types so they wouldn't explicitly define UUID. Please revert this and add MyComClass for testing support for COM classes instead, thanks!

[Charles Milette (sylveon)]

If this is removed, winrt::guid_of returns the UUID of IStringable, which is definitely not correct behavior for this purpose. CoCreateInstance(__uuidof(ISomeInterface)) should not be valid.

[Charles Milette (sylveon)]

These are meant to be winrt type types so they wouldn't explicitly define UUID.

However, they are being registered to a COM server so they need a CLSID. The fact these classes implement WinRT interfaces does not detract from this.

[Charles Milette (sylveon)]

Implementing a COM server with WinRT types is valid and should be supported, so I'd object to merging this PR.

However, you have to abide by COM rules and have a CLSID. The current implementation does not do that.

winrt::guid_of is only valid here when you have a full class defined in metadata with an exclusiveto default_interface. And if you have that, why the heck are you not implementing a WinRT server.

[Duncan Horn (dunhor)]

Going through this I can see valid arguments for both sides. First, some comments...

C++/WinRT doesn't normally expect you to implement COM classes with winrt::implements, so it assumes that the presence of winrt::implements means you're implementing a WinRT runtime object

Using C++/WinRT for non-WinRT types is quite common, particularly within the Windows codebase itself. Chris Guzak (@ChrisGuzak) has been a major proponent of this usage. That said, it's not incredibly common to declspec(uuid()) on a class implementation, so the C++/WinRT behavior here seems somewhat reasonable, though IMO this behavior should be limited to just class implementations with a default exclusiveto interface. Regardless, the ship has sailed here and I will assume the design to be immutable.

Implementing a COM server with WinRT types is valid and should be supported

This change doesn't affect that. Really all it's doing is making it more explicit about which GUID to use for the CLSID. You can __declspec(uuid()) your class definitions and it will still work. This is arguably better for the reasons Charles Milette (@sylveon) already pointed out in the review since CLSIDs and UUIDs are two distinct things. One could argue that the UUID for an exclusiveto interface might be fine to use as a CLSID since it should be unique to the implementation, at least in the same ways CLSIDs are, however AFAICT there's no way to enforce this, e.g. with some static_assert(winrt::is_exclusiveto<winrt::default_interface<T>>).

this is for COM servers, not WinRT servers

winrt::guid_of is only valid here when you have a full class defined in metadata with an exclusiveto default_interface. And if you have that, why the heck are you not implementing a WinRT server.

There are valid reasons you would want to register a WinRT class as a COM server since many features of Windows use COM types as currencies. Additionally, I think you mean "useful" not "valid" since the (original) tests pretty clearly demonstrate that it compiles with IStringable as the first interface (regardless of how bad of an idea that might be).

I think a fix could be to instead try __uuidof first (via SFINAE?)

This is my suggestion. I wrote the following code some years ago; perhaps it's time for a revival:

template <typename T>
structhas_iid
{
template <typename U = T, std::enable_if_t<std::is_same_v<GUID, std::decay_t<decltype(__uuidof(U))>>, int> = 0>
static std::true_type invoke(int);
template <typename U = T>
static std::false_type invoke(float);
staticconstexprbool value = decltype(invoke(0))::value;
};
template <typename T>
constexprbool has_iid_v = has_iid<T>::value;

You could then if constexpr using this type trait to decide whether to use __uuidof() or winrt::guid_of<>. This is still not the greatest since it's still possible to register your type with a non-exclusiveto interface as the CLSID, but at that point you are willingly shooting yourself in the foot.

[Charles Milette (sylveon)]

Using C++/WinRT for non-WinRT types is quite common, particularly within the Windows codebase itself.

Absolutely, I do so myself. But parts of the supporting infra, like guid_of, was obviously designed for WinRT usage only in mind, and the ship has sailed for amending that because it would be a runtime behavior change. It would be interesting to make it a compiler error in cases where it does unexpected things (like here), however.

There are valid reasons you would want to register a WinRT class as a COM server since many features of Windows use COM types as currencies.

I agree there are valid cases. But those cases aren't usually the ones using full WinMD files with metadata and exclusiveto interfaces (the cases where guid_of is useful here).

This is my suggestion. I wrote the following code some years ago; perhaps it's time for a revival:
You could then if constexpr using this type trait to decide whether to use __uuidof() or winrt::guid_of<>. This is still not the greatest since it's still possible to register your type with a non-exclusiveto interface as the CLSID, but at that point you are willingly shooting yourself in the foot.

Unfortunately I don't think it's the best decision: I don't think that accidentally forgetting __declspec(uuid(...)) should be considered shooting yourself in the foot. Footgunning should be done explicitly, not by omission.

There is indeed no way to detect if a specific interface is exclusiveto, as that information isn't preserved in C++ source. C++26 will add custom attributes and the ability to reflect on them, which could interesting for C++/WinRT to expose here.

I think the most reasonable thing is to directly use __uuidof, as the PR does. A custom CLSID is pretty much always the thing you want there. Forgetting to include it will give a clear compiler error telling you to add one, and if you really want to shoot yourself in the foot by making your CLSID the IID of some interface, you can just copy it into the __declspec, in which case the footgunning is made as an explicit choice.

[Duncan Horn (dunhor)]

But those cases aren't usually the ones using full WinMD files with metadata and exclusiveto interfaces

This isn't as uncommon as you might think, at least with OS code. That said, I'm not aware of any instances that use CoRegisterClassObject, so that scenario might still be rare and searching for instances is a little tricky. In any case, it's best not to assume nobody will do it.

C++26 will add custom attributes and the ability to reflect on them

Unless the C++ committee is moving heaven and earth even more so than they already are to get the base reflection in the working draft by the C++26 deadline, C++26 won't have reflection for attributes. But that shouldn't be necessary since the code gen can already include that info into the type system in a way that is template metaprogramming friendly.

I think the most reasonable thing is to directly use __uuidof, as the PR does. A custom CLSID is pretty much always the thing you want there

For authoring, I'll agree with you, however consumption then becomes a little trickier. You'd need to have a CLSID defined in a shared location & ensure that its definition is identical to the __declspec. Not hard to do by any means, but is a source of possible errors. With winrt::guid_of, at least that info is baked into the type system (when used correctly). Personally, I'd prefer something more like using T::clsid, so that you can just declare a static constexpr GUID clsid = CLSID_MyType but I'm not sure how others feel about that. Someone could even do static constexpr GUID clsid = winrt::guid_of<MyType> if they wanted the current behavior.

In any case, I think I'm starting to come around to just accepting this change as-is. roxk shouldn't be blocked since adding a __declspec(uuid()) should be pretty simple (albeit possibly error prone if the default interface is used by consumers and isn't stable). A quick search suggests that there aren't many consumers, so while this would be a breaking change, the fallout is manageable. So unless he has objections with specific examples where this work-around wouldn't be feasible, I'm thinking we just roll with this change. If C++/WinRT gets support for querying whether a default interface is exclusiveto or not, we can revisit the fallback.

[Duncan Horn (dunhor)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Charles Milette (sylveon)]

That said, I'm not aware of any instances that use CoRegisterClassObject, so that scenario might still be rare and searching for instances is a little tricky.

That's what I was thinking specifically. If you have WinMD/IDL files, usually you'd want a WinRT server instead of a COM server, to get the most benefit out of the hassle of getting that setup.

Unless the C++ committee is moving heaven and earth even more so than they already are to get the base reflection in the working draft by the C++26 deadline, C++26 won't have reflection for attributes. But that shouldn't be necessary since the code gen can already include that info into the type system in a way that is template metaprogramming friendly.

It is in the working draft now! It got merged last month. Static reflection is actually coming :)

Anyways, I talked with roxk a bit offline and for them the __declspec(uuid(...)) is a no-sell (they see it as introducing COM-isms in what would otherwise be, from their side, pure WinRT code). I think the best solution to keep everyone happy would be defer the best way to pick the CLSID to the user by simply switching to taking it as a parameter (would have to remove the fold expression and do registration one-by-one). It's the most flexible solution as well, for example in some cases where your CLSID might not be a constant for whatever reason.

As you've mentioned, the number of consumers is low (and docs don't exist yet) so I think we can get away with an API break here (it would also be more explicit to the user than a behavior change).

[Fredrik Orderud (forderud)]

I just updated the PR to hopefully resolve the "missing clang format" CI error.

[Duncan Horn (dunhor)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Duncan Horn (dunhor)]

It is in the working draft now! It got merged last month. Static reflection is actually coming :)

Oh, interesting. Attribute reflection really sounded like a future thing when first introduced; glad to see it expedited. Still, a template metaprogramming solution is probably more preferable in the near term should someone wish to pursue that route.

I think the best solution to keep everyone happy would be defer the best way to pick the CLSID to the user by simply switching to taking it as a parameter (would have to remove the fold expression and do registration one-by-one)

That sounds like a reasonable alternative to me. Just make sure to use something like std::array so that we can static_assert that sizeof... the template arguments and the size of the array match. roxk can still get what he wants with a simple wrapper, something like:

template <typename... Types>
constexprautomake_clsid_array()
{
return std::array<GUID, sizeof...(Types)>{ winrt::guid_of<Types>()... };
}

[Fredrik Orderud (forderud)]

Closing PR, since it appear to have been replaced by #537 that is now merged.
Thanks for helping out on this Charles Milette (@sylveon) 👍