fix(nitrogen): look up Kotlin callbacks with a boxed return type - #1591
fix(nitrogen): look up Kotlin callbacks with a boxed return type#1591giaBaoJS wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
We should avoid boxing for performance reasons. Maybe we can add a (private?) method called |
|
Probably this PR is fine to be merged at first, and then a follow-up PR to make it faster (with the |
|
I am also not sure if that even makes things faster, I think that needs to actually be benchmarked. Not sure what the JVM does under the hood in this case. |
A generated `fun interface Func_X: (..) -> R` overrides `FunctionN.invoke`, whose return type is a generic, so kotlinc keeps the boxed type in the JVM signature. Parameters are specialized to primitives, return types are not. The generated JNI lookup used the primitive signature (`()D` for `() => number`), which does not exist on the class, so the first C++ call into a Kotlin-implemented callback that returns a value throws NoSuchMethodError.
6e4d9c8 to
30bff40
Compare
|
Yes, I can do that. This PR stays as the correctness fix, and I will open a separate PR for On your third point, I did not want to assume boxing is expensive, so I measured what the JVM does with it. Method: OpenJDK 17 on arm64 macOS, an interface returning Two things come out of that:
So the ceiling for One thing I ran into while looking at it: Rebased on main, this is mergeable again. |
What breaks
packages/nitrogen/src/syntax/kotlin/KotlinFunction.ts:142builds the JNI signature of the generatedJFunc_X::invoke()from the plain JNI type of the callback's return type. ForSync<() => number>that isdouble():The Kotlin half of the same callback is a
fun interfacethat extends the Kotlin function type:invokeoverridesFunction0<R>.invoke, and kotlinc cannot specialize a generic return type to a JVM primitive, so it keeps the boxed type. Parameters are specialized (kotlinc adds a(Ljava/lang/Object;)Ljava/lang/Object;bridge for them), return types are not.fbjni resolves methods by exact descriptor, so the Android build does not catch this.
GetMethodIDfinds nothing and the call throwsNoSuchMethodError.Ground truth
kotlinc 2.4.10 +
javap -p -s, on the exactfun interfaceshape nitrogen generates:fun interface F: () -> Doubleinvoke()Ljava/lang/Double;()Dfun interface F: () -> Booleaninvoke()Ljava/lang/Boolean;()Zfun interface F: () -> Longinvoke()Ljava/lang/Long;()Jfun interface F: () -> Unitinvoke()V()V(already correct)fun interface F: (Double) -> Unitinvoke(D)V(D)V(already correct)fun interface F: () -> Stringinvoke()Ljava/lang/String;Unitis the exception: it maps tovoid, so a void callback was fine. Every other primitive return was broken.When it fires
Only when the callback was implemented in Kotlin. A callback that came from JS is a
Func_X_cxx, andKotlinCxxBridgedTypeunwraps that viagetFunction()without ever touching JNI. Anything else falls intoJNICallable<JFunc_X, R(Args...)>, which callsJFunc_X::invoke().That is why CI never hit it:
react-native-nitro-testusedFunc_doubleonly as a parameter (callbackSync), never as a value handed from Kotlin to C++.iOS is unaffected, Swift closures do not go through this path.
Fix
KotlinFunction.tsnow boxes the return type of theinvokelookup for the primitive kinds, and unboxes__resultwith the existingparseFromKotlinToCpp(.., isBoxed)machinery:std::optional<double>returns already emittedjni::local_ref<jni::JDouble>, so they were correct before and are untouched.Test
One method on
SharedTestObjectProps, in the "Sync funcs" block next to the existingcallbackSync:plus one assertion in
example/src/getTests.ts, so the Harness workflows cover it on both platforms. It reuses theFunc_doublespecialization thatcallbackSyncalready generates, this time in the return direction, which is the direction that was broken.How I proved it
I compiled the generated Kotlin (
nitrogen/generated/android/kotlin, minus the ViewManagers, which need React Native on the classpath) with kotlinc, then checked everygetField/getMethodlookup innitrogen/generated/android/c++againstjavap -p -son the resulting class files.Before the fix, one lookup did not exist:
Resolving that descriptor the way
GetMethodIDdoes:After the fix, all 339 JNI names and descriptors emitted for
react-native-nitro-testmatch the compiler, with no exceptions:I also checked the fix is not vacuous by mutating it in the other direction (boxing to
jni::JLonginstead ofjni::JDouble), which the same check catches as()Ljava/lang/Long;vs()Ljava/lang/Double;.Not covered
UInt64callbacks are broken in a different way and are out of scope here:ULongis a value class, so kotlinc mangles the whole method name (invoke-s-VKNKUfor aULongreturn,invoke-VKZWuLQfor aULongparameter). Boxing does not help there, so I left that path exactly as it was. Happy to open a separate issue for it.Checks
bun specsrun, generated files committed. Apart from the new method, the only generated change isJFunc_double.hppand some include reordering.bun run build,bun typecheck,bun lint,bun lint-cpp,bun lint-swift,bun lint-kotlin: all clean, no files changed by the formatters.