Start enabling -Wextra and -Wconversion in "rn_xplat_cxx_library" (1/2)

Summary:
## Stack

These can suss out some real bugs, and helps further avoid mismatch with downstream MSVC on /W4 as used by MSFT.

I enabled the families of warnings, but suppressed some major individual warnings that weren't clean. But I did clean some up, notably, missing initializer, and shortening 64 bit to 32 bit. We can do some of the rest incrementally (e.g. `-Wunused-parameter` has a fixit).

This change illuminates that MapBuffer is missing 64 bit integer support, but we often pass 64 bit counters to it, which is a bug. For now I just left TODOs around those.

`rn_xplat_cxx_library` is used for external libraries interfacing with RN, which we probably don't want to police, so I structured these stricter warnings as an opt-in flag, only enabled for our own rules.

## Diff

This fixes up source code to avoid emitting the extra warnings now enforced. Of what is enabled, this is mostly shortening 64 to 32, or missing field in initializer.

Changelog: [Internal]

Reviewed By: christophpurrer

Differential Revision: D52589303

fbshipit-source-id: 11cb778d065799fd0ead3ae706934146d13500bb

Author: NickGerleman

packages/react-native/React/Base/RCTBridgeModule.h

@@ -236,7 +236,7 @@ RCT_EXTERN_C_END
  */
 #define RCT_REMAP_METHOD(js_name, method)       \
   _RCT_EXTERN_REMAP_METHOD(js_name, method, NO) \
-  -(void)method RCT_DYNAMIC;
+  -(void)method RCT_DYNAMIC
 
 /**
  * Similar to RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD but lets you set
@@ -248,7 +248,7 @@ RCT_EXTERN_C_END
  */
 #define RCT_REMAP_BLOCKING_SYNCHRONOUS_METHOD(js_name, returnType, method) \
   _RCT_EXTERN_REMAP_METHOD(js_name, method, YES)                           \
-  -(returnType)method RCT_DYNAMIC;
+  -(returnType)method RCT_DYNAMIC
 
 /**
  * Use this macro in a private Objective-C implementation file to automatically

packages/react-native/ReactAndroid/src/main/jni/react/fabric/CoreComponentsRegistry.cpp

@@ -85,14 +85,16 @@ CoreComponentsRegistry::initHybrid(
       [](const EventDispatcher::Weak& eventDispatcher,
          const ContextContainer::Shared& contextContainer)
       -> ComponentDescriptorRegistry::Shared {
+    ComponentDescriptorParameters params{
+        .eventDispatcher = eventDispatcher,
+        .contextContainer = contextContainer,
+        .flavor = nullptr};
+
     auto registry = CoreComponentsRegistry::sharedProviderRegistry()
-                        ->createComponentDescriptorRegistry(
-                            {eventDispatcher, contextContainer});
+                        ->createComponentDescriptorRegistry(params);
     auto& mutableRegistry = const_cast<ComponentDescriptorRegistry&>(*registry);
     mutableRegistry.setFallbackComponentDescriptor(
-        std::make_shared<UnimplementedNativeViewComponentDescriptor>(
-            ComponentDescriptorParameters{
-                eventDispatcher, contextContainer, nullptr}));
+        std::make_shared<UnimplementedNativeViewComponentDescriptor>(params));
 
     return registry;
   };

packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp

@@ -85,7 +85,7 @@ static inline int getIntBufferSizeForType(CppMountItem::Type mountItemType) {
 
 static inline void updateBufferSizes(
     CppMountItem::Type mountItemType,
-    int numInstructions,
+    size_t numInstructions,
     int& batchMountItemIntsSize,
     int& batchMountItemObjectsSize) {
   if (numInstructions == 0) {
@@ -182,7 +182,7 @@ static inline void computeBufferSizes(
 
 static inline void writeIntBufferTypePreamble(
     int mountItemType,
-    int numItems,
+    size_t numItems,
     _JNIEnv* env,
     jintArray& intBufferArray,
     int& intBufferPosition) {
@@ -193,7 +193,7 @@ static inline void writeIntBufferTypePreamble(
     intBufferPosition += 1;
   } else {
     temp[0] = mountItemType | CppMountItem::Type::Multiple;
-    temp[1] = numItems;
+    temp[1] = static_cast<jint>(numItems);
     env->SetIntArrayRegion(intBufferArray, intBufferPosition, 2, temp);
     intBufferPosition += 2;
   }

packages/react-native/ReactAndroid/src/main/jni/react/fabric/ReactNativeConfigHolder.cpp

@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-#import "ReactNativeConfigHolder.h"
+#include "ReactNativeConfigHolder.h"
 
 #include <fbjni/fbjni.h>
 

packages/react-native/ReactAndroid/src/main/jni/react/jni/MethodInvoker.cpp

@@ -59,7 +59,8 @@ jint extractInteger(const folly::dynamic& value) {
   // The logic here is taken from convertDynamicIfIntegral, but the
   // return type and exception are different.
   if (value.isInt()) {
-    return value.getInt();
+    // TODO: this truncates 64 bit ints, valid in JS
+    return static_cast<jint>(value.getInt());
   }
   double dbl = value.getDouble();
   jint result = static_cast<jint>(dbl);
@@ -227,7 +228,7 @@ MethodCallResult MethodInvoker::invoke(
 
   auto env = Environment::current();
   auto argCount = signature_.size() - 2;
-  JniLocalScope scope(env, argCount);
+  JniLocalScope scope(env, static_cast<int>(argCount));
   jvalue args[argCount];
   std::transform(
       signature_.begin() + 2,

packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp

@@ -23,7 +23,7 @@ void ReadableNativeArray::mapException(std::exception_ptr ex) {
 }
 
 local_ref<JArrayClass<jobject>> ReadableNativeArray::importArray() {
-  jint size = array_.size();
+  auto size = static_cast<jint>(array_.size());
   auto jarray = JArrayClass<jobject>::newArray(size);
   for (jint ii = 0; ii < size; ii++) {
     addDynamicToJArray(jarray, ii, array_.at(ii));
@@ -32,7 +32,7 @@ local_ref<JArrayClass<jobject>> ReadableNativeArray::importArray() {
 }
 
 local_ref<JArrayClass<jobject>> ReadableNativeArray::importTypeArray() {
-  jint size = array_.size();
+  auto size = static_cast<jint>(array_.size());
   auto jarray = JArrayClass<jobject>::newArray(size);
   for (jint ii = 0; ii < size; ii++) {
     (*jarray)[ii] = ReadableType::getType(array_.at(ii).type());

packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp

@@ -80,7 +80,7 @@ local_ref<JArrayClass<jstring>> ReadableNativeMap::importKeys() {
 local_ref<JArrayClass<jobject>> ReadableNativeMap::importValues() {
   throwIfConsumed();
 
-  jint size = keys_.value().size();
+  auto size = static_cast<jint>(keys_.value().size());
   auto jarray = JArrayClass<jobject>::newArray(size);
   for (jint ii = 0; ii < size; ii++) {
     const std::string& key = (*keys_)[ii].getString();
@@ -92,7 +92,7 @@ local_ref<JArrayClass<jobject>> ReadableNativeMap::importValues() {
 local_ref<JArrayClass<jobject>> ReadableNativeMap::importTypes() {
   throwIfConsumed();
 
-  jint size = keys_.value().size();
+  auto size = static_cast<jint>(keys_.value().size());
   auto jarray = JArrayClass<jobject>::newArray(size);
   for (jint ii = 0; ii < size; ii++) {
     const std::string& key = (*keys_)[ii].getString();

packages/react-native/ReactAndroid/src/main/jni/react/newarchdefaults/DefaultComponentsRegistry.cpp

@@ -40,15 +40,17 @@ DefaultComponentsRegistry::initHybrid(
       [](const EventDispatcher::Weak& eventDispatcher,
          const ContextContainer::Shared& contextContainer)
       -> ComponentDescriptorRegistry::Shared {
+    ComponentDescriptorParameters params{
+        .eventDispatcher = eventDispatcher,
+        .contextContainer = contextContainer,
+        .flavor = nullptr};
+
     auto registry = DefaultComponentsRegistry::sharedProviderRegistry()
-                        ->createComponentDescriptorRegistry(
-                            {eventDispatcher, contextContainer});
+                        ->createComponentDescriptorRegistry(params);
 
     auto& mutableRegistry = const_cast<ComponentDescriptorRegistry&>(*registry);
     mutableRegistry.setFallbackComponentDescriptor(
-        std::make_shared<UnimplementedNativeViewComponentDescriptor>(
-            ComponentDescriptorParameters{
-                eventDispatcher, contextContainer, nullptr}));
+        std::make_shared<UnimplementedNativeViewComponentDescriptor>(params));
 
     return registry;
   };

packages/react-native/ReactCommon/cxxreact/JSBigString.cpp

@@ -38,8 +38,8 @@ JSBigFileString::JSBigFileString(int fd, size_t size, off_t offset /*= 0*/)
     const static auto ps = sysconf(_SC_PAGESIZE);
     auto d = lldiv(offset, ps);
 
-    m_mapOff = d.quot;
-    m_pageOff = d.rem;
+    m_mapOff = static_cast<off_t>(d.quot);
+    m_pageOff = static_cast<off_t>(d.rem);
     m_size = size + m_pageOff;
   } else {
     m_mapOff = 0;

packages/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp

@@ -250,13 +250,15 @@ JNIArgs convertJSIArgsToJNIArgs(
     size_t count,
     const std::shared_ptr<CallInvoker>& jsInvoker,
     TurboModuleMethodValueKind valueKind) {
-  unsigned int expectedArgumentCount = valueKind == PromiseKind
+  size_t expectedArgumentCount = valueKind == PromiseKind
       ? methodArgTypes.size() - 1
       : methodArgTypes.size();
 
   if (expectedArgumentCount != count) {
     throw JavaTurboModuleInvalidArgumentCountException(
-        methodName, count, expectedArgumentCount);
+        methodName,
+        static_cast<int>(count),
+        static_cast<int>(expectedArgumentCount));
   }
 
   JNIArgs jniArgs(valueKind == PromiseKind ? count + 1 : count);
@@ -505,7 +507,8 @@ jsi::Value JavaTurboModule::invokeJavaMethod(
    * GlobalReferences. The LocalReferences are then promptly deleted
    * after the conversion.
    */
-  unsigned int actualArgCount = valueKind == VoidKind ? 0 : argCount;
+  unsigned int actualArgCount =
+      valueKind == VoidKind ? 0 : static_cast<unsigned int>(argCount);
   unsigned int estimatedLocalRefCount =
       actualArgCount + maxReturnObjects + buffer;
 

packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTInteropTurboModule.h

@@ -23,7 +23,7 @@ class JSI_EXPORT ObjCInteropTurboModule : public ObjCTurboModule {
   struct MethodDescriptor {
     std::string methodName;
     SEL selector;
-    int jsArgCount;
+    size_t jsArgCount;
     TurboModuleMethodValueKind jsReturnKind;
   };
 

packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTInteropTurboModule.mm

@@ -192,11 +192,11 @@ T RCTConvertTo(SEL selector, id json)
   methodArgumentTypeNames_ = methodArgTypeNames;
 
   for (const ExportedMethod &method : methods) {
-    const int numArgs = [method.argumentTypes count];
+    const size_t numArgs = [method.argumentTypes count];
     const bool isPromiseMethod =
         numArgs >= 2 && [method.argumentTypes[numArgs - 1] isEqualToString:@"RCTPromiseRejectBlock"];
 
-    const int jsArgCount = isPromiseMethod ? numArgs - 2 : numArgs;
+    const size_t jsArgCount = isPromiseMethod ? numArgs - 2 : numArgs;
 
     /**
      * In the TurboModule system, only promises and voids are special. So, set those.
@@ -330,7 +330,7 @@ T RCTConvertTo(SEL selector, id json)
   NSString *methodName = @(methodNameCStr);
   std::string methodJsSignature = name_ + "." + methodNameCStr + "()";
 
-  NSString *argumentType = getArgumentTypeName(runtime, methodName, index);
+  NSString *argumentType = getArgumentTypeName(runtime, methodName, static_cast<int>(index));
   std::string errorPrefix = methodJsSignature + ": Error while converting JavaScript argument " +
       std::to_string(index) + " to Objective C type " + [argumentType UTF8String] + ". ";
 
@@ -586,7 +586,7 @@ T RCTConvertTo(SEL selector, id json)
   }
 
   if ([methodArgumentTypeNames_[methodName] count] <= argIndex) {
-    int paramCount = [methodArgumentTypeNames_[methodName] count];
+    size_t paramCount = [methodArgumentTypeNames_[methodName] count];
     throw jsi::JSError(runtime, errorPrefix + "Method has only " + std::to_string(paramCount) + " parameter types.");
   }
 

packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.h

@@ -62,7 +62,7 @@ class JSI_EXPORT ObjCTurboModule : public TurboModule {
   std::shared_ptr<NativeMethodCallInvoker> nativeMethodCallInvoker_;
 
  protected:
-  void setMethodArgConversionSelector(NSString *methodName, int argIndex, NSString *fnName);
+  void setMethodArgConversionSelector(NSString *methodName, size_t argIndex, NSString *fnName);
 
   /**
    * Why is this virtual?
@@ -126,8 +126,8 @@ class JSI_EXPORT ObjCTurboModule : public TurboModule {
   NSDictionary<NSString *, NSArray<NSString *> *> *methodArgumentTypeNames_;
 
   bool isMethodSync(TurboModuleMethodValueKind returnType);
-  BOOL hasMethodArgConversionSelector(NSString *methodName, int argIndex);
-  SEL getMethodArgConversionSelector(NSString *methodName, int argIndex);
+  BOOL hasMethodArgConversionSelector(NSString *methodName, size_t argIndex);
+  SEL getMethodArgConversionSelector(NSString *methodName, size_t argIndex);
   NSInvocation *createMethodInvocation(
       jsi::Runtime &runtime,
       bool isSync,

packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm

@@ -595,7 +595,7 @@ SystraceSection s(
      * Convert objects using RCTConvert.
      */
     if (objCArgType == @encode(id)) {
-      NSString *argumentType = getArgumentTypeName(runtime, methodNameNSString, i);
+      NSString *argumentType = getArgumentTypeName(runtime, methodNameNSString, static_cast<int>(i));
       if (argumentType != nil) {
         NSString *rctConvertMethodName = [NSString stringWithFormat:@"%@:", argumentType];
         SEL rctConvertSelector = NSSelectorFromString(rctConvertMethodName);
@@ -776,19 +776,19 @@ SystraceSection s(
   return returnValue;
 }
 
-BOOL ObjCTurboModule::hasMethodArgConversionSelector(NSString *methodName, int argIndex)
+BOOL ObjCTurboModule::hasMethodArgConversionSelector(NSString *methodName, size_t argIndex)
 {
   return methodArgConversionSelectors_ && methodArgConversionSelectors_[methodName] &&
       ![methodArgConversionSelectors_[methodName][argIndex] isEqual:[NSNull null]];
 }
 
-SEL ObjCTurboModule::getMethodArgConversionSelector(NSString *methodName, int argIndex)
+SEL ObjCTurboModule::getMethodArgConversionSelector(NSString *methodName, size_t argIndex)
 {
   assert(hasMethodArgConversionSelector(methodName, argIndex));
   return (SEL)((NSValue *)methodArgConversionSelectors_[methodName][argIndex]).pointerValue;
 }
 
-void ObjCTurboModule::setMethodArgConversionSelector(NSString *methodName, int argIndex, NSString *fnName)
+void ObjCTurboModule::setMethodArgConversionSelector(NSString *methodName, size_t argIndex, NSString *fnName)
 {
   if (!methodArgConversionSelectors_) {
     methodArgConversionSelectors_ = [NSMutableDictionary new];

packages/react-native/ReactCommon/react/renderer/attributedstring/conversions.h

@@ -1162,9 +1162,10 @@ inline MapBuffer toMapBuffer(const AttributedString& attributedString) {
   }
 
   auto builder = MapBufferBuilder();
-  builder.putInt(
-      AS_KEY_HASH,
-      std::hash<facebook::react::AttributedString>{}(attributedString));
+  size_t hash =
+      std::hash<facebook::react::AttributedString>{}(attributedString);
+  // TODO: This truncates half the hash
+  builder.putInt(AS_KEY_HASH, static_cast<int>(hash));
   builder.putString(AS_KEY_STRING, attributedString.getString());
   auto fragmentsMap = fragmentsBuilder.build();
   builder.putMapBuffer(AS_KEY_FRAGMENTS, fragmentsMap);

packages/react-native/ReactCommon/react/renderer/components/legacyviewmanagerinterop/RCTLegacyViewManagerInteropCoordinator.mm

@@ -46,7 +46,7 @@ @implementation RCTLegacyViewManagerInteropCoordinator {
 - (instancetype)initWithComponentData:(RCTComponentData *)componentData
                                bridge:(nullable RCTBridge *)bridge
                           bridgeProxy:(nullable RCTBridgeProxy *)bridgeProxy
-                bridgelessInteropData:(RCTBridgeModuleDecorator *)bridgelessInteropData;
+                bridgelessInteropData:(RCTBridgeModuleDecorator *)bridgelessInteropData
 {
   if (self = [super init]) {
     _componentData = componentData;
@@ -89,7 +89,7 @@ - (void)removeObserveForTag:(NSInteger)tag
   [_eventInterceptors removeObjectForKey:[NSNumber numberWithInteger:tag]];
 }
 
-- (UIView *)createPaperViewWithTag:(NSInteger)tag;
+- (UIView *)createPaperViewWithTag:(NSInteger)tag
 {
   UIView *view = [_componentData createViewWithTag:[NSNumber numberWithInteger:tag] rootTag:NULL];
   [_bridgelessInteropData attachInteropAPIsToModule:(id<RCTBridgeModule>)_componentData.bridgelessViewManager];

packages/react-native/ReactCommon/react/renderer/components/textinput/platform/android/react/renderer/components/androidtextinput/AndroidTextInputState.cpp

@@ -88,7 +88,10 @@ MapBuffer AndroidTextInputState::getMapBuffer() const {
   auto builder = MapBufferBuilder();
   // See comment in getDynamic block.
   if (cachedAttributedStringId == 0) {
-    builder.putInt(TX_STATE_KEY_MOST_RECENT_EVENT_COUNT, mostRecentEventCount);
+    // TODO truncation
+    builder.putInt(
+        TX_STATE_KEY_MOST_RECENT_EVENT_COUNT,
+        static_cast<int32_t>(mostRecentEventCount));
 
     auto attStringMapBuffer = toMapBuffer(attributedString);
     builder.putMapBuffer(TX_STATE_KEY_ATTRIBUTED_STRING, attStringMapBuffer);

packages/react-native/ReactCommon/react/renderer/components/textinput/platform/ios/react/renderer/components/iostextinput/TextInputShadowNode.cpp

@@ -60,8 +60,11 @@ AttributedString TextInputShadowNode::getAttributedString(
       layoutContext.fontSizeMultiplier);
   auto attributedString = AttributedString{};
 
-  attributedString.appendFragment(
-      AttributedString::Fragment{getConcreteProps().text, textAttributes});
+  attributedString.appendFragment(AttributedString::Fragment{
+      .string = getConcreteProps().text,
+      .textAttributes = textAttributes,
+      // TODO: Is this really meant to be by value?
+      .parentShadowView = ShadowView{}});
 
   auto attachments = Attachments{};
   BaseTextShadowNode::buildAttributedString(

packages/react-native/ReactCommon/react/renderer/components/view/platform/android/react/renderer/components/view/AccessibilityPropsMapBuffer.cpp

@@ -12,7 +12,7 @@ namespace facebook::react {
 
 static MapBuffer convertAccessibilityActions(
     const std::vector<AccessibilityAction>& actions) {
-  MapBufferBuilder builder(actions.size());
+  MapBufferBuilder builder(static_cast<uint32_t>(actions.size()));
   for (auto i = 0; i < actions.size(); i++) {
     const auto& action = actions[i];
     MapBufferBuilder actionsBuilder(2);
@@ -28,7 +28,7 @@ static MapBuffer convertAccessibilityActions(
 
 static MapBuffer convertAccessibilityLabelledBy(
     const AccessibilityLabelledBy& labelledBy) {
-  MapBufferBuilder builder(labelledBy.value.size());
+  MapBufferBuilder builder(static_cast<uint32_t>(labelledBy.value.size()));
   for (auto i = 0; i < labelledBy.value.size(); i++) {
     builder.putString(i, labelledBy.value[i]);
   }

packages/react-native/ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.cpp

@@ -116,18 +116,18 @@ void MapBufferBuilder::putMapBuffer(MapBuffer::Key key, const MapBuffer& map) {
 void MapBufferBuilder::putMapBufferList(
     MapBuffer::Key key,
     const std::vector<MapBuffer>& mapBufferList) {
-  int32_t offset = dynamicData_.size();
+  auto offset = static_cast<int32_t>(dynamicData_.size());
   int32_t dataSize = 0;
   for (const MapBuffer& mapBuffer : mapBufferList) {
-    dataSize = dataSize + INT_SIZE + mapBuffer.size();
+    dataSize = dataSize + INT_SIZE + static_cast<int32_t>(mapBuffer.size());
   }
 
   dynamicData_.resize(offset + INT_SIZE, 0);
   memcpy(dynamicData_.data() + offset, &dataSize, INT_SIZE);
 
   for (const MapBuffer& mapBuffer : mapBufferList) {
-    int32_t mapBufferSize = mapBuffer.size();
-    int32_t dynamicDataSize = dynamicData_.size();
+    auto mapBufferSize = static_cast<int32_t>(mapBuffer.size());
+    auto dynamicDataSize = static_cast<int32_t>(dynamicData_.size());
     dynamicData_.resize(dynamicDataSize + INT_SIZE + mapBufferSize, 0);
     // format [length of buffer (int)] + [bytes of MapBuffer]
     memcpy(dynamicData_.data() + dynamicDataSize, &mapBufferSize, INT_SIZE);

packages/react-native/ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.h

@@ -27,6 +27,8 @@ class MapBufferBuilder {
 
   void putInt(MapBuffer::Key key, int32_t value);
 
+  // TODO: Support 64 bit integers
+
   void putBool(MapBuffer::Key key, bool value);
 
   void putDouble(MapBuffer::Key key, double value);