microsoft / cppwinrt

C++/WinRT
MIT License
1.64k stars 236 forks source link

Bug: Composing implementation types cannot call as but can call try_as #1363

Closed sylveon closed 11 months ago

sylveon commented 11 months ago

Version

2.0.230706.1

Summary

When an implementation type composes another class, it cannot call as<> to do a checked QueryInterface of the class it composes. It can only do try_as<>, dropping the error.

Reproducible example

struct MyWindow : winrt::Microsoft::UI::Xaml::WindowT<MyWindow>
{
    MyWindow()
    {
        as<IWindowNative>(); // compile error
        try_as<IWindowNative>(); // okay
    }
};

Expected behavior

Both methods should work.

Actual behavior

as<> fails building.

Additional comments

No response

kennykerr commented 11 months ago

I've tagged a few project maintainers. If one of them is available to offer guidance and mentorship for this issue, they will reach out according to the contributing guide to discuss and agree on an approach.

@microsoft/cppwinrt-maintainers

oldnewthing commented 11 months ago

The try_as is really just a hack to fool the projection wrappers. To query the composed object, do m_inner.as<>.

Both as<> and try_as<> are ambiguous with regard to whether you are querying yourself or querying the contained object. I think calling them directly should be avoided.

kennykerr commented 11 months ago

Yep, that's why we originally provided try_as and no more. Personally, I think this change would be more confusing.

oldnewthing commented 11 months ago

In retrospect, maybe the try_as should have been named impl_try_as so that it was more clearly a projection implementation detail, so people wouldn't be tempted to call it.

sylveon commented 11 months ago

Currently, Windows App SDK docs tell people to call this->try_as.

I was trying to follow this example, but using as to assert that the interface is always there (there shouldn't be a reason for IWindowNative to be missing), and got confused why it wasn't compiling.

For me personally, using a field seems much more hacky and tapping into the implementation than calling a try_as or as method. m_inner doesn't seem like a documented thing either, whereas the behavior of try_as and as are documented fairly well. So naturally people are going to go for the latter.

I think either the WASDK docs should be updated to avoid this method, or we should add as for consistency. The current state of affairs makes people confused why one is present but the other is not.

sylveon commented 11 months ago

Perhaps adding and documenting a get_inner() method would also be a solution.

kennykerr commented 11 months ago

@stevewhims who may be able to suggest how to approach docs.

stevewhims commented 11 months ago

Yes, given the great info and advice in this thread, I'll do these two things:

  1. Change the example that @sylveon mentions to use m_inner.as (which I can confirm works fine) instead of this->try_as (which I was using only to get around the build error). And change all other places in the docs where I use this->as or this->try_as to use m_inner instead.
  2. Find somewhere/way to document m_inner.

-Steve

sylveon commented 11 months ago

Sounds like a good solution!

Maybe it would also be possible to find and fix the projection wrappers using that try_as method to remove it entirely to prevent that problem happening again in the future.

stevewhims commented 11 months ago

Here are the changes I've published. Please let me know if we need more.

https://cpubwin.visualstudio.com/win32/_git/winapps-win32-api/pullrequest/20773?_a=files https://github.com/MicrosoftDocs/windows-dev-docs-pr/pull/3800/files https://github.com/MicrosoftDocs/windows-dev-docs-pr/pull/3802/files

If we need an API ref topic (along the lines of https://learn.microsoft.com/uwp/cpp-ref-for-winrt/windows-foundation-iunknown), then I could use some help around which type m_inner is a data member of. Is it root_implements_composing_outer<true>? Do we need API ref content for that?

sylveon commented 11 months ago

These links are all non-working for me, so I can't tell whether the change is good.

m_inner docs would probably go in the docs for winrt::implements.

stevewhims commented 11 months ago

uld probably go in the docs for winrt

Sorry -- yes, those are links to PRs in private repos, so they were FAO the MS folks on the thread. Thanks for the info about winrt::implements! That's the kind of thing I was after. :)

-Steve