Skip to content

Conversation

@moskalakamil
Copy link

fixes a bug where a std::optional<double> (used to represent number | undefined) would not work correctly in Release mode due to a Swift C++ interop issue.

Even though std::optional<double> is an optional type, it is not always detected by needsSpecialHandling, which caused the generated bridging to follow the generic path. This worked in Debug, but Release builds triggered a Swift interop bug causing the optional number to be interpreted incorrectly.

A previous attempt to fix this (see #1065) introduced the following pattern:

has_value() ? .pointer : nil

However, this did not address the issue - the Swift C++ interop layer still failed in Release mode.

A reproducible example of the failing case is here:
moskalakamil@bef989c

Instead of using .pointer, this PR switches to:

if bridge.has_value_${bridge.specializationName}(${cppParameterName}) {
    let __unwrapped = bridge.get_${bridge.specializationName}(${cppParameterName})
    return ${indent(wrapping.parseFromCppToSwift('__unwrapped', language), '    ')}
} else {
    return nil
}
}()

cc @hyochan

@vercel
Copy link

vercel bot commented Dec 10, 2025

@moskalakamil is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

@mrousavy
Copy link
Owner

Thanks for the fix - a few notes;

  1. CI is failing - you didn't lint properly. Run bun lint in the root
  2. Is there a reason why you put this into the react-native-nitro-test-external package? I would prefer to keep this in react-native-nitro-test, unless it specifically only happens when using an external type from another package

@moskalakamil
Copy link
Author

in my case, the bug only appeared when two nitro packages were linked together. Building each package individually in release mode worked fine, but once they depended on each other the issue surfaced immediately. This is very similar to what happened in PR #1065; when you added optional number handling there, the problem did not show up because the package was isolated. Not sure how it looks in react-native-nitro-sound (cc again @hyochan)

@mrousavy
Copy link
Owner

Since it happens in react-native-nitro-sound and @hyochan does not use external dependencies (as far as I know?) i am not fully sure if this is a deterministic way to test this.

I'll build a release version of this tomorrow and check if it can be reliably reproduced, then we'll see

@moskalakamil moskalakamil force-pushed the fix/nitrogen/ios-optional-number-release-build branch from 69a9ee3 to bb06e3a Compare December 11, 2025 09:44
@moskalakamil moskalakamil force-pushed the fix/nitrogen/ios-optional-number-release-build branch from bb06e3a to 68e2856 Compare December 11, 2025 18:15
@moskalakamil
Copy link
Author

hey @mrousavy, have you had a chance to take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants