Improve C++20 modules support

[sylveon]

This is a WIP PR which would resolve #951. It also solves warnings and errors with the existing ixx file.

It's not fully ready yet, and has several pitfalls

• Some basic example code builds but fails to run:

  #include <iostream>
  
  import winrt;
  
  using namespace winrt;
  using namespace Windows::Foundation;
  using namespace Windows::Web::Syndication;
  
  void PrintFeed(SyndicationFeed const& syndicationFeed)
  {
  	for (SyndicationItem const& syndicationItem : syndicationFeed.Items())
  	{
  		std::wcout << syndicationItem.Title().Text().c_str() << std::endl;
  	}
  }
  
  IAsyncAction ProcessFeedAsync()
  {
  	Uri rssFeedUri{ L"https://blogs.windows.com/feed" };
  	SyndicationClient syndicationClient;
  	SyndicationFeed syndicationFeed{ co_await syndicationClient.RetrieveFeedAsync(rssFeedUri) };
  	PrintFeed(syndicationFeed);
  }
  
  int main()
  {
  	winrt::init_apartment();
  
  	auto processOp{ ProcessFeedAsync() };
  	// do other work while the feed is being printed.
  	processOp.get(); // no more work to do; call get() so that we see the printout before the application exits.
  }

  Adding winrt::name_of<Uri>(); in main, weirdly, solves it and makes the example run as expected. This might be a codegen bug but I haven't taken time to reduce it yet.
• Coroutines using IAsyncOperation and IAsyncOperationWithProgress fail to build due to a compiler bug
• It currently relies on a MSVC specific extension to export template explicit and partial specializations from a module. It probably won't work on Clang, but it's the only thing to make use of specializations to work to begin with. I've started a thread on the std-proposals mailing list to see if we can get a CWG issue to resolve this.

[kennykerr]

Thanks for looking into this!

[sylveon]

It currently relies on a MSVC specific extension to export template explicit and partial specializations from a module. It probably won't work on Clang, but it's the only thing to make use of specializations to work to begin with. I've started a thread on the std-proposals mailing list to see if we can get a CWG issue to resolve this.

Apparently it is supposed to work without needing to be exported (specializations are visible if their declarations are reachable), MSVC has an issue filled with it, but I'm not sure if there's a DevCom feedback entry.

[kennykerr]

It doesn't look like there's much that can be done until a future version of the C++ compiler arrives with the necessary fixes. I'd rather not keep PRs open indefinitely. Shall we revisit this once support arrives in the compiler?

[sylveon]

Yep, I'll wait until these are all fixed.

[zadjii-msft]

Hey so I may be way out of my depth here, but could we workaround this on the cppwinrt side, by slightly adjusting the definition of resume_apartment_context?

If I'm understanding this correctly, the error comes from the last two lines in the resume_apartment_context struct, combined with a compiler error:

com_ptr<IContextCallback> m_context = try_capture<IContextCallback>(WINRT_IMPL_CoGetObjectContext);
int32_t m_context_type = get_apartment_type().first;

However, I think this can be fixed by replacing the default initialization of the members of resume_apartment_context with initialization in its default ctor:

The important bit is

@@ -8661,15 +9013,22 @@ namespace winrt::impl
             return true;
         case 2: /* APTTYPE_NA */
             return type.second == 3 /* APTTYPEQUALIFIER_NA_ON_STA */ ||
                 type.second == 5 /* APTTYPEQUALIFIER_NA_ON_MAINSTA */;
         }
         return false;
     }

     struct resume_apartment_context
     {
-        resume_apartment_context() = default;
-
+        com_ptr<IContextCallback> m_context;
+        int32_t m_context_type;
+        resume_apartment_context()
+        {
+            m_context = try_capture<IContextCallback>(WINRT_IMPL_CoGetObjectContext);
+            m_context_type = get_apartment_type().first;
+        };
         resume_apartment_context(std::nullptr_t) : m_context(nullptr), m_context_type(-1) {}
         resume_apartment_context(resume_apartment_context const&) = default;
         resume_apartment_context(resume_apartment_context&& other) noexcept :
             m_context(std::move(other.m_context)), m_context_type(std::exchange(other.m_context_type, -1)) {}
@@ -8684,9 +9043,6 @@ namespace winrt::impl
         {
             return m_context_type >= 0;
         }
-
-        com_ptr<IContextCallback> m_context = try_capture<IContextCallback>(WINRT_IMPL_CoGetObjectContext);
-        int32_t m_context_type = get_apartment_type().first;
     };

     inline int32_t __stdcall resume_apartment_callback(com_callback_args* args) noexcept

Would that end up being effectively the same code? I tried making that change locally in a test solution, and that seemed to allow the code to compile with a cppwinrt module. Thoughts?

[sylveon]

That's not the only issue. There's the nameof issue I mentionned above still affecting cppwinrt, preventing most code from running.

[zadjii-msft]

True, I saw that too. Just figured that some incremental progress might be better than nothing at all. Do we have any idea what specifically is causing the name_of thing? If not, I'll try taking a look (though, admittedly, my template-fu is not strong)

(also /cc @cdacamar as an fyi)

[Scottj1s]

@sylveon, @zadjii-msft

Good news -

Use of modules with C++/WinRT winrt::impl::name_v causes ICEs and/or runtime errors has been fixed with Version 17.3.0 Preview 2.0 [32530.156.main]

[sylveon]

Sweet!

I've been working recently on support for having one module partition per namespace (well more like 4 because we still need 0.ixx, 1.ixx 2.ixx and then a proper ixx), but am hitting some weird linker errors.

The linker complains it can't find consume_Windows_Foundation_IReference::Value, despite the template's full code being defined right above the method that uses it (specifically, the IReference equality operator). If we comment out the .Value() call from that operator (to see if at least consuming other WinRT types works), we somehow get an internal compiler error during linking.

If we can get this working though, it has significant advantages over bringing all headers in a single massive module:

• It builds faster, due to being able to take advantage of multi-processor compilation.
• It can rebuild lazily. For example changing one API in a specific namespace only causes recompilation of the ixx for that namespace and other namespaces which consume types from it. It doesn't rebuild the entire winrt.ixx like currently (which takes upwards of 5 minutes on a 5900X), nor does it rebuild the entire PCH (like it would pre-modules).

I wonder if VS 17.3 preview 2 would help - it doesn't appear to be out to the public yet

FWIW, the internal compiler error is the following:

t:\projects\cppwinrt\_build\x64\release\winrt\ixx\winrt.ixx : fatal error C1001: Internal compiler error. [T:\Projects\cppwinrt\test\test_cpp20\test_cpp20.vcxproj]
  (compiler file 'D:\a\_work\1\s\src\vctools\Compiler\Utc\src\p2\main.c', line 220)
   To work around this problem, try simplifying or changing the program near the locations listed above.
  If possible please provide a repro here: https://developercommunity.visualstudio.com
  Please choose the Technical Support command on the Visual C++
   Help menu, or open the Technical Support help file for more information
    link!InvokeCompilerPassW()+0x5797
    link!InvokeCompilerPassW()+0x5797


LINK : fatal error LNK1000: Internal error during IMAGE::BuildImage [T:\Projects\cppwinrt\test\test_cpp20\test_cpp20.vcxproj]

[sylveon]

I have a branch with a couple more modules-specific fixes for the massive single ixx approach in here: https://github.com/sylveon/cppwinrt/tree/user/sylveon/modules

[sylveon]

Okay, it appears the linker ICE is part of the issue that is fixed in preview 2!

For the undefined symbol issue, it's this: https://developercommunity.visualstudio.com/t/c20-modules-unresolved-external-symbol/10049210

Once these are resolved, if I'm lucky enough my solution will work!

[Scottj1s]

@sylveon, great - I was hoping all the ICE issues were fixed for you as well. One question I have is around structure. It sounds like you're advocating a fine-grained approach, based on namespace. I could see that, especially if higher level modules pulled in lower levels, like:

1. direct replacement of existing PCH (entire projection surface)
   import winrt;
   using namespace winrt;
   using namespace Windows::Foundation;
   using namespace Windows::Web::Syndication;
   ...

2. targeted namespace module import
   import winrt_Windows_Foundation;
   using namespace winrt;
   using namespace Windows::Foundation;
   ...

Is it necessary to create modules for the *.0.h, *.1.h, *.2.h groups as well? That structure was needed to prevent circular references - may still be needed with modules.

[kennykerr]

Do C++ modules support circular references?

[Scottj1s]

No idea - some experimentation needed. If so, we probably don't need the fine-grained structure.

[sylveon]

Yes, it's still necessary to create modules for *.0.h, *.1.h, *.2.h. Modules do not support circular references, and if the compiler finds one, it will simply complain and exit (which is, mind you, a much better error message than what'd you get for circular includes in headers):

error : Cannot build the following source files because there is a cyclic dependency between them: B.ixx depends on D.ixx depends on B.ixx.

Right now the structure is to use module partition units (4 per namespace - 0, 1, 2 and the main header) and then have a single winrt module that does export import :Windows.Foundation; and etc for every namespace.

Having one module per namespace is a bit trickier (if not outright impossible), due to the aforementioned circular reference issue. We'd need to have separate modules (not just module partition units, because you can't import a module's partition units outside of the module) for 0, 1 and 2 (so that modules for a specific namespace can refer to another without introducing cyclic dependencies), but this introduces the issue that a class forward declared in module winrt_Windows_Foundation_0 is entirely different from one defined in module winrt_Windows_Foundation_2, because they are part of a different module.

This isn't an issue when using module partition units for a massive single winrt module, since linkage is per-module, not per partition unit, so a class forward declared in winrt:Windows.Foundation.0 is the same as the one defined in winrt:Windows.Foundation.2

[kennykerr]

I've only experimented with having the entire projection in a single module. We should clearly identify the pros and cons of the two approaches. At a glance, one module per namespace and layer seems pretty complex.

[sylveon]

I'm using partition units ultimately exposed in a single module because the other approach (one module per namespace) wouldn't work as far as I can see (due to reasons described above), but a single massive ixx that includes everything takes forever to compile due to not being able to take advantage of multi-core processors, and also requires to be rebuilt entirely for every change.

Using partition units allows the compiler to compile them in parallel (it builds up a dependency graph and then compiles from the leaves all the way to the root using one core per source file), and also allows it to lazily recompile (e.g. a change in the API for Foo.Bar does not require the compiler to reprocess all of Windows.* again, making the inner loop much faster). So we get the most important advantages of separate modules, but without the drawbacks regarding solving circular references (since we can keep the 0, 1, 2 layout).

Another point for using the single winrt module, is that we don't need to export anything in winrt::impl, while with separate modules we'd need a winrt.base that exports some parts of winrt::impl for other modules to use.

[Scottj1s]

Yes, I found that the monolithic ixx was 3-4 times the compile time of the PCH, so I like the finer-grained approach. But are partitions necessary for the 0, 1, 2 files? Can we create these as modules instead and just export-import them as needed into top-level namespace modules? Does that run into having to export impl namespace artifacts? How close are you to pushing your changes? I found I had to export several impl artifacts for component projections.

[cdacamar]

Do C++ modules support circular references?

No. At the interface-level, you cannot create a circular reference; the compiler and standard will not let you: [module.import]/10.

Speaking to splitting the implementation up into multiple partitions: there are pros and cons to it, with more pros than cons. Many of the pros involve better inner-loop development testing. If you need to rebuild a single partition then there's a good chance that you will not spend time building the others which do not directly depend on it. It provides better component isolation in that you can more easily reason about the contents of individual module partition files than you can navigating the single .ixx.

Some cons are: they complicate the build graph. The build system needs to organize all of your partitions in a way that they are efficient to build and semantically correct for the purposes of interface dependency. There is a small performance hit in loading several .ifcs which compose the entire interface (the compiler needs to read X files on disk as opposed to the single artifact in the monolithic .ixx case.

If you ask me, I will almost always go for the partition-based approach. I like the hygiene of having my components logically separated and it's easier for me to locally reason about individual files if they're much smaller, even if they drag in extra interfaces.

[sylveon]

But are partitions necessary for the 0, 1, 2 files? Can we create these as modules instead and just export-import them as needed into top-level namespace modules?

We can't really make the independent modules, because 0 forward declares much of the interface which is then implemented in 1 and 2 and the main header. If they where separate modules, then a class declared in 0 is not the same as the one implemented in 2.

Does that run into having to export impl namespace artifacts?

Yes, because they are separate modules. You can only use non-exported symbols within the same module's partition units.

How close are you to pushing your changes?

I'm mostly blocked on this https://developercommunity.visualstudio.com/t/c20-modules-unresolved-external-symbol/10049210 before getting a MVP. There are a few other things that I needed to remove/disable so far to get it compiling, and it needs some general cleanup (lots of hardcoded strings that should be moved to the strings folder). I can do some general cleanup and push the changes on my fork.

[Scottj1s]

@sylveon any updates to report? Can we make progress with the workarounds suggested by Cameron DaCamara for https://developercommunity.visualstudio.com/t/c20-modules-unresolved-external-symbol/10049210? It might be a while before a proper fix is available.

[sylveon]

While some of the issues have been fixed (the name_v runtime error and linker ICE), that specific one hasn't, and the suggested workaround (out-of-class definitions being in the same translation unit/module partition) is not viable for cppwinrt because it would introduce a circular dependency between module partitions, which is not supported.

@cdacamar maybe knows if there's any work going to fix the issue.

I would love to progress further as well but this is a blocker.

[cdacamar]

@sylveon You should be able to define any member function out-of-class as a template workaround. One such example:

struct U {
    void g();
};

template <typename T>
struct S {
    void f(U u);
};

template <typename T>
void S<T>::f(U u) {
    u.g();
}

void use_S_f() {
    S<int> s;
    s.f({ });
}

The snippet above has a strong dependency on the U, but I can declare S<T>::f as a template to avoid the problem:

struct U;

template <typename T>
struct S {
    template <typename X = U>
    void f(X u);
};

template <typename T>
template <typename X>
void S<T>::f(X u) {
    u.g();
}

struct U {
    void g();
};

void use_S_f() {
    S<int> s;
    s.f({ });
}

So you essentially decouple the definition of U from the template which needs it so all the names used from U are dependent in the member function.

Does the above help workaround your problem or is the C++/WinRT sample substantially different from the above?

[sylveon]

Unfortunately the recommended solution "solves" the issue by introducing another one: it allows passing unexpected types to WinRT functions, and it will expose itself as runtime failures because of an unexpected vtable layout. Since cppwinrt immediately type erases it as a void*, no compiler type checking can fix the issue. cppwinrt relies on the function signature to enforce that the correct type is passed. Additionally, some circular dependencies show themselves only in the return types of functions (class A has a function which returns class B, while class B has a function which returns class A).

I have committed whatever work I have in my personal fork (hasn't been touched since June): https://github.com/sylveon/cppwinrt/tree/user/sylveon/modules

However, it is very much a WIP: it has some hardcoded things (for example, I forcibly removed all of the auto return type trickery to directly put the expected return type - I don't remember why, but I imagine it was probably to workaround some compiler bug or modules limitation), several TODOs, broken or disabled stuff, and may not represent the final design I end up using in the end (when I was working on it I had to go back to the drawing table a few times because of limitations or compiler bugs).

Once the blocking compiler bug is fixed, I would be very happy to resume work but as it stands the modules compile but any consumer fails linking so it's the farthest I can go.

[sylveon]

Note that my work focuses purely on consuming WinRT types, both system and user ones. I haven't looked at producing yet, I imagine this might be a much more complex endeavor (and we also have the thorny issue that the XAML compiler would almost certainly need an update - probably doable for WinUI 3, whenever that goes open source ;), but hard to imagine it happening for system XAML).

[zadjii-msft]

Hey I was just curious if there had ever been any movement on this in the last few months. I kinda lost track of this when I went on leave - did we ever figure out a way to resolve this (XAML issues notwithstanding)?

If there's a list of outstanding compiler bugs that need resolving, I could forward those along to our compiler team friends.

[sylveon]

Last time I looked at this it was blocked by compiler bugs - I will take a look again and see if they've been resolved.

[cdacamar]

There should be far fewer compiler bugs now, but the primary bug c++20 modules unresolved external symbol
is still very much active :(. The blocker there is that the compiler needs better handling for out-of-class declarations spread out over multiple modules. The workaround, of course, is to keep your declarations/definitions grouped in a single source file, but based on some responses above that isn't always possible because a definition may rely on another type defined later, though making the function a template could also solve this problem.

[Scottj1s]

@sylveon just wanted to confirm that Cameron's workaround is indeed not viable? i.e., we can't employ a static_assert or concept to constrain the templated method back to its original type?

[sylveon]

It sounds plausible, but I'm not sure if it has further impacts, e.g. source compatibility, compilation times, binary size, etc.