nitro icon indicating copy to clipboard operation
nitro copied to clipboard

fix: Fix optional doubles not building in Release mode on Swift

Open mrousavy opened this issue 1 month ago • 8 comments

Fixes a bug mentioned by https://github.com/hyochan/react-native-nitro-sound/pull/746#discussion_r2571562926 where a number | undefined (std::optional<double>) would not build in Release mode on Swift.

The fix is to not use the Swift generated .value accessor, but instead do a .has_value() ? .pointer : nil access.

The problem is, we use std::optional<double> in the Person struct in the example/test module, and it builds just fine in Release mode.

So this cannot be blindly merged until we have an actual test in this repo demonstrating this exact problem. cc @hyochan let me know if you find something

mrousavy avatar Nov 28 '25 13:11 mrousavy

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
nitro-docs Skipped Skipped Nov 28, 2025 1:28pm

vercel[bot] avatar Nov 28 '25 13:11 vercel[bot]

This is where we have this case already: image

Why didn't this .value access fail in Release mode before? All release builds in CI went through just fine.

Maybe we need to actually access .age on the native Swift side for it to use the symbol? Not sure, but I can definitely use some help here to add a test to prevent future regressions. I'm on vacation til monday

mrousavy avatar Nov 28 '25 13:11 mrousavy

I just added a test for this that passes the following struct:

interface PartialPerson {
  age?: number
  name?: string
}

..to native, and native Swift accesses .age and returns it to JS again.

I am running this test two times:

  • with age being undefined (in which case undefined should be returned to JS)
  • with age being 57 (in which case the number 57 should be returned to JS).

I built this in release, once with the code before:

  var age: Double? {
    @inline(__always)
    get {
      return self.__age.value
    }

..and once with the code after the change suggested by @hyochan in the thread over at his repo:

  var age: Double? {
    @inline(__always)
    get {
      return self.__age.hasValue ? self.__age.pointee : nil
    }

In both cases, the tests run fine without any corruption or errors in release mode: image

So we need another way to actually reproduce this. Are you guys maybe on an old Xcode version? I'm on Version 26.1.1 (17B100).

mrousavy avatar Nov 28 '25 13:11 mrousavy

I think there is still issue even we applied nitro patch in #746.

Our Test Environment

  • Xcode: 26.1.1 (Build 17B100) - Same as yours!
  • React Native: 0.82.1
  • react-native-nitro-modules: 0.31.10
  • macOS: Darwin 24.6.0

Our Test Results

We tested with the .has_value() ? .pointee : nil patch applied to our AudioSet.swift.

AudioSet configuration in JS:

const audioSet = {
  AVSampleRateKeyIOS: 22050,
  AVNumberOfChannelsKeyIOS: 2,
  AudioSamplingRate: 22050,
  AudioChannels: 2,
  AudioEncodingBitRate: 64000,
};

Release build recording result:

Data format: 1 ch, 8000 Hz, Int16

Expected: 2 channels, 22050 Hz Actual: 1 channel, 8000 Hz (fallback defaults)

The patch did NOT fix the issue for us in Release builds.

Key Difference: Struct Complexity

Your PartialPerson test struct:

  • 2 optional properties (age?: number, name?: string)
  • Simple round-trip test: JS → Native → JS

Our AudioSet struct:

  • 18 optional properties (multiple double?, enum types, boolean?)
  • Struct is passed to native and values are consumed internally (not returned to JS)
  • Multiple optional properties accessed in sequence within native code
{
  "AudioSet": {
    "AudioEncoderAndroid": "AudioEncoderAndroidType?",
    "OutputFormatAndroid": "OutputFormatAndroidType?",
    "AudioSourceAndroid": "AudioSourceAndroidType?",
    "AVEncoderAudioQualityKeyIOS": "AVEncoderAudioQualityIOSType?",
    "AVModeIOS": "AVModeIOSOption?",
    "AVEncodingOptionIOS": "AVEncodingOption?",
    "AVFormatIDKeyIOS": "AVEncodingOption?",
    "AVNumberOfChannelsKeyIOS": "double?",
    "AVSampleRateKeyIOS": "double?",
    "AVLinearPCMBitDepthKeyIOS": "AVLinearPCMBitDepthKeyIOSType?",
    "AVLinearPCMIsBigEndianKeyIOS": "boolean?",
    "AVLinearPCMIsFloatKeyIOS": "boolean?",
    "AVLinearPCMIsNonInterleavedIOS": "boolean?",
    "AudioQuality": "AudioQualityType?",
    "AudioChannels": "double?",
    "AudioSamplingRate": "double?",
    "AudioEncodingBitRate": "double?",
    "IncludeBase64": "boolean?"
  }
}

Possible Causes

  1. Struct size/complexity: The bug may only manifest with larger structs (18 vs 2 optional properties)
  2. Memory layout: Swift compiler optimization may corrupt memory when many optionals exist in one struct
  3. Property access pattern: Accessing multiple optional properties in sequence within native code (not round-trip)
  4. Mixed optional types: Our struct has double?, enum?, and boolean? mixed together

Suggestion for Reproduction

Could you try a more complex struct similar to ours?

interface ComplexStruct {
  prop1?: number  // double?
  prop2?: number
  prop3?: number
  prop4?: number
  prop5?: SomeEnum  // enum type
  prop6?: SomeEnum
  prop7?: number
  prop8?: number
  prop9?: boolean
  prop10?: boolean
  prop11?: number
  prop12?: number
}

And access multiple properties in sequence on the native side (not just one property round-trip).

How to Reproduce with Our Repo

git clone https://github.com/hyochan/react-native-nitro-sound.git
cd react-native-nitro-sound
yarn install
cd example
yarn ios:pod
yarn build:ios  # Release build
# Run on simulator, go to "NitroSound with Hook" screen, record audio
# Check recorded file with: afinfo <path_to_m4a_file>

The recorded file should show 22050 Hz, 2 channels if working correctly. In Release build, it shows 8000 Hz, 1 channel (defaults).

hyochan avatar Nov 28 '25 13:11 hyochan

Could you try a more complex struct similar to ours?

Can you try this? Fork this repo and just edit the structs - it's quite easy to build the example app.

I'm on vacation now

~~which Xcode version are you using?~~ EDIT: my bad, i didn't see - you are using the same version hmmm

mrousavy avatar Nov 28 '25 17:11 mrousavy

I think if you can reproduce this you should probably create a bug report in the swiftlang/swift repo.

mrousavy avatar Nov 28 '25 17:11 mrousavy

I think if you can reproduce this you should probably create a bug report in the swiftlang/swift repo.

I've tested current PR directly in our react-native-nitro-sound repo. Here are the results:

Test Setup:

  • Installed nitrogen from PR #1065 branch via npm pack
  • Ran yarn prepare to regenerate Swift files
  • Built iOS Release for simulator

Result:

afinfo output:

Data format: 1 ch, 8000 Hz, Int16
Expected: 2 ch, 22050 Hz

Key Observation:

The generated AudioSet.swift shows mixed patterns:

  • Some properties use .hasValue (PR #1065 fix): return self.__AVSampleRateKeyIOS.hasValue ? self.__AVSampleRateKeyIOS.pointee : nil
  • Some properties still use .has_value() (old pattern): return self.__AVEncoderAudioQualityKeyIOS.has_value() ? self.__AVEncoderAudioQualityKeyIOS.pointee : nil
  • Bool properties use bridge helper functions: if bridge.has_value_std__optional_bool_(self.__AVLinearPCMIsBigEndianKeyIOS) { ... }

Our struct has 18 optional properties (vs your PartialPerson test with 2)

hyochan avatar Nov 29 '25 04:11 hyochan

I think if you can reproduce this you should probably create a bug report in the swiftlang/swift repo.

Done in https://github.com/swiftlang/swift/issues/85735. Have nice vacation!

hyochan avatar Nov 29 '25 04:11 hyochan