-
-
Notifications
You must be signed in to change notification settings - Fork 72
fix: Fix optional doubles not building in Release mode on Swift #1065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This is where we have this case already: Why didn't this Maybe we need to actually access |
…f that fails in release
|
I just added a test for this that passes the following struct: interface PartialPerson {
age?: number
name?: string
}..to native, and native Swift accesses I am running this test two times:
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: 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). |
|
I think there is still issue even we applied nitro patch in #746. Our Test Environment
Our Test ResultsWe tested with the AudioSet configuration in JS: const audioSet = {
AVSampleRateKeyIOS: 22050,
AVNumberOfChannelsKeyIOS: 2,
AudioSamplingRate: 22050,
AudioChannels: 2,
AudioEncodingBitRate: 64000,
};Release build recording result: Expected: 2 channels, 22050 Hz The patch did NOT fix the issue for us in Release builds. Key Difference: Struct ComplexityYour
Our
{
"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
Suggestion for ReproductionCould 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 Repogit 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. |
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
|
|
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. Test Setup:
Result:afinfo output: Key Observation:The generated AudioSet.swift shows mixed patterns:
Our struct has 18 optional properties (vs your PartialPerson test with 2) |
Done in swiftlang/swift#85735. Have nice vacation! |


Fixes a bug mentioned by hyochan/react-native-nitro-sound#746 (comment) where a
number | undefined(std::optional<double>) would not build in Release mode on Swift.The fix is to not use the Swift generated
.valueaccessor, but instead do a.has_value() ? .pointer : nilaccess.The problem is, we use
std::optional<double>in thePersonstruct 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