microsoft / cppwinrt

C++/WinRT
MIT License
1.67k stars 241 forks source link

C++20 and beyond (cppwinrt v3) #1123

Closed kennykerr closed 2 years ago

kennykerr commented 2 years ago

cppwinrt v2 is based on C++17, which lacks a number of new features that may be of some benefit to the cppwinrt library. I'm starting to experiment with cppwinrt v3 and what it would look like to adopt some of the C++ language and library features introduced in C++20 and C++23. cppwinrt v2 already supports C++20 coroutines but some other features are harder to introduce without more substantial and breaking changes. Here's a starting list of what I'd like to see in v3. Feel free to comment on other features that you think might be beneficial.

C++20

C++23

It should be clear that the theme is make things simpler and not just adding more bells and whistles. If we find that adding support for X makes things more complicated, then we'd be far less likely to adopt it. By themselves, the C++20 features aren't really compelling enough to spin up a v3 of cppwinrt, but C++23's "deducing this" feature could potentially move the needle so I'm thinking of target C++23 as the min version.

sylveon commented 2 years ago

The work in the modules PR I opened a while ago could be used, but I'd need to check if the compiler bugs I hit have been fixed.

However, there is still stuff that could be done to improve modules here. For example a single change in any API rebuilds the entire module which takes a solid 2-3 minutes on a fast machine. We should probably use module fragments (for example, one per top-level namespace) so that a change to APIs in Contoso.* doesn't cause the compiler to rebuild all of Windows.*.

Also, it obviously needs support for producing runtime components, as right now the modules code in cppwinrt only supports consuming.

Another question is if modules should be something that is always on. While I don't oppose that, some people might because it puts quite a migration burden on existing code.

I believe deducing this might help quite a lot with build times as well, so it may be worth making C++23 the min version.

asklar commented 2 years ago

I believe the last straggler compiler bug impacting modules was fixed recently, @Scottj1s will have more data on that. Deducing this will make builder pattern a lot easier to implement/consume.

Scottj1s commented 2 years ago

@sylveon I've been building on your topic branch, working around a few module compilation issues. I've been working with the modules devs and the last blocker has now been addressed: https://developercommunity.visualstudio.com/t/use-of-modules-with-cwinrt-winrtimplname-v-causes/1613181?from=email When that fix is available in a preview, I'll validate and open a PR here.

I also raised the question of using module fragments and that's recommended.

Regarding migration, project templates are often opted into the latest features, whereas existing projects are defaulted off for safety. We can control both with a new project property.

sylveon commented 2 years ago

Great news! I believe exporting namespace std is not needed anymore either, but haven't tried.

jlaanstra commented 2 years ago

Let's make -opt the default.

kennykerr commented 2 years ago

Let's make -opt the default.

Great idea, we'll need to overcome the reasons why -opt is not always appropriate but I think that's doable. I also want to remove as many "options" as possible. It just adds so much cognitive and technical complexity.

sylveon commented 2 years ago

Yeah, we should also make fast ABI the default.

kennykerr commented 2 years ago

Yeah, we should also make fast ABI the default.

Although I originally implemented the fast ABI, I have serious doubts about its viability and thus far we've seen no adoption so I'm not inclined to think that it should be something we turn on by default or even support at all. It is incredibly complicated compared to the theoretical gains it provides to APIs in general.

jonwis commented 2 years ago

Fast ABI

Let's talk about this first - IIRC the blockers were that the main producers in Windows needed to turn on FastABI support, which was missing for a while. Types like XAML and Composition with lots of little interfaces and versions should benefit from this, yes? WinAppSDK sort of resets the XAML & Composition interface structure, but with our potential release cadence we'll be back into the "5 iterations of Button" by 2025.

Even if there's not wide public adoption of FastABI, in-Windows consumption seems like should be useful. The main wins were about binary size and runtime probing for interfaces, yes? I think we'd still want those. Maybe not a p0, but definitely still interesting.

I'd love to hear more about the state of FastABI in general, maybe in another discusison/thread.

Also, is there a timeline for when VS makes available and the OS repo gets support for the features under consideration?

sylveon commented 2 years ago

Yes I pretty much always believed XAML would be perfect candidate for fast ABI and am surprised it hasn't adopted it yet. Windows App SDK should also definitely look into it.

What IDE support is missing, specifically? I can turn on fast ABI from the C++/WinRT properties sheet in VS.

kennykerr commented 2 years ago

the blockers were that the main producers in Windows needed to turn on FastABI support, which was missing for a while

Not for a while. It has never happened.

Types like XAML and Composition with lots of little interfaces and versions should benefit from this, yes?

The fast ABI was designed for Composition, not Xaml. Composition never adopted it and there is no evidence that it would benefit Xaml due to its reliance on open hierarchies and cross-binary aggregation. I suspect that would break some of the assumptions that the fast ABI depends on and at the very least dramatically reduce any benefit.

Even if there's not wide public adoption of FastABI, in-Windows consumption seems like should be useful.

I am not aware of any adoption either inside or outside Windows.

The main wins were about binary size and runtime probing for interfaces, yes?

The chief benefit is a reduction in QueryInterface/Release calls, but only if such calls can be eliminated at compile time. An object size reduction is available but only if you drop backward compatibility. There are no other benefits and a great deal of complexity.

davidmatson commented 2 years ago

Another thing to consider for v3: hresult_error deriving from std::exception A number of things "just work" that way - see #1129 for one example.

davidmatson commented 2 years ago

V3 would also be a good time to revisit the type used for counted, null-terminated strings. std::string_view is not expected to be null-terminated - for example, you can take one std::string_view, and get a substring from it without allocating additional memory. See: https://devblogs.microsoft.com/oldnewthing/20210823-00/?p=105598 https://en.cppreference.com/w/cpp/string/basic_string_view/substr (cppwinrt's current use of std::string_view is basically fighting the standard library here.)

A counted, null-terminated string might be better represented by a winrt::zstring_view, similar to how gsl defines zstring: https://github.com/microsoft/GSL/blob/383723676cd548d615159701ac3d050f8dd1e128/include/gsl/string_span#L70

dmachaj commented 2 years ago

@davidmatson there is already a wil::zstring_view that could probably be used directly. See wil/stl.h.

sylveon commented 2 years ago

There where efforts to add such a null terminated string view type to the standard, but I'm not sure what's the current status on that. However I don't think we should make cppwinrt depend on wil merely for this. If it depends on wil, we should also use this occasion to eliminate a lot of duplicate functionality between both.

davidmatson commented 2 years ago

I don't think cppwinrt should depend on WIL. My understanding is that the two projects reflect some clear differences in design philosophy / aesthetics - for example, WIL overrides cppwinrt's normal error handling to use WIL's exception type, which made different tradeoffs. And that would be a large dependency to take into cppwinrt, and one of the clearest wins for cppwinrt is that it's just standard C++ headers - I think having no big dependencies is important to what makes cppwinrt so appealing.

smaillet-ms commented 2 years ago

I'd like to see a couple things adjusted: 1) Resolve module locking so we don't have issues like https://github.com/microsoft/xlang/issues/649 2) Define C++/WinRT exceptions as derived from std::exception (The non-standard approach used in C++WinRT has caused a lot of problems for integration with other libraries with respect to catch and translate type handlers. (aligns with @davidmatson suggestion so more of a +1 on that)

ChrisGuzak commented 2 years ago

I'd like to see

dmachaj commented 2 years ago

To build on what @ChrisGuzak said there is an integration point between cppwinrt and WIL when logging errors. cppwinrt always passes nulls and zeros for the source information, which is not especially helpful. std::source_location would improve the situation greatly.

cppwinrt side: https://github.com/microsoft/cppwinrt/blob/master/strings/base_error.h#:~:text=if%20(winrt_throw_hresult_handler),%7D

WIL side: https://github.com/microsoft/wil/blob/80e36f4295f4b690cb4646627d8b14a15081af92/include/wil/cppwinrt.h#:~:text=%7D-,inline%20void%20__stdcall%20winrt_throw_hresult(uint32_t%20lineNumber%2C%20char%20const*%20fileName%2C%20char,%7D,-inline%20void%20WilInitialize_CppWinRT

JaiganeshKumaran commented 2 years ago

How will builder style properties affect the implementation side? Won't they be no longer synchronised as the projected signature is not the same as the implementation signature?

sylveon commented 2 years ago

Nobody's stopping you from adding them to your implementation as well.

kennykerr commented 2 years ago

It's not a change to the ABI so the implementation would not be expected to return anything.

sylveon commented 2 years ago

Another thought: it seems that the way forward with regards to dispatching is to use wil::resume_foreground instead of winrt::resume_foreground (the former also being better than the latter). It would make sense for cppwinrt v3 to remove the winrt::resume_foreground helpers (as well as the ability to directly co_await a DispatcherQueue). Maybe marking them [[deprecated]] in an update to cppwinrt v2 would be a good idea.

Another idea is bringing the changes from wil back into cppwinrt and exposing a customization point to consider for third party dispatching solutions and future changes to how WASDK dispatching works - MS and dispatching is like the JS ecosystem and frameworks. It changes every other day, apparently.

kennykerr commented 2 years ago

Agreed, and why I think these UI helpers should just be removed entirely in future.

kennykerr commented 2 years ago

I spent some time digging into C++23's "deducing this" - that looked like the only feature that by itself could really simplify cppwinrt in a meaningful way. Unfortunately, it is not flexible enough to replace CRTP in cppwinrt. I also heard it was dropped from C++23, perhaps because it isn't very useful in its current form.

Feel free to keep the conversation going, but without that there isn't a compelling enough reason to spin up a major new version of cppwinrt so I'll close this issue for now.

asklar commented 2 years ago

Hi @kennykerr, thanks for investigating. Could you clarify what does/doesn't work with deducing this? It's in current VS builds (regardless of standard status), so if it helps address the builder pattern or simplify cppwinrt we should pursue it IMO. Are there specific usages we were hoping to simplify with it that it doesn't? if so which/how?

Also, as a point of process, in general we don't close issues this way in other microsoft org repos; we post a reply, allow for interaction with the issue (from other microsoft teams as well as the community), and once it is clear we won't pursue this issue we would close it as "not planned" (note that the issue is closed as completed, which is a new github feature :) ). Point is, it would "feel nicer" if issues where there is room for back and forth with collaborators, weren't summarily closed :)

There are a few issues duped to this one and several features lumped together into this issue, so I don't think closing this (without reopening the issues that were duped to this one), is correct. Could we reopen this until we have converged, or reopen the associated issues so we can try to find other solutions?

asklar commented 2 years ago

Also, @CaseyCarter confirmed that deducing this is not dropped from C++23 (thanks @sylveon for asking the question), so reopening this hope that's ok : )

kennykerr commented 2 years ago

in general we don't close issues this way

Happy to continue discussing and its certainly easy enough to reopen an issue if needed. I just like to run a tight ship on the repos I maintain and prefer not to keep issues open when there's no progress or actionable work to be done. So, I'm happy to leave it open if folks would like to discuss this further but the conversation can also continue on a closed issue, and I will eventually close it again if there's no progress toward action. ๐Ÿ˜‰

Could you clarify what does/doesn't work with deducing this?

cppwinrt relies on CRTP very heavily to stitch together not only static polymorphism but also the static composition of the object's memory layout. The "deducing this" feature cannot do the latter according to @TartanLlama and @cdacamar. If someone can produce a solution involving C++23 that gets rid of CRTP in cppwinrt I'm happy to consider it.

cdacamar commented 2 years ago

cppwinrt relies on CRTP very heavily to stitch together not only static polymorphism but also the static composition of the object's memory layout.

Do you have a small example of how this works today? My initial thought is that it reminds me of ECS, but I want to confirm my suspicion. Based on how that looks the C++23 deducing this feature may or may not help.

kennykerr commented 2 years ago

It's hard to reduce to a small example. Conceptually at least, there needs to be a way to get the effect of virtual bases on sharing a base data member without all the other cost.

struct IUnknown { void* vtbl{}; };
struct IOne : virtual IUnknown {};
struct ITwo : virtual IUnknown {};
struct Class: IOne, ITwo {};

I need an instance of Class and an instance of IOne to both be the size of a single pointer without using templates. I achieve this today with cppwinrt by deriving the first interface (e.g. IOne) directly and any remaining interfaces (e.g. ITwo) have their methods stitched in with empty base templates and CRTP is then used to get at the single vtbl pointer provided by the first interface. The interfaces also need to know at compile time which vtbl pointer is actually at play. That's a gross simplification. For the gory details check out the talk that James and I gave at CppCon a few years back:

https://www.youtube.com/watch?v=lm4IwfiJ3EU

andy-schmidt commented 2 years ago

Is there any chance v3 would merge in the C++ Win32 Metadata projection? This would be a monumental enabler for Windows App SDK development with modern C++. And also for Windows Services development with modern C++.

asklar commented 2 years ago

@andy-schmidt I don't think that makes sense. Apps that want to use cppwin32-projected APIs can reference that package if they so choose. We should avoid scope creep (the list at the top of this issue is already huge), and it doesn't belong in cppwinrt IMO.

andy-schmidt commented 2 years ago

@asklar I am asking because merging cppwin32 and cppwinrt is listed as a potential goal for cppwin32. Additionally, it appears that development of cppwin32 as a standalone project/component has been abandoned by Microsoft, and I was hoping that was because the two projects were to be merged. It really does appear that the C++ language projection for the Windows API is vaporware.

As for scope creep, this is a major new version, not a minor bump and at the very least I am trying to let Microsoft know that there is interest in both strong support for C++ 20/23 in C++/WinRT and for long term development of a modern C++ language projection for the Windows API. Microsoft Foundation Classes and the Active Template Library are poor choices for new C++ development.

sylveon commented 2 years ago

cppwin32 would not replace MFC or ATL, it (and cswin32) provides a raw projection of the Windows API, with some helpers at best. Not a fully fledged framework.

But in C++, we already have this, it's just using the normal PSDK headers and using wil for the helpers.

While I do agree that cppwin32 would help in removing some ugly parts of the platform SDK headers (like the min and max macros), the platform SDK headers are the "source of truth" (the metadata used by cswin32 and cppwin32 is generated by scraping them). Some of the metadata is not always correct or present due to being scrapped (GetMessage being the most egregious example), and it's more complexity than needed when C++ can just use the source of truth directly.

cdacamar commented 2 years ago

@kennykerr It's not clear to me that the C++23 feature "Deducing this" would radically simplify that particular pattern. It could potentially clean up a lot of casts though, for example from your talk you have a case where impl_IStorageItem contained a lot of casts to get your final result. With "Deducing this" the same code could look like:

struct IStorageItem {
    virtual const char* name() const;
};

struct impl_IStorageItem {
    template <typename Derived>
    const char* name(this const Derived& self) {
        return static_cast<const IStorageItem&>(self).name();
    }
};

struct IStorageFile : impl_IStorageItem {
    operator IStorageItem() const;
};

const char* get_name(const IStorageFile& file) {
    return file.name();
}

Does this help illustrate the possible refactor potential?

kennykerr commented 2 years ago

@cdacamar indeed that would work. I'm not sure whether its a lot simpler, but there appears to be a code gen bug. Here's a minimal standalone example that proves it works at least. ๐Ÿ˜‰

The static_cast injects a copy even if the type does not need to be converted. In COM land, this forces an expensive injection of AddRef/Release that would not be acceptable. Can you confirm this is a bug or am I doing something wrong?

#include <stdint.h>
#include <stdio.h>
#include <string>
#include <format>

struct IInspectable { 
    size_t ptr{};
    explicit IInspectable(size_t ptr) : ptr(ptr) {}
    IInspectable(IInspectable const& other) :ptr(other.ptr) {
        printf("copy\n");
    }
    ~IInspectable() {
        printf("drop\n");
    }
};

static_assert(sizeof(IInspectable) == sizeof(void*));

struct IStringable;

struct impl_IStringable {
    template <typename D>
    std::string ToString(this const D& self) {
        // PROBLEM: this static_cast injects a copy even if D is IStringable
        auto d = static_cast<const IStringable&>(self);
        // omit actual call through vtbl for brevity
        return std::format("{}", d.ptr);
    }
};

struct IStringable : IInspectable, impl_IStringable {
    explicit IStringable(size_t ptr) : IInspectable(ptr) {}
};

static_assert(sizeof(IStringable) == sizeof(void*));

struct Uri : IInspectable, impl_IStringable {
    explicit Uri(size_t ptr) : IInspectable(ptr) {}
    operator IStringable() const {
        printf("query\n");
        return IStringable(ptr * 2);
    }
};

static_assert(sizeof(Uri) == sizeof(void*));

void print(const IStringable& d) {
    printf("%s\n", d.ToString().c_str());
}

int main()
{
    {
        IStringable s(123);
        print(s);
    }
    printf("\n");
    {
        Uri uri(456);
        print(uri);

        printf("\n");

        IStringable uri_s = uri;
        print(uri_s);
    }
}

The actual result is:

copy
drop
123
drop

query
copy
drop
912
drop

query
copy
drop
912
drop
drop

But the expected result is:

123
drop

query
912
drop

query
912
drop
drop
cdacamar commented 2 years ago

@kennykerr That compiler behavior is expected. If I create a similar program (removing Deducing this for GCC/Clang) I get a similar program output: https://godbolt.org/z/Kdze5f8no. What is happening there is that in the static_cast within the ToString() function the compiler is selecting the user-defined conversion function as the candidate and creates a lifetime-extended temporary there because the cast is to a const T&. Then you create a local variable copy of T to assign that lifetime-extended temporary to and the compiler has no choice but to create a copy-initialization out of that result, so you get a copy. A similar thing happens when the 'this' type is IStringable--the compiler is forced to copy it.

The fix is to make the local variable d also be a const auto&.

kennykerr commented 2 years ago

Of course, thanks for the reminder. I'm confusing C++'s auto with Rust's let, which does the right thing. ๐Ÿ˜œ

Seriously, that's pretty cool - thanks for the example. At least we know it works. Whether its compelling enough to switch is another question. I'll noodle on it some more now that I have a working prototype.

kennykerr commented 2 years ago

We have no plans to spin up a cppwinrt v3 at this point. Individual features, like std::source_location (#1185), may still be added from time to time, but overall there isn't enough to motivate a major new version at this point.

Scottj1s commented 1 year ago

Another feature to consider in making a (breaking change) major update is to make an explicit opt-in for version adaptive code ("light-up" scenarios) for WinRT APIs, requiring ApiInformation checks. For example, when TargetPlatformVersion != TargetPlatformMinVersion, C++/WinRT client apps could also require setting CppWinRTEnableVersionAdaptiveCode = true, or experience a build break. This would protect apps from inadvertently calling WinRT APIs that may not be present on TargetPlatformMinVersion devices. C#/WinRT-based apps have the SupportedOSPlatform attribute to assist, but C++ has no equivalent. Requiring the developer to consciously enable light-up scenarios would be an improvement.