Build React Native Windows with C++ 20 #12219

NickGerleman · 2023-10-10T12:19:41Z

Changes:

Build flags:

Change compiler flag for ABI internals to build with C++ 20
Remove the /await flag which was used to enable experimental coroutines TS support in C++ 17
1. CppWinRT will also set this for you if you don't set stdcpp20 it looks like
Build integration-test-app and automation-channel with C++ 20, so we can get rid of their legacy coroutine usage (not strictly required)

Coroutines:

Replace coroutine functions with std::experimental::coroutine_handle with std::coroutine_handle
Replace std::future usage in coroutines with concurrency:task since the former relies on coroutine extensions not part of the C++ 20 standard (provided previously by /await)
1. This is recommended by https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/concurrency#asynchronously-return-a-non-windows-runtime-type

C++ 20 breaking changes

Replace a usage of allocator.construct() (deprecated in C++ 17 but silenced in RNW, removed in C++ 20) with placement new.
Use wstring when passing unicode file paths. u8string() is now its own type in C++ 20, that doesn't play nice with libraries not meant to support it, and there isn't a zero-cost way to convert between it and std::string. Using wstring APIs for fs path sink lets us avoid that, and ends up avoiding extra conversions/allocations since the path is already UTF-16 internally, and we previously converted to utf8, to convert back to utf16.
Interpret some string literal u8 strings in test data as char* using reinterepret_cast. This is sadly the portable way to do this, without relying on MSVC setting /utf-8 (which this project does).
Fix "Explicit template specialization cannot have a storage class" by moving a function outside of a struct

Folly:

Update folly to take in facebook/folly@abbd46b
Set FOLLY_CFG_NO_COROUTINES to avoid building Folly coroutine bits, not used by RN (consistent with Android/iOS build)
Made Fix MSVC Warning when compiling format.cpp facebook/folly#2078 to fix a warning explicitly set by /sdl in folly
Suppressed C4244 which showed in some Folly concurrency bits. This is already suppressed in other places, but the alternative would be to make an upstream PR or fork
Updated the forked files in TEMP_UntilFollyUpdate

Before merging

We should take in a new version of Folly with the change in Fix MSVC Warning when compiling format.cpp facebook/folly#2078
1. The original author of ContexprMath.h is on PTO until the end of the month, but I might be able to wrangle someone to accept it (RNW could otherwise fork, like done in other places, but best to avoid the debt)
We should be sure it is okay to suppress C4244 when building Folly (just Folly, not Folly header users). Some other places already do this though. Otherwise we might need to add a static_cast somewhere.

Effect to RNW consumers

This change should not require updates to any of the consumers of Microsoft.ReactNative. Some care might need to be taken for Office/Win32 build with Folly at the ABI boundary in order to be sure we avoid any ODR violations (though, we're likely to be fine).

Microsoft Reviewers: Open in CodeFlow

vnext/Folly/Folly.vcxproj

NickGerleman · 2023-10-11T09:06:05Z

This should allow removal of the patches mentioned in #12195, but some of the listed ones there were unrelated to C++ 20, and were related to casting instead. So it might be better to just remove everything at once in a future round of integrating changes.

NickGerleman · 2023-10-11T10:00:37Z

Failures for universal playground debug are also showing on last continuous: https://dev.azure.com/ms/react-native-windows/_build/results?buildId=504190

Hermes + Fabric E2ETest crash visiting the "TextInputs with key prop" RNTester page seems new though, and awfully strange. Looks like the job was able to upload a crash dump I can take a look at though.

Edit: The symbols weren't uploaded though 🤔.

vmoroz · 2023-10-11T15:04:19Z

vnext/Shared/Utils.h

@@ -20,6 +20,6 @@ struct Url {
  std::string Target();
 };

-std::future<std::string> getApplicationDataPath(const wchar_t *childfolder);
+concurrency::task<std::string> getApplicationDataPath(const wchar_t *childfolder);


While I understand the need for this change, it is very unfortunate one since we do not want to use the ConcRT library in Office (unless something changed recently). We must follow up on it and replace it with Mso::Future. Mso::Future does not support coroutines yet. Thus, we cannot use it here yet. @JunielKatarn, let's follow up on it. Until that this code cannot be adopted in Office.

I have checked our internal C++ alias and they do not recommend using std::future, ConcRT, or PPL (std::future is based on PPL).
I guess it was a reason why Raymond Chen created a new library that works well with C++/WinRT.
https://github.com/microsoft/wil/blob/master/include/wil/coroutine.h
This way we would need to add dependency on WIL, which we were avoiding so far.

Anyway, the change is good for now. We must follow up on that after the PR is merged.

We have few enough cases that we could also likely convert these to WinRT types and just use IAsyncOperation. It's also not entirely ideal, but would avoid concurrency runtime usage. Let me know what you think of that.

IMHO, in the current situation it would be the best to use IAsyncOperation, but I do not know how difficult to apply it. I will leave it up to your judgement.

vmoroz · 2023-10-11T15:07:24Z

@NickGerleman, thank you for bringing the C++20 support to RNW! It must be good to go as soon as it can pass the tests.

chiaramooney · 2023-10-11T17:47:07Z

Found a couple of issues that were filed the last time we looked into upgrading to C++20. @NickGerleman before merging can you check that these concerns are adressed?

NickGerleman · 2023-10-12T22:58:04Z

Found a couple of issues that were filed the last time we looked into upgrading to C++20. @NickGerleman before merging can you check that these concerns are adressed?

C++20 break in coroutines #7774

C++20 breaks #7748

I think we should be able to close these out. I think they were likely addressed during a previous cppwinrt bump.

acoates-ms · 2023-10-17T22:21:45Z

/azp run

azure-pipelines · 2023-10-17T22:21:53Z

Azure Pipelines failed to run 1 pipeline(s).

.ado/jobs/e2e-test.yml

NickGerleman · 2023-10-17T22:30:01Z

vnext/Folly/TEMP_UntilFollyUpdate/ConstexprMath.h

@@ -0,0 +1,968 @@
+/*


facebook/folly#2086 should hopefully land before next weeks Folly release, and also let us remove the fork of Hash.h

It is great to see changes to be done in Folly repo!
Just nitpicking on your folly PR: did you consider using "if constexpr" for signed/unsigned instead of cloning the constexpr_ipow template.

Folly targets C++ 14 as a minimum I believe.

NickGerleman · 2023-10-29T03:11:49Z

I plan to break apart this change into multiple smaller ones.

A revision of the all-in-one C++ 20 PR caused a crash n Fabric E2E tests. I wanted to look at the crash dump, alongside the pdb and DLL used by the build. The previous copy of this logic, reused for Fabric, was dependent on the structure for a C# appx build. This updates the direcotry based on what I locally observed for the Fabric E2E test build.

These projects make use of ts-specific `std::experimental` coroutine APIs. Bump to C++ 20, and C++ 20 standard coroutines. These both have WinRT binary boundaries, so this should have no interaction with other projects (and should have no impact to MSRN interop.

This sets a couple new preprocessor conditions: 1. `FOLLY_CFG_NO_COROUTINES` to prevent types like `folly::optional` from bringing in Folly's coroutine stack when Folly detects C++ 20 coroutines as available. 2. `_SILENCE_CXX20_U8PATH_DEPRECATION_WARNING`: Because the deprecation came before a suitable replacement API. This is more scoped than the `_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS` used by `React.Cpp.props`.

This API was deprecated in C++ 17, and removed from C++ 20. Replace it with placement new.

1. Both WinRT and std::filesystem use system-native UTF-16 paths on Windows. Defaulting to UTF16 lets us avoid conversion until needed, and fixes some instances of round-tripping between formats. 2. We rely on `path.u8string()` to return a Unicode representation of a path, but that API in C++ 20 returns `std::u8string` which is incompatible with every other existing utf-8 API. `path.string()` may convert to system codepage instead. 3. This makes coroutine usage more efficient when using `IAsyncOperation<winrt::hstring>`, the pattern to replace `std::future<std::string>` usage mechanically. `std::filesystem::path` internally uses Windows native UTF-16 path. We use `u8string()` to convert to UTF-8.

`u8` is needed to portably ensure source is parsed as UTF-8 (though RNW already has build settings to do this) In C++ 20, these are represented using `char8_t` instead of `char`, which makes them incompatible with the other UTF-8 string APIs. This wraps C string literals with `reinterpret_cast`, to keep the portablility benefits of `u8` but also store into a `std::string`.

…lass" Fixes "[Explicit template specialization cannot have a storage class](https://stackoverflow.com/questions/29041775/explicit-template-specialization-cannot-have-a-storage-class-member-method-spe)" in C++ 20 by moving a function outside of a struct.

This bumps Folly to 2023.10.16.00, and cherry-picks facebook/folly@e65afc9 which fixes some strict MSVC warnings seen in a new code, and one of the existing patches. We can remove ConstexprMath.h`, `CacheLocality.cpp`, and the existing patch for `Hash.h` upon intake of `2023.11.06.` when that is pushed. Folly is still being removed opportunistically from RN. We no longer use folly containers build folly futures on Android/iOS.

This moves projects in vnext to build with C++ 20 and C++ 20 coroutines. This should not impact the ABI of MSRN DLLs, but may need care for cases of Folly at legacy ABI boundary. 1. Set `<CppStandard>stdcpp20</CppStandard>` in `React.cpp.props` 2. Remove explicit usages of `/await` to opt into coroutines TS 3. Replace `std::experimental::coroutine_handle` with `std::coroutine_handle`

NickGerleman changed the title ~~WIP: C++ 20~~ Build React Native Windows with C++ 20 Oct 11, 2023

NickGerleman commented Oct 11, 2023

View reviewed changes

vnext/Folly/Folly.vcxproj Outdated Show resolved Hide resolved

NickGerleman force-pushed the patch-2 branch 2 times, most recently from a4bfa2c to e56614a Compare October 11, 2023 08:58

NickGerleman marked this pull request as ready for review October 11, 2023 09:00

NickGerleman requested review from a team as code owners October 11, 2023 09:00

vmoroz reviewed Oct 11, 2023

View reviewed changes

NickGerleman requested a review from a team as a code owner October 17, 2023 21:24

NickGerleman requested a review from a team as a code owner October 17, 2023 22:17

NickGerleman commented Oct 17, 2023

View reviewed changes

.ado/jobs/e2e-test.yml Outdated Show resolved Hide resolved

NickGerleman commented Oct 17, 2023

View reviewed changes

chiaramooney assigned chiaramooney and vmoroz Oct 25, 2023

NickGerleman marked this pull request as draft October 29, 2023 03:11

NickGerleman force-pushed the patch-2 branch from 245d5ef to 74e629d Compare November 1, 2023 06:13

NickGerleman added 5 commits October 31, 2023 23:22

C++ 20: Replace Usage of std::allocator::construct()

309768e

This API was deprecated in C++ 17, and removed from C++ 20. Replace it with placement new.

NickGerleman added 4 commits November 1, 2023 00:31

NickGerleman force-pushed the patch-2 branch from 74e629d to 4cfb5e6 Compare November 1, 2023 07:31

NickGerleman closed this Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build React Native Windows with C++ 20 #12219

Build React Native Windows with C++ 20 #12219

NickGerleman commented Oct 10, 2023 •

edited

Loading

NickGerleman commented Oct 11, 2023

NickGerleman commented Oct 11, 2023 •

edited

Loading

vmoroz Oct 11, 2023

vmoroz Oct 11, 2023

NickGerleman Oct 11, 2023

vmoroz Oct 11, 2023

vmoroz commented Oct 11, 2023

chiaramooney commented Oct 11, 2023

NickGerleman commented Oct 12, 2023

acoates-ms commented Oct 17, 2023

azure-pipelines bot commented Oct 17, 2023

NickGerleman Oct 17, 2023

vmoroz Oct 17, 2023

NickGerleman Oct 28, 2023

NickGerleman commented Oct 29, 2023 •

edited

Loading

Build React Native Windows with C++ 20 #12219

Build React Native Windows with C++ 20 #12219

Conversation

NickGerleman commented Oct 10, 2023 • edited Loading

Changes:

Before merging

Effect to RNW consumers

Microsoft Reviewers: Open in CodeFlow

NickGerleman commented Oct 11, 2023

NickGerleman commented Oct 11, 2023 • edited Loading

vmoroz Oct 11, 2023

Choose a reason for hiding this comment

vmoroz Oct 11, 2023

Choose a reason for hiding this comment

NickGerleman Oct 11, 2023

Choose a reason for hiding this comment

vmoroz Oct 11, 2023

Choose a reason for hiding this comment

vmoroz commented Oct 11, 2023

chiaramooney commented Oct 11, 2023

NickGerleman commented Oct 12, 2023

acoates-ms commented Oct 17, 2023

azure-pipelines bot commented Oct 17, 2023

NickGerleman Oct 17, 2023

Choose a reason for hiding this comment

vmoroz Oct 17, 2023

Choose a reason for hiding this comment

NickGerleman Oct 28, 2023

Choose a reason for hiding this comment

NickGerleman commented Oct 29, 2023 • edited Loading

NickGerleman commented Oct 10, 2023 •

edited

Loading

NickGerleman commented Oct 11, 2023 •

edited

Loading

NickGerleman commented Oct 29, 2023 •

edited

Loading