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: https://github.com/facebook/react-native/blob/577582e2303baa0ffce15e95cc406cf32cb5ceaa/ReactCommon/react/renderer/core/PropsMacros.h#L17-L25

[asklar]

Cc @JoshuaGross

[lyahdav]

Looks like it should be wrapped with #if __clang__. Example: https://github.com/facebook/react-native/blob/8bd3edec88148d0ab1f225d2119435681fbbba33/ReactCommon/cxxreact/ReactMarker.cpp#L14

[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?