fix(nitrogen): keep | undefined on array elements and Record values - #1478
Open
giaBaoJS wants to merge 1 commit into
Open
fix(nitrogen): keep | undefined on array elements and Record values#1478giaBaoJS wants to merge 1 commit into
| undefined on array elements and Record values#1478giaBaoJS wants to merge 1 commit into
Conversation
`createType(...)` threads optionality through every nested position except two: the array element type and the `Record<K, V>` value type, which both hardcoded `isOptional = false`. With `isOptional = false`, an element type such as `string | undefined` falls into the union branch, which filters `undefined` out on the assumption that it "is already handled as isOptional" — but for these two positions it never was. The result is that `(string | undefined)[]` silently generated `std::vector<std::string>` instead of `std::vector<std::optional<std::string>>`. Pass `isNullable()` for both, so a nested element is created exactly the same way a parameter, tuple element or struct property of the same type already is. This makes `std::vector<std::optional<T>>` reachable for the first time, which exposed a gap in the Kotlin JNI array bridge: `KotlinCxxBridgedType.dereferenceToJObject(...)` switches on the type's own kind, so an `optional` always took the `*ref` branch. For the five kinds that require `.get()` (array-buffer, function, hybrid-object-base, map, promise) an optional element generated `*__elementJni`, which does not compile. `dereferenceToJObject(...)` now forwards to the wrapping type for optionals. Fixes mrousavy#1202
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1202
Builds on the repro from #1201 by @chrispader — thanks for pinning it down. That PR added the spec + red CI; this one adds the fix and distills the repro to a single method on an existing spec, per CONTRIBUTING.
Root cause
createType(...)threads optionality through every nested type position — except two.isOptionalis computedhasQuestionToken() || isOptional() || type.isNullable()Parameter.ts:28t.isNullable()createType.ts:277prop.isOptional() || propType.isNullable()getInterfaceProperties.ts:35.getUnionTypes().some(t => t.isUndefined())createType.ts:283,:299falsecreateType.ts:272Record<K, V>valuefalsecreateType.ts:311With
isOptional = false, an element type likestring | undefinedreaches the union branch (createType.ts:337), which filtersundefinedout at:363with the comment "already treated asisOptional". For these two positions it never was, so the| undefinedis silently dropped.The fix
Pass
isNullable()for both, so a nested element is built exactly the same way a parameter, tuple element or struct property of the same type already is. Two lines.I deliberately used
isNullable()rather than the narrower.some(t => t.isUndefined()), so thatT[]and a bareTagree. Nitrogen already generatesstd::optional<std::variant<nitro::NullType, std::string>>for a parameter of typestring | null(Parameter.ts:28); withisUndefinedonly, the array element form would have disagreed with the scalar form for that type. Both predicates produce identical output for the type in #1202.Second hunk: Kotlin JNI array bridge
The fix makes
std::vector<std::optional<T>>reachable for the first time, which exposes a real gap.KotlinCxxBridgedType.dereferenceToJObject(...)switches on the type's own kind:An
optionalnever matches those five, so it always fell to*ref— even when the wrapped type needs.get(). This is a compile error, not a runtime issue. Verified by reverting just this hunk and running the Android build:dereferenceToJObject(...)now forwards to the wrapping type for optionals.std::optional<std::string>was already fine either way — fbjni'soperator*on a nulllocal_refis well defined (ReprStorage::setalways placement-news aReprholding a possibly-nulljobject,References-inl.h:66-80), so it lands as a Javanullin the array.Test
One method on the existing
SharedTestObjectProps, reusing existing types — no new structs, enums or spec files:Each argument is load-bearing:
stringspins the array-element fix (and the return exercises the Kotlin→C++ direction),mappins theRecordvalue fix,arrayBufferspins thedereferenceToJObjecthunk — without it the Android build passes and the JNI gap ships unnoticed.This is a compile-time test (the bug is codegen), so it lives in the specs and is covered by
build-ios.yml/build-android.yml.getTests.tsis untouched.Generated output — before / after
C++ (
HybridTestObjectSwiftKotlinSpec.hpp)Swift (
HybridTestObjectSwiftKotlinSpec.swift)Kotlin (
HybridTestObjectSwiftKotlinSpec.kt)Kotlin JNI bridge (
JHybridTestObjectSwiftKotlinSpec.cpp), thearrayBuffersloop:Verification
Everything below was actually run on this branch (macOS, Xcode 26.2, NDK 27.1, JDK 17):
bun specsinreact-native-nitro-test-externalandreact-native-nitro-testregenerates a tree byte-identical tomain(git diff --exit-codeclean over bothnitrogen/generatedtrees). No existing spec changes shape.createType.tsand regenerating puts the buggystd::vector<std::string>/std::unordered_map<std::string, std::string>signature back; restoring it returns the optional form. Reverting onlyKotlinCxxBridgedType.tsbreaks the Android build with the error quoted above.run-nitrogen.ymldoes):bun install→bun run build→bun specsin both test packages →git diff --exit-code— clean.xcodebuild -workspace NitroExample.xcworkspace -scheme NitroExample -sdk iphonesimulator→** BUILD SUCCEEDED **. This compiles the generated Swift↔C++ bridge, includingstd::vector<std::optional<std::string>>andstd::optional<std::shared_ptr<ArrayBuffer>>round-trips../gradlew :react-native-nitro-test:assembleDebug -PreactNativeArchitectures=arm64-v8a→BUILD SUCCESSFUL. This compiles the JNI bridge above.bun typecheck,bun lint,bun lint-cpp,bun lint-swift,bun lint-kotlinall pass.Not verified: I did not run the Harness runtime tests on a device/emulator, and the Android build was arm64-v8a only. There is no runtime assertion in this PR — the bug is a codegen shape mismatch, which the native builds pin.