New props infrastructure is not portable

[asklar]

Description

This commit: 47280de

Introduced a compile-time string hashing mechanism (perhaps inspired by this) to do switch statements on string values.

However the resulting code uses clang-specific pragmas, which won't work with other compilers (e.g. MSVC).
This breaks ingestion of RN core into React Native Windows.

Version

main

Output of npx react-native info

n/a

Steps to reproduce

n/a

Snack, code example, screenshot, or link to a repository

n/a

[chiaramooney]

Pipeline showing break in RNW: https://dev.azure.com/ms/react-native-windows/_build/results?buildId=323774&view=results
Broken Files: Props.cpp and BaseTextProps.cpp.
Pragma Code:

react-native/ReactCommon/react/renderer/core/PropsMacros.h

Lines 17 to 25 in 577582e

	#define CONSTEXPR_RAW_PROPS_KEY_HASH(s) \
	({ \
	_Pragma("clang diagnostic push") _Pragma( \
	"clang diagnostic ignored \"-Wshadow\"") constexpr RawPropsPropNameLength \
	len = sizeof(s) - 1; \
	constexpr RawPropsPropNameHash hash = folly::hash::fnv32_buf(s, len); \
	hash; \
	_Pragma("clang diagnostic pop") \
	})

[asklar]

Cc @JoshuaGross

[lyahdav]

Looks like it should be wrapped with #if __clang__. Example:

react-native/ReactCommon/cxxreact/ReactMarker.cpp

Line 14 in 8bd3ede

#if __clang__

[lyahdav]

@asklar do you know if in React Native Windows we'll get a warning or error if we had this code as-is without the pragma? The issue with Clang was that this code could introduce variables len and hash which could shadow variables with the same name in an outer scope. If React Native Windows has MSVC configured to produce an error for this situation we'll have to add the MSVC pragma equivalent. If so, do you know what that is?

[asklar]

if we remove the pragma, I imagine msvc might produce a warning if the right warning level flag is set, perhaps @chiaramooney can try it.
is there a reason why we need the extra variable? why not just say:

#define CONSTEXPR_RAW_PROPS_KEY_HASH(s)    (folly::hash::fnv32_buf(s, sizeof(s) - 1))

is the problem that folly's fnv32_buf functions aren't constexpr? take a look at the PR I made a reference to originally and see if that helps, that does everything in constexpr without any funky macros

[JoshuaGross]

@asklar I should be able to change the macro into a consteval function to enforce its evaluation at compile-time. Let me know if there are any concerns with doing that for MSVC. It looks like you're using consteval in https://github.com/microsoft/react-native-windows/pull/9841/files as well (I had not seen this PR previously) so maybe that's an acceptable solution here; I'll give it a shot.

[asklar]

thanks @JoshuaGross for taking a look! Yeah I'm using a macro to do consteval for cpp20 and constexpr otherwise; we don't yet use cpp20, so this would downgrade to constexpr for us.

[JoshuaGross]

Yeah we're not on C++20 yet either, so I'll have to find something that works with just constexpr. I'll play around a little bit. If y'all get to it faster, let me know if MSVC is fine with just removing the pragmas.

[JoshuaGross]

@asklar What version of C++ are you on? Can I assume at least C++17?

[asklar]

yes we're on cpp17

[JoshuaGross]

I have a local diff we can try out. @asklar want to see if this works for you?

// We need to use clang pragmas inside of a macro below,
// so we need to pull out the "if" statement here.
#if __clang__
#define CLANG_PRAGMA(s) _Pragma(s)
#else
#define CLANG_PRAGMA(s)
#endif

// Get hash at compile-time. sizeof(str) - 1 == strlen
#define CONSTEXPR_RAW_PROPS_KEY_HASH(s)                   \
  ({                                                      \
    CLANG_PRAGMA("clang diagnostic push")                 \
    CLANG_PRAGMA("clang diagnostic ignored \"-Wshadow\"") \
    constexpr RawPropsPropNameHash hash =                 \
        folly::hash::fnv32_buf(s, sizeof(s) - 1);         \
    hash;                                                 \
    CLANG_PRAGMA("clang diagnostic pop")                  \
  })

I'll have more commentary if/when I land the actual diff. I'm still trying to avoid doing this the "proper" way without macros because it results in 10x as much code and is IMO a lot less readable, though that's highly subjective. If this compiles on MSVC let's do this; if not, is there a scoped way to ignore shadowing warnings in MSVC?

[JoshuaGross]

d1d11c7

[JoshuaGross]

@asklar please reopen if this is still an issue on your end

[chiaramooney]

@JoshuaGross Error still present following attempt to integrate 7/4 nightly build. cc @asklar

[lyahdav]

@chiaramooney can you confirm it's coming from the same place that @JoshuaGross modified in d1d11c7? If so, can you see if somehow __clang__ is defined in MSVC builds?

[chiaramooney]

Yes same place. @asklar Off the top of your head do you know if clang is defined in MSVC builds, otherwise I can investigate?

[asklar]

__clang__ is not defined for MSVC:

[asklar]

Looks like I don't have permissions to reopen, @JoshuaGross mind reopening this?

[lyahdav]

__clang__ is not defined for MSVC:

@asklar so if __clang__ is not defined, I'm confused how it's running code under #if __clang__. Can we get a link to the full build output and can we confirm that the RNW build included d1d11c7? Maybe it's coming from another file that needs a similar fix?

[chiaramooney]

@lyahdav https://dev.azure.com/ms/react-native-windows/_build/results?buildId=328217&view=results

[asklar]

it looks like the syntax case ({something}): is not valid for MSVC:
https://godbolt.org/z/P4qh7rKrf

[asklar]

ok, looks like that syntax is a language extension in GCC (and clang implements it too) but it is not standard C++:

https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

@JoshuaGross could we write this in a way that is portable? maybe move the "statement expression" inside of a #define instead of just the pragma

[asklar]

an alternative approach is to use an immediate lambda instead of a statement expression:

case [&]{ /* ... */ return something; }(): /* ... */

[lyahdav]

FYI @JoshuaGross is OOO until next week. If anyone from the MSFT side wants to put up a PR with a fix sooner, I can find someone on our side to review

[asklar]

@chiaramooney can you apply the change and send a PR to here?

[chiaramooney]

Trying

#define CONSTEXPR_RAW_PROPS_KEY_HASH(s)                   \
  []{                                                     \
    CLANG_PRAGMA("clang diagnostic push")                 \
    CLANG_PRAGMA("clang diagnostic ignored \"-Wshadow\"") \
    return folly::hash::fnv32_buf(s, sizeof(s) - 1);      \
    CLANG_PRAGMA("clang diagnostic pop")                  \
    }()

still results in the same syntax errors as case ({something}): @rozele might be looking into it to see what's wrong

[JoshuaGross]

@asklar @chiaramooney I'm back from leave but not sure when I'll be able to prioritize this. The only hard requirement/goal from my end is that these blocks continue to be compile-time evaluated on iOS and Android; it would be nice if that were the case for all platforms, but there might not be a portable way of doing it. Can one of you iterate on this until it compiles for MSVC? And then we can make sure the solution is acceptable for iOS/Android as well. I don't normally compile using MSVC so I'd prefer not to drive the solution.

[chiaramooney]

Alright I believe we have a fix. Both BaseTextProps.cpp and PropsMacros.h need edits. New versions of files can be found here:
https://github.com/microsoft/react-native-windows/blob/daf914bbb2d7ad7bc20cdac022818d0b65b3306b/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/react/renderer/components/text/BaseTextProps.cpp

https://github.com/microsoft/react-native-windows/blob/daf914bbb2d7ad7bc20cdac022818d0b65b3306b/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/react/renderer/core/PropsMacros.h

These changes use the approach of replacing all ({something}) syntax with lambda expressions in both the GET_FIELD_VALUE and CONSTEXPR_RAW_PROPS_KEY_HASH(s) macros and adding a semicolon to the end of the GET_FIELD_VALUE macro. @JoshuaGross @lyahdav

[chiaramooney]

@JoshuaGross @lyahdav If you have a moment to validate that the changes also work on your end. I can put out a PR.

[lyahdav]

@chiaramooney I'd say just put out a PR on this repo for it, and then we'll see if builds successfully in our CI.

[kelset]

@chiaramooney I noticed that your PR landed - do you need this in 0.70?

[lyahdav]

The PR was unlanded. Looks like it caused some regression. I'm awaiting some clarification internally on the status.

[lyahdav]

Looks like a fix for the regression is landing. The fix should work on MSVC, but we'll need someone from the MSFT side to verify. You can monitor for commits touching BaseTextProps.cpp to see when it's landed.

[kelset]

ok so let's postpone this to the next RC - we'll do RC2 without this, and you all can let me know if this will need to be in RC3

[asklar]

This issue is blocking ingestion into RNW, so if we can get this sorted out sooner than later (in rc1) that'd be great.

[asklar]

@lyahdav can you reopen this issue if it still needs to be [re-]merged?

[lyahdav]

Looks like the fix has landed: a142a78. @rshest didn't test this on MSVC but it should work in theory. If there's an issue you can let us know here.