Skip to content

Conversation

@oldnewthing
Copy link
Member

@oldnewthing oldnewthing commented Feb 21, 2022

Turns out that IWeakReference does work for classic COM: Even though the output pointer of IWeakReference::Resolve is typed as IInspectable**, it is annotated as iid_is(riid), so COM ignores the formal type and relies on the IID, which is not constrained to derive from IInspectable. Verified that the COM marshaling metadata for the Resolve method is unaffected by the formal type of the output pointer. In particular, querying the resulting object for IInspectable will send a request over the wire (if never requested before), proving the COM did not infer IInspectable support.

This is a behavior change: Previously, pure classic COM objects did not support weak references. However, it removes developer surprise, since they see a method called get_weak() and expect it to work.

Also improved the diagnostic in the case where you call get_weak() from a class that does not support weak references. The old error message was

This is only for weak ref support.

This message appears to be some sort of internal error and creates developer confusion.

Updated the error so that you get

Weak references are not supported because no_weak_ref was specified.

Used if constexpr in make_weak_ref to generate the assertion only once per violation. The previous code generated the assertion but tried to compile the code anyway, resulting in confusing cascade errors which obscured the root cause.

Right now, if you call `get_weak()` from a class that
does not support weak references, you get the message

> This is only for weak ref support.

This message appears to be some sort of internal
error and creates developer confusion.

Updated the error so that you get

> Weak references are not supported by this class.

and one (or both) of

* Weak references are not supported because object does not implement IInspectable.
* Weak references are not supported because no_weak_ref was specified.
@oldnewthing oldnewthing changed the title Improve diagnostics when `get_weak() is not supported Improve diagnostics when get_weak() is not supported Feb 21, 2022
@kennykerr
Copy link
Collaborator

Thanks I'll take a look soon. I just wanted to point out that weak references don't actually require IInspectable. That's something I came to realize after implementing this in cppwinrt. So if it simplifies things we could simply remove that constraint.

@oldnewthing
Copy link
Member Author

Yeah, it's weird because IWeakReference::Resolve takes a REFIID which is great, but returns an IInspectable* which implies that the IID must derive from IInspectable (for type safety). C++ callers probably won't care, but this may cause problems for other languages.

@kennykerr
Copy link
Collaborator

I spoke to the author of the header, and it sounds like it was just an honest mistake (which will confuse C++ developers forever). Either way, both caller and callee must agree on the interface being requested and in practice this is implemented using QueryInterface without checking whether the interface being requested actually derives from IInspectable. Since cppwinrt, I have implemented weak references in other languages without this restriction.

@oldnewthing
Copy link
Member Author

The parameter is annotated as [out, retval, iid_is(riid)] IInspectable **. I hope the iid_is(riid) overrides the IInspectable** in the marshaler, so that the result isn't cached in the local proxy as an IInspectable as well as the provided interface. I'll run an experiment to verify.

@sylveon
Copy link
Contributor

sylveon commented Feb 21, 2022

If IInspectable is not needed, is it too late to fix the header?

@oldnewthing
Copy link
Member Author

oldnewthing commented Feb 21, 2022

Confirmed that COM does not pay attention to the return type IInspectable. If you later QI for IInspectable, it goes back out over the wire.

Bonus confirmed that the proxy data is identical to the version where the second parameter is annotated as [out, retval, iid_is(riid)] void** which is what it should have been in the first place.

@kennykerr
Copy link
Collaborator

If IInspectable is not needed, is it too late to fix the header?

Fixing the SDK has huge compat implications, so we generally don't do that. Besides, most projects should be using a language projection which would hide such details.

Turns out that IWeakReference does work for classic COM:
Even though the output pointer of `IWeakReference::Resolve`
is typed as `IInspectable**`, it is annotated as `iid_is(riid)`,
so COM ignores the formal type and relies on the IID, which
is not constrained to derive from IInspectable. Verified that
the COM marshaling metadata for the Resolve method is unaffected
by the formal type of the output pointer. In particular,
querying the resulting object for `IInspectable` will send a request
over the wire (if never requested before), proving the COM did not
infer `IInspectable` support.

This is a behavior change: Previously, pure classic COM objects
did not support weak references. However, it removes developer
surprise, since they see a method called `get_weak()` and
expect it to work.
@oldnewthing oldnewthing changed the title Improve diagnostics when get_weak() is not supported Extend weak reference support to classic COM Feb 22, 2022
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kennykerr kennykerr merged commit 0c8f4e1 into microsoft:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants