Fix off-by-one error in cxx codegen (#36574)

Summary: Pull Request resolved: #36574 We would previously generate the following codegen for optional args ``` return static_cast<NativeAudioModuleCxxSpecJSI *>(&turboModule)->playAudio( rt, args[0].asString(rt), args[1].isNull() || args[1].isUndefined() ? std::nullopt : std::make_optional(args[1].asString(rt)), args[2].isNull() || args[2].isUndefined() ? std::nullopt : std::make_optional(args[2].asString(rt)), args[3].asNumber(), count < 4 || args[4].isNull() || args[4].isUndefined() ? std::nullopt : std::make_optional(args[4].asObject(rt)), count < 5 || args[5].isNull() || args[5].isUndefined() ? std::nullopt : std::make_optional(args[5].asObject(rt)), count < 6 || args[6].isNull() || args[6].isUndefined() ? std::nullopt : std::make_optional(args[6].asBool()) ); ``` However, the counts checked are off-by-one, causing us to incorrectly process args. Changelog: [General][Fixed] Issue with TurboModule C++ codegen with optional args Differential Revision: D44299193 fbshipit-source-id: f00b9f5e09c2f524f9393137346c256d8b6b2979
facebook · Mar 22, 2023 · 0a8164d · 0a8164d
1 parent 4a15f90
commit 0a8164d
Show file tree

Hide file tree

Showing 7 changed files with 981 additions and 243 deletions.
diff --git a/...s/react-native-codegen/e2e/__tests__/modules/__snapshots__/GenerateModuleCpp-test.js.snap b/...s/react-native-codegen/e2e/__tests__/modules/__snapshots__/GenerateModuleCpp-test.js.snap
diff --git a/packages/react-native-codegen/src/generators/modules/GenerateModuleCpp.js b/packages/react-native-codegen/src/generators/modules/GenerateModuleCpp.js
@@ -40,8 +40,8 @@ const HostFunctionTemplate = ({
 }>) => {
   const isNullable = returnTypeAnnotation.type === 'NullableTypeAnnotation';
   const isVoid = returnTypeAnnotation.type === 'VoidTypeAnnotation';
-  const methodCallArgs = ['rt', ...args].join(', ');
-  const methodCall = `static_cast<${hasteModuleName}CxxSpecJSI *>(&turboModule)->${methodName}(${methodCallArgs})`;
+  const methodCallArgs = ['    rt', ...args].join(',\n    ');
+  const methodCall = `static_cast<${hasteModuleName}CxxSpecJSI *>(&turboModule)->${methodName}(\n${methodCallArgs}\n  )`;
 
   return `static jsi::Value __hostFunction_${hasteModuleName}CxxSpecJSI_${methodName}(jsi::Runtime &rt, TurboModule &turboModule, const jsi::Value* args, size_t count) {${
     isVoid
@@ -139,7 +139,7 @@ function serializeArg(
     } else {
       let condition = `${val}.isNull() || ${val}.isUndefined()`;
       if (optional) {
-        condition = `count < ${index} || ${condition}`;
+        condition = `count <= ${index} || ${condition}`;
       }
       return `${condition} ? std::nullopt : std::make_optional(${expression})`;
     }

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
diff --git a/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp b/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp
@@ -117,6 +117,12 @@ AsyncPromise<std::string> NativeCxxModuleExample::getValueWithPromise(
   return promise;
 }
 
+bool NativeCxxModuleExample::getWithWithOptionalArgs(
+    jsi::Runtime &rt,
+    std::optional<bool> optionalArg) {
+  return optionalArg.value_or(false);
+}
+
 void NativeCxxModuleExample::voidFunc(jsi::Runtime &rt) {
   // Nothing to do
 }

diff --git a/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.h b/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.h
@@ -120,6 +120,10 @@ class NativeCxxModuleExample
 
   AsyncPromise<std::string> getValueWithPromise(jsi::Runtime &rt, bool error);
 
+  bool getWithWithOptionalArgs(
+      jsi::Runtime &rt,
+      std::optional<bool> optionalArg);
+
   void voidFunc(jsi::Runtime &rt);
 
   void emitCustomDeviceEvent(jsi::Runtime &rt, jsi::String eventName);

diff --git a/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.js b/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.js
@@ -70,6 +70,7 @@ export interface Spec extends TurboModule {
   +getValue: (x: number, y: string, z: ObjectStruct) => ValueStruct;
   +getValueWithCallback: (callback: (value: string) => void) => void;
   +getValueWithPromise: (error: boolean) => Promise<string>;
+  +getWithWithOptionalArgs: (optionalArg?: boolean) => boolean;
   +voidFunc: () => void;
   +emitCustomDeviceEvent: (eventName: string) => void;
 }

diff --git a/packages/rn-tester/js/examples/TurboModule/NativeCxxModuleExampleExample.js b/packages/rn-tester/js/examples/TurboModule/NativeCxxModuleExampleExample.js
@@ -55,6 +55,7 @@ type Examples =
   | 'promise'
   | 'rejectPromise'
   | 'voidFunc'
+  | 'optionalArgs'
   | 'emitDeviceEvent';
 
 class NativeCxxModuleExampleExample extends React.Component<{||}, State> {
@@ -99,6 +100,7 @@ class NativeCxxModuleExampleExample extends React.Component<{||}, State> {
         .then(() => {})
         .catch(e => this._setResult('rejectPromise', e.message)),
     voidFunc: () => NativeCxxModuleExample?.voidFunc(),
+    optionalArgs: () => NativeCxxModuleExample?.getWithWithOptionalArgs(),
     emitDeviceEvent: () => {
       const CUSTOM_EVENT_TYPE = 'myCustomDeviceEvent';
       DeviceEventEmitter.removeAllListeners(CUSTOM_EVENT_TYPE);