Fix Cpp codegen handling of optional arguments

Summary: Changelog: [General][Fixed] - Codegen for C++ TurboModules of optional method arguments was incorrect Reviewed By: christophpurrer Differential Revision: D40979066 fbshipit-source-id: 5bb48dbafc14dcea21b7e0b15e3f4bb527bc8476
facebook · Nov 3, 2022 · e81c98c · e81c98c
1 parent f32a3a5
commit e81c98c
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 10 deletions.
diff --git a/packages/react-native-codegen/src/generators/modules/GenerateModuleCpp.js b/packages/react-native-codegen/src/generators/modules/GenerateModuleCpp.js
@@ -118,9 +118,10 @@ function serializeArg(
   index: number,
   resolveAlias: AliasResolver,
 ): string {
-  const {typeAnnotation: nullableTypeAnnotation} = arg;
+  const {typeAnnotation: nullableTypeAnnotation, optional} = arg;
   const [typeAnnotation, nullable] =
     unwrapNullable<NativeModuleParamTypeAnnotation>(nullableTypeAnnotation);
+  const isRequired = !optional && !nullable;
 
   let realTypeAnnotation = typeAnnotation;
   if (realTypeAnnotation.type === 'TypeAliasTypeAnnotation') {
@@ -130,12 +131,15 @@ function serializeArg(
   function wrap(callback: (val: string) => string) {
     const val = `args[${index}]`;
     const expression = callback(val);
-
-    if (nullable) {
-      return `${val}.isNull() || ${val}.isUndefined() ? std::nullopt : std::make_optional(${expression})`;
+    if (isRequired) {
+      return expression;
+    } else {
+      let condition = `${val}.isNull() || ${val}.isUndefined()`;
+      if (optional) {
+        condition = `count < ${index} || ${condition}`;
+      }
+      return `${condition} ? std::nullopt : std::make_optional(${expression})`;
     }
-
-    return expression;
   }
 
   switch (realTypeAnnotation.type) {

diff --git a/packages/react-native-codegen/src/generators/modules/GenerateModuleH.js b/packages/react-native-codegen/src/generators/modules/GenerateModuleH.js
@@ -107,20 +107,22 @@ ${modules.join('\n\n')}
 
 function translatePrimitiveJSTypeToCpp(
   nullableTypeAnnotation: Nullable<NativeModuleTypeAnnotation>,
+  optional: boolean,
   createErrorMessage: (typeName: string) => string,
   resolveAlias: AliasResolver,
 ) {
   const [typeAnnotation, nullable] = unwrapNullable<NativeModuleTypeAnnotation>(
     nullableTypeAnnotation,
   );
+  const isRequired = !optional && !nullable;
 
   let realTypeAnnotation = typeAnnotation;
   if (realTypeAnnotation.type === 'TypeAliasTypeAnnotation') {
     realTypeAnnotation = resolveAlias(realTypeAnnotation.name);
   }
 
   function wrap(type: string) {
-    return nullable ? `std::optional<${type}>` : type;
+    return isRequired ? type : `std::optional<${type}>`;
   }
 
   switch (realTypeAnnotation.type) {
@@ -199,6 +201,7 @@ function translatePropertyToCpp(
   const paramTypes = propTypeAnnotation.params.map(param => {
     const translatedParam = translatePrimitiveJSTypeToCpp(
       param.typeAnnotation,
+      param.optional,
       typeName =>
         `Unsupported type for param "${param.name}" in ${prop.name}. Found: ${typeName}`,
       resolveAlias,
@@ -208,6 +211,7 @@ function translatePropertyToCpp(
 
   const returnType = translatePrimitiveJSTypeToCpp(
     propTypeAnnotation.returnTypeAnnotation,
+    false,
     typeName => `Unsupported return type for ${prop.name}. Found: ${typeName}`,
     resolveAlias,
   );

diff --git a/packages/react-native-codegen/src/generators/modules/__test_fixtures__/fixtures.js b/packages/react-native-codegen/src/generators/modules/__test_fixtures__/fixtures.js
@@ -275,6 +275,25 @@ const SIMPLE_NATIVE_MODULES: SchemaType = {
               ],
             },
           },
+          {
+            name: 'getValueWithOptionalArg',
+            optional: false,
+            typeAnnotation: {
+              type: 'FunctionTypeAnnotation',
+              returnTypeAnnotation: {
+                type: 'PromiseTypeAnnotation',
+              },
+              params: [
+                {
+                  optional: true,
+                  name: 'parameter',
+                  typeAnnotation: {
+                    type: 'GenericObjectTypeAnnotation',
+                  },
+                },
+              ],
+            },
+          },
           {
             name: 'getEnums',
             optional: false,

diff --git a/...ive-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleCpp-test.js.snap b/...ive-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleCpp-test.js.snap
@@ -54,7 +54,7 @@ static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_optionals(jsi
   return jsi::Value::undefined();
 }
 static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_optionalMethod(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
-  static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->optionalMethod(rt, args[0].asObject(rt), args[1].asObject(rt).asFunction(rt), args[2].asObject(rt).asArray(rt));
+  static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->optionalMethod(rt, args[0].asObject(rt), args[1].asObject(rt).asFunction(rt), count < 2 || args[2].isNull() || args[2].isUndefined() ? std::nullopt : std::make_optional(args[2].asObject(rt).asArray(rt)));
   return jsi::Value::undefined();
 }
 static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getArrays(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
@@ -337,6 +337,9 @@ static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithC
 static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithPromise(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
   return static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->getValueWithPromise(rt, args[0].asBool());
 }
+static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithOptionalArg(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
+  return static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->getValueWithOptionalArg(rt, count < 0 || args[0].isNull() || args[0].isUndefined() ? std::nullopt : std::make_optional(args[0].asObject(rt)));
+}
 static jsi::Value __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getEnums(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {
   return static_cast<NativeSampleTurboModuleCxxSpecJSI *>(&turboModule)->getEnums(rt, args[0].asNumber(), args[1].asNumber(), args[2].asString(rt));
 }
@@ -354,6 +357,7 @@ NativeSampleTurboModuleCxxSpecJSI::NativeSampleTurboModuleCxxSpecJSI(std::shared
   methodMap_[\\"getValue\\"] = MethodMetadata {3, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValue};
   methodMap_[\\"getValueWithCallback\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithCallback};
   methodMap_[\\"getValueWithPromise\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithPromise};
+  methodMap_[\\"getValueWithOptionalArg\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getValueWithOptionalArg};
   methodMap_[\\"getEnums\\"] = MethodMetadata {3, __hostFunction_NativeSampleTurboModuleCxxSpecJSI_getEnums};
 }
 

diff --git a/...ative-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleH-test.js.snap b/...ative-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleH-test.js.snap
@@ -87,7 +87,7 @@ protected:
 public:
   virtual jsi::Object difficult(jsi::Runtime &rt, jsi::Object A) = 0;
   virtual void optionals(jsi::Runtime &rt, jsi::Object A) = 0;
-  virtual void optionalMethod(jsi::Runtime &rt, jsi::Object options, jsi::Function callback, jsi::Array extras) = 0;
+  virtual void optionalMethod(jsi::Runtime &rt, jsi::Object options, jsi::Function callback, std::optional<jsi::Array> extras) = 0;
   virtual void getArrays(jsi::Runtime &rt, jsi::Object options) = 0;
   virtual std::optional<jsi::Object> getNullableObject(jsi::Runtime &rt) = 0;
   virtual std::optional<jsi::Object> getNullableGenericObject(jsi::Runtime &rt) = 0;
@@ -129,7 +129,7 @@ private:
       return bridging::callFromJs<void>(
           rt, &T::optionals, jsInvoker_, instance_, std::move(A));
     }
-    void optionalMethod(jsi::Runtime &rt, jsi::Object options, jsi::Function callback, jsi::Array extras) override {
+    void optionalMethod(jsi::Runtime &rt, jsi::Object options, jsi::Function callback, std::optional<jsi::Array> extras) override {
       static_assert(
           bridging::getParameterCount(&T::optionalMethod) == 4,
           \\"Expected optionalMethod(...) to have 4 parameters\\");
@@ -668,6 +668,7 @@ public:
   virtual jsi::Object getValue(jsi::Runtime &rt, double x, jsi::String y, jsi::Object z) = 0;
   virtual void getValueWithCallback(jsi::Runtime &rt, jsi::Function callback) = 0;
   virtual jsi::Value getValueWithPromise(jsi::Runtime &rt, bool error) = 0;
+  virtual jsi::Value getValueWithOptionalArg(jsi::Runtime &rt, std::optional<jsi::Object> parameter) = 0;
   virtual jsi::String getEnums(jsi::Runtime &rt, double enumInt, double enumFloat, jsi::String enumString) = 0;
 
 };
@@ -778,6 +779,14 @@ private:
       return bridging::callFromJs<jsi::Value>(
           rt, &T::getValueWithPromise, jsInvoker_, instance_, std::move(error));
     }
+    jsi::Value getValueWithOptionalArg(jsi::Runtime &rt, std::optional<jsi::Object> parameter) override {
+      static_assert(
+          bridging::getParameterCount(&T::getValueWithOptionalArg) == 2,
+          \\"Expected getValueWithOptionalArg(...) to have 2 parameters\\");
+
+      return bridging::callFromJs<jsi::Value>(
+          rt, &T::getValueWithOptionalArg, jsInvoker_, instance_, std::move(parameter));
+    }
     jsi::String getEnums(jsi::Runtime &rt, double enumInt, double enumFloat, jsi::String enumString) override {
       static_assert(
           bridging::getParameterCount(&T::getEnums) == 4,

diff --git a/...codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleHObjCpp-test.js.snap b/...codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleHObjCpp-test.js.snap
@@ -933,6 +933,9 @@ namespace JS {
 - (void)getValueWithPromise:(BOOL)error
                     resolve:(RCTPromiseResolveBlock)resolve
                      reject:(RCTPromiseRejectBlock)reject;
+- (void)getValueWithOptionalArg:(NSDictionary *)parameter
+                        resolve:(RCTPromiseResolveBlock)resolve
+                         reject:(RCTPromiseRejectBlock)reject;
 - (NSString *)getEnums:(double)enumInt
              enumFloat:(double)enumFloat
             enumString:(NSString *)enumString;

diff --git a/...odegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleJavaSpec-test.js.snap b/...odegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleJavaSpec-test.js.snap
@@ -377,6 +377,10 @@ public abstract class NativeSampleTurboModuleSpec extends ReactContextBaseJavaMo
   @DoNotStrip
   public abstract void getValueWithPromise(boolean error, Promise promise);
 
+  @ReactMethod
+  @DoNotStrip
+  public abstract void getValueWithOptionalArg(ReadableMap parameter, Promise promise);
+
   @ReactMethod(isBlockingSynchronousMethod = true)
   @DoNotStrip
   public abstract String getEnums(double enumInt, double enumFloat, String enumString);

diff --git a/...-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleJniCpp-test.js.snap b/...-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleJniCpp-test.js.snap
@@ -384,6 +384,11 @@ static facebook::jsi::Value __hostFunction_NativeSampleTurboModuleSpecJSI_getVal
   return static_cast<JavaTurboModule &>(turboModule).invokeJavaMethod(rt, PromiseKind, \\"getValueWithPromise\\", \\"(ZLcom/facebook/react/bridge/Promise;)V\\", args, count, cachedMethodId);
 }
 
+static facebook::jsi::Value __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithOptionalArg(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) {
+  static jmethodID cachedMethodId = nullptr;
+  return static_cast<JavaTurboModule &>(turboModule).invokeJavaMethod(rt, PromiseKind, \\"getValueWithOptionalArg\\", \\"(Lcom/facebook/react/bridge/ReadableMap;Lcom/facebook/react/bridge/Promise;)V\\", args, count, cachedMethodId);
+}
+
 static facebook::jsi::Value __hostFunction_NativeSampleTurboModuleSpecJSI_getEnums(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) {
   static jmethodID cachedMethodId = nullptr;
   return static_cast<JavaTurboModule &>(turboModule).invokeJavaMethod(rt, StringKind, \\"getEnums\\", \\"(DDLjava/lang/String;)Ljava/lang/String;\\", args, count, cachedMethodId);
@@ -402,6 +407,7 @@ NativeSampleTurboModuleSpecJSI::NativeSampleTurboModuleSpecJSI(const JavaTurboMo
   methodMap_[\\"getValue\\"] = MethodMetadata {3, __hostFunction_NativeSampleTurboModuleSpecJSI_getValue};
   methodMap_[\\"getValueWithCallback\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithCallback};
   methodMap_[\\"getValueWithPromise\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithPromise};
+  methodMap_[\\"getValueWithOptionalArg\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithOptionalArg};
   methodMap_[\\"getEnums\\"] = MethodMetadata {3, __hostFunction_NativeSampleTurboModuleSpecJSI_getEnums};
 }
 

diff --git a/...tive-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleMm-test.js.snap b/...tive-codegen/src/generators/modules/__tests__/__snapshots__/GenerateModuleMm-test.js.snap
@@ -453,6 +453,10 @@ namespace facebook {
       return static_cast<ObjCTurboModule&>(turboModule).invokeObjCMethod(rt, PromiseKind, \\"getValueWithPromise\\", @selector(getValueWithPromise:resolve:reject:), args, count);
     }
 
+    static facebook::jsi::Value __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithOptionalArg(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) {
+      return static_cast<ObjCTurboModule&>(turboModule).invokeObjCMethod(rt, PromiseKind, \\"getValueWithOptionalArg\\", @selector(getValueWithOptionalArg:resolve:reject:), args, count);
+    }
+
     static facebook::jsi::Value __hostFunction_NativeSampleTurboModuleSpecJSI_getEnums(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) {
       return static_cast<ObjCTurboModule&>(turboModule).invokeObjCMethod(rt, StringKind, \\"getEnums\\", @selector(getEnums:enumFloat:enumString:), args, count);
     }
@@ -494,6 +498,9 @@ namespace facebook {
         methodMap_[\\"getValueWithPromise\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithPromise};
         
         
+        methodMap_[\\"getValueWithOptionalArg\\"] = MethodMetadata {1, __hostFunction_NativeSampleTurboModuleSpecJSI_getValueWithOptionalArg};
+        
+        
         methodMap_[\\"getEnums\\"] = MethodMetadata {3, __hostFunction_NativeSampleTurboModuleSpecJSI_getEnums};