Code generation for callback support in Kotlin #629

emarteca · 2024-08-14T01:51:50Z

Code generation following the pattern described in this section of the design doc for callback support.

emarteca · 2024-08-14T18:20:27Z

@jcrist1

Manishearth · 2024-08-14T18:25:04Z

I plan to review this soonish, but @jcrist1 should let me know what their expectations are for their own review: do you want us to wait on landing this until you've also reviewed, or okay with landing with just my review, or some other thing?

(keeping in mind i understand very little about Kotlin)

jcrist1 · 2024-08-14T18:31:25Z

Skimming over it quickly it doesn't look like it will take long to review. I'd like to try and do a review tonight still. But if I don't get one in by tomorrow feel free to move forward with it.

Manishearth · 2024-08-14T18:32:45Z

sounds good, seems like we have roughly the same timeline for reviewing this. No need to rush if you can't get to it, I mostly wanted to make sure we communicated review needs and timelines

jcrist1

First it looks like the code gen is broken but that has nothing to do with your PR. I'll look into it in a bit to see how much effort it would be to fix.

Otherwise it looks super cool! I manually cleaned up the generated kotlin code to get it compiling, and created a little test:

package dev.diplomattest.somelib

import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test

class CallbackTest {
    @Test
    fun testCallback() {
        val cb: (Int ) -> Int = { i -> i + 2 }
        val calledBack = CallbackWrapper.testMultiArgCallback(
            DiplomatCallback_CallbackWrapper_test_multi_arg_callback_diplomatCallback_f.fromCallback(cb),
            10
        )
        assertEquals(22, calledBack)
    }
}

which passed. It would be nice to add it to the ~~main~~ test code so in principle it would be tested with cargo make test-kotlin-feature.

I think, however, it highlights how it is a bit unergonomic in its current form. It would be nice if the CallbackWrapper.testMultiArgCallback method could directly take the cb without having to wrap it. WDYT? I don't think this would be that difficult to change as you could just move the kotlin code gen around a little bit.

I'll try to have a more careful look at the generated jna code tomorrow, but would also be happy if we just test out a bunch of the generated callback consuming methods.

jcrist1

Actually had a couple of code comments that I thought I had saved, but hadn't. So quickly added them.

tool/templates/kotlin/Struct.kt.jinja

tool/src/kotlin/snapshots/diplomat_tool__kotlin__test__struct.snap

tool/templates/kotlin/Struct.kt.jinja

feature_tests/kotlin/somelib/src/main/kotlin/dev/diplomattest/somelib/CallbackWrapper.kt

tool/templates/kotlin/Struct.kt.jinja

feature_tests/kotlin/somelib/src/main/kotlin/dev/diplomattest/somelib/CallbackWrapper.kt

tool/src/kotlin/mod.rs

feature_tests/kotlin/somelib/src/main/kotlin/dev/diplomattest/somelib/CallbackWrapper.kt

Manishearth · 2024-08-14T20:23:55Z

feature_tests/kotlin/somelib/src/main/kotlin/dev/diplomattest/somelib/CallbackWrapper.kt

+                }
+            }
+            val cb_wrap = DiplomatCallback_CallbackWrapper_test_multiple_cb_args_diplomatCallback_g_Native()
+            cb_wrap.run_callback = callback;


question: where is the data/dtor getting set here?

In the previous line it's calling the internal constructor on the Native callback wrapper, which has the default value for run_callback, and a 0 pointer for dataa. The destructor is also set to a 0 pointer -- it won't do anything on the Rust side, since I think we decided Kotlin will do all its memory management for the callback structs. The direct assignment to run_callback here is overwriting the default value for this field.

so wait, how does Rust tell Kotlin that it's no longer holding on to the callback?

I might be missing something in the design here.

So I wasn't paying that much attention to the other PR but the callback is expanded into

#[no_mangle] extern "C" fn CallbackWrapper_test_multiple_cb_args( f: DiplomatCallback<i32>, g: DiplomatCallback<i32>, ) -> i32 { let f = move || unsafe { std::mem::transmute::< unsafe extern "C" fn(*const c_void, ...) -> i32, unsafe extern "C" fn(*const c_void) -> i32, >(f.run_callback)(f.data) }; let g = move |arg0: i32| unsafe { std::mem::transmute::< unsafe extern "C" fn(*const c_void, ...) -> i32, unsafe extern "C" fn(*const c_void, i32) -> i32, >(g.run_callback)(g.data, arg0) }; CallbackWrapper::test_multiple_cb_args(f, g) }

So the impl is only ever monomorphised into the closure g. As I understand it g.data and g.run_callback are copied into the closure and the closure is dropped after the call. But because they aren't owned pointers nothing is done to the backing data. And Kotlin will always wait until the method CallbackWrapper.testMultipleCbArgs is finished before it can GC the callback.

I expect there would be a problem if a struct can have a callback as a field, or if we could wrap a callback in an opaque but this at least fails to compile because of no lifetime bounds on the impl Fn:

#[diplomat::opaque] struct BadThing(Box<dyn Fn(i32) -> i32>); impl CallbackWrapper { ... pub fn test_opaque_callback(f: impl Fn(i32) -> i32) -> Box<BadThing> { Box::new(BadThing(Box::new(f))) } }

But if we want callbacks as params or in opaques, then we need lifetime bounds afaics, and to add the callback struct to the edges of the the object.

Good points -- indeed (as discussed offline too) we need to have Rust store a GC root to the callback wrapper object so that the JVM doesn't get rid of it until it's dropped on the Rust side too. We'll do this in a subsequent PR, but basically the mechanism will be to have the data field store a boxed JNI GlobalRef that references the callback wrapper object. Then in the Drop, Rust will drop the GlobalRef.

emarteca · 2024-08-15T03:08:20Z

Thanks for the comments! I've pushed some fixes, the main one being refactoring so that a user can directly pass the callback in and the .fromCallback is called internally. I still need to fix the tests to reflect this change, and to add support for callbacks in methods on other custom types (enums, opaques).

emarteca · 2024-08-15T18:07:34Z

@jcrist1 to run the tests, should it just be cargo make gen-kotlin-feature && cargo make test-kotlin-feature ? This is still failing for me. The first error is because the duckscript is trying to copy a dylib file which is not generated, and when I change dylib to so (since this is generated), the cargo make test-kotlin-feature fails with many Kotlin compilation errors.

tool/src/kotlin/mod.rs

jcrist1 · 2024-08-15T18:55:42Z

@jcrist1 to run the tests, should it just be cargo make gen-kotlin-feature && cargo make test-kotlin-feature ? This is still failing for me. The first error is because the duckscript is trying to copy a dylib file which is not generated, and when I change dylib to so (since this is generated), the cargo make test-kotlin-feature fails with many Kotlin compilation errors.

dylib to so makes sense. I think the cargo make test-kotlin-feature doesn't run on CI so that wouldn't have been caught, and I only wrote it thinking about my mac.

As for the compile error did you merge the latest main? I had to fix some of the kotlin code gen that fell out of sync in #631 If not, then give that a try. Regardless copy the output of the error, and I'll see if I can figure out why it's failing

jcrist1 · 2024-08-15T18:56:16Z

@emarteca pinging, cause I neglected to ping in the actual message ☝️

Manishearth · 2024-08-15T18:58:28Z

@jcrist1 yeah, we really should get that CI setup

jcrist1 · 2024-08-15T19:16:45Z

@Manishearth I can look into adding a test-kotlin tomorrow to the ci step. Would need to install java and gradle, but it looks pretty straightforward. https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-java-with-gradle.

emarteca · 2024-08-15T21:25:06Z

@jcrist1 thanks for taking a look! Here's the errors: this is after running cargo make gen-kotlin-feature && cargo make test-kotlin-feature: https://pastebin.com/EUEAegEu

I did merge the newest updates from main first! Also FWIW I am using linux, so maybe there is an OS difference causing these issues

emarteca · 2024-08-15T22:49:01Z

I just pushed callback param support for opaques and enums -- the only thing left (I think) is to fix the tests!
I'm going to make a separate PR with the proper drop setup.

jcrist1 · 2024-08-16T07:28:23Z

@jcrist1 thanks for taking a look! Here's the errors: this is after running cargo make gen-kotlin-feature && cargo make test-kotlin-feature: https://pastebin.com/EUEAegEu

I did merge the newest updates from main first! Also FWIW I am using linux, so maybe there is an OS difference causing these issues

@emarteca can you paste the output of gradle --version? I'm expecting an incorrect java version. I tested on java 19.

jcrist1 · 2024-08-16T10:02:58Z

I've got a linux backed ci pipeline up here: #638

emarteca · 2024-08-16T16:41:21Z

@jcrist1 thanks for taking a look! Here's the errors: this is after running cargo make gen-kotlin-feature && cargo make test-kotlin-feature: https://pastebin.com/EUEAegEu
I did merge the newest updates from main first! Also FWIW I am using linux, so maybe there is an OS difference causing these issues

@emarteca can you paste the output of gradle --version? I'm expecting an incorrect java version. I tested on java 19.

Thanks! Yes, you're right -- I'm using Java 17, and gradle 8.3. The output of gradle --version is below:

------------------------------------------------------------
Gradle 8.3
------------------------------------------------------------

Build time:   2023-08-17 07:06:47 UTC
Revision:     8afbf24b469158b714b36e84c6f4d4976c86fcd5

Kotlin:       1.9.0
Groovy:       3.0.17
Ant:          Apache Ant(TM) version 1.10.13 compiled on January 4 2023
JVM:          17.0.10 (Debian 17.0.10+7-Debian-1)
OS:           Linux 6.7.12-1rodete1-amd64 amd64

I'll get back to you on whether we need to get this working on Java 17 or if we can upgrade.

jcrist1 · 2024-08-19T06:22:47Z

@jcrist1 thanks for taking a look! Here's the errors: this is after running cargo make gen-kotlin-feature && cargo make test-kotlin-feature: https://pastebin.com/EUEAegEu
I did merge the newest updates from main first! Also FWIW I am using linux, so maybe there is an OS difference causing these issues

@emarteca can you paste the output of gradle --version? I'm expecting an incorrect java version. I tested on java 19.

Thanks! Yes, you're right -- I'm using Java 17, and gradle 8.3. The output of gradle --version is below:
------------------------------------------------------------
Gradle 8.3
------------------------------------------------------------

Build time:   2023-08-17 07:06:47 UTC
Revision:     8afbf24b469158b714b36e84c6f4d4976c86fcd5

Kotlin:       1.9.0
Groovy:       3.0.17
Ant:          Apache Ant(TM) version 1.10.13 compiled on January 4 2023
JVM:          17.0.10 (Debian 17.0.10+7-Debian-1)
OS:           Linux 6.7.12-1rodete1-amd64 amd64
I'll get back to you on whether we need to get this working on Java 17 or if we can upgrade.

I managed to test with Java 17 in IntelliJ, and there were a couple of panama imports that I apparently have in one one test, that broke the compile. But it managed to compile once I deleted the imports. I can have a look at downgrading the supported java version.

jcrist1 · 2024-08-19T06:23:10Z

and once again cause I never remember to ping: @emarteca ☝️

emarteca · 2024-08-19T17:46:01Z

@jcrist1 thanks!! It turns out I do need to keep this compatible with Java 17. Does the test work in newer Java without the imports?
I think ideally, we'd be able to support various versions of Java; but if we're going to pick one it probably makes sense to stick to the newer version. If you give me the hack to fix the build for old Java then I can test it manually, and just make sure the callback tests work on your end with the newer Java?

jcrist1 · 2024-08-19T18:24:00Z

@emarteca there should be no issue with JVM 19 support if this works with JVM 17. It was just some accidental code that was I think I accidentally added when I started working on #499.

jcrist1 · 2024-08-19T18:24:49Z

I pushed some changes to #638 that should make it compatible with JVM 17

Manishearth

overall lgtm, please let me know when this is in a mergeable state

I believe dtors still need to be fixed but that is going to be done later

jcrist1 · 2024-08-19T19:28:21Z

@emarteca the kotlin ci changes that should support jvm 17 were added. Can you try merging main and see if it works?

emarteca · 2024-08-19T20:54:09Z

Thanks @jcrist1 , it does compile now!! 🙏 I have to fix a bug that cropped up (the generated callback code is showing up in all of the Kotlin files, not just the file for the struct that includes the callbacks), and then I'll ping you!

emarteca · 2024-08-19T22:49:32Z

feature_tests/kotlin/somelib/src/test/kotlin/dev/diplomattest/somelib/CallbackWrapperTest.kt

+        val cb: (CallbackTestingStruct) -> Int = { s -> s.x + s.y}
+        val calledBack = CallbackWrapper.testCbWithStruct(cb)


@jcrist1 this test is broken with the error java.lang.IllegalArgumentException: Callback argument class dev.diplomattest.somelib.CallbackTestingStruct requires custom type conversion -- do you know how to fix this? Is this a bug, or is this a Kotlin feature?

emarteca · 2024-08-19T22:51:44Z

@jcrist1 my code gen is working again, and I made a feature_test test for it! The only thing is that the test for the callback that takes a custom struct is broken -- could you maybe take a look, I am not well versed in Kotlin! (this comment points to the broken test: linked)

jcrist1 · 2024-08-20T04:06:23Z

I am not well versed in Kotlin!

😂 sure I can try, but it's actually the same story for me (my background is scala).

Will have a look tonight CEST.

emarteca · 2024-08-20T17:02:08Z

I am not well versed in Kotlin!

😂 sure I can try, but it's actually the same story for me (my background is scala).

ahah i had thought you were an expert! But no worries, I'll look at it again today too :-)

Manishearth · 2024-08-20T17:04:37Z

For passing the failing tests, rerun tests in that crate and then run cargo insta accept (you'll need to install insta)

You may need to do this multiple times

Manishearth · 2024-08-20T17:05:04Z

(I forget if you'd figured out how to do this already, but in case this is the first time you're seeing this, worth explaining)

Manishearth · 2024-08-20T17:05:11Z

I think this is more or less ready to land.

emarteca · 2024-08-20T17:30:02Z

For passing the failing tests, rerun tests in that crate and then run cargo insta accept (you'll need to install insta)

You may need to do this multiple times

oh thanks, I didn't know about this

emarteca · 2024-08-20T17:31:21Z

I think this is more or less ready to land.

sweet! that sounds good -- i think there might be an issue with the struct argument callbacks (that's the type conversion message i was debugging with @jcrist1 ), but I can also comment out that test for now and figure that out in a subsequent PR. The other types of arguments work.

emarteca · 2024-08-20T17:54:40Z

Ok I figured out the issue with struct arguments -- just going to push a fix, and then we should be good to go with this PR

Manishearth · 2024-08-20T18:30:59Z

Still needs a regen

jcrist1

@emarteca I assume your comment

Ok I figured out the issue with struct arguments -- just going to push a fix, and then we should be good to go with this PR

means you don't need any help debugging and all the tests are passing so I'm approving

means that no more help is needed

Manishearth · 2024-08-20T19:51:29Z

Merged. The dtors / GlobalRef stuff are still a todo

Manishearth · 2024-08-20T19:53:12Z

Followup: #648

emarteca · 2024-08-20T20:36:22Z

Thank you!! 🙏

emarteca force-pushed the callback_gen_kotlin branch from 077497f to 9b32ab3 Compare August 14, 2024 18:17

emarteca marked this pull request as ready for review August 14, 2024 18:18

jcrist1 reviewed Aug 14, 2024

View reviewed changes

tool/templates/kotlin/Struct.kt.jinja Outdated Show resolved Hide resolved

tool/src/kotlin/snapshots/diplomat_tool__kotlin__test__struct.snap Outdated Show resolved Hide resolved

Manishearth reviewed Aug 14, 2024

View reviewed changes

jcrist1 mentioned this pull request Aug 14, 2024

fix-kotlin - fix codegen #631

Merged

Manishearth reviewed Aug 15, 2024

View reviewed changes

tool/src/kotlin/mod.rs Show resolved Hide resolved

Manishearth approved these changes Aug 19, 2024

View reviewed changes

Ellen Arteca added 3 commits August 19, 2024 15:47

allowing callback params in opaques and enums

68e4415

fixing kotlin code gen

18ed917

more tests for kotlin

6cc037b

emarteca force-pushed the callback_gen_kotlin branch from fe7bb46 to 6cc037b Compare August 19, 2024 22:47

emarteca commented Aug 19, 2024

View reviewed changes

fixing case for named type callback input types

8c84e39

Manishearth approved these changes Aug 20, 2024

View reviewed changes

Ellen Arteca added 2 commits August 20, 2024 11:24

snapshot test update

991d76d

clippy

bb4fb08

Ellen Arteca and others added 5 commits August 20, 2024 11:49

fix more tests

222bb64

test fix

ad47af5

test fix

0be4b18

test fix

584781d

regen

b6662ea

jcrist1 approved these changes Aug 20, 2024

View reviewed changes

Manishearth merged commit 6290ef6 into rust-diplomat:main Aug 20, 2024
16 checks passed

Manishearth mentioned this pull request Aug 20, 2024

Followup: Kotlin callback destructors #648

Open

		val cb: (CallbackTestingStruct) -> Int = { s -> s.x + s.y}
		val calledBack = CallbackWrapper.testCbWithStruct(cb)

Code generation for callback support in Kotlin #629

Code generation for callback support in Kotlin #629

Conversation

emarteca commented Aug 14, 2024 • edited Loading

emarteca commented Aug 14, 2024

Manishearth commented Aug 14, 2024

jcrist1 commented Aug 14, 2024

Manishearth commented Aug 14, 2024

jcrist1 left a comment • edited Loading

Choose a reason for hiding this comment

jcrist1 left a comment

Choose a reason for hiding this comment

Manishearth Aug 14, 2024

Choose a reason for hiding this comment

emarteca Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Manishearth Aug 15, 2024

Choose a reason for hiding this comment

jcrist1 Aug 15, 2024

Choose a reason for hiding this comment

jcrist1 Aug 15, 2024

Choose a reason for hiding this comment

emarteca Aug 15, 2024

Choose a reason for hiding this comment

emarteca commented Aug 15, 2024

emarteca commented Aug 15, 2024 • edited Loading

jcrist1 commented Aug 15, 2024

jcrist1 commented Aug 15, 2024

Manishearth commented Aug 15, 2024

jcrist1 commented Aug 15, 2024

emarteca commented Aug 15, 2024 • edited Loading

emarteca commented Aug 15, 2024

jcrist1 commented Aug 16, 2024

jcrist1 commented Aug 16, 2024 • edited Loading

emarteca commented Aug 16, 2024

jcrist1 commented Aug 19, 2024

jcrist1 commented Aug 19, 2024

emarteca commented Aug 19, 2024

jcrist1 commented Aug 19, 2024

jcrist1 commented Aug 19, 2024

Manishearth left a comment

Choose a reason for hiding this comment

jcrist1 commented Aug 19, 2024

emarteca commented Aug 19, 2024

emarteca Aug 19, 2024

Choose a reason for hiding this comment

emarteca commented Aug 19, 2024 • edited Loading

jcrist1 commented Aug 20, 2024

emarteca commented Aug 20, 2024 • edited Loading

Manishearth commented Aug 20, 2024

Manishearth commented Aug 20, 2024

Manishearth commented Aug 20, 2024

emarteca commented Aug 20, 2024

emarteca commented Aug 20, 2024

emarteca commented Aug 20, 2024

Manishearth commented Aug 20, 2024

jcrist1 left a comment

Choose a reason for hiding this comment

Manishearth commented Aug 20, 2024

Manishearth commented Aug 20, 2024

emarteca commented Aug 20, 2024

emarteca commented Aug 14, 2024 •

edited

Loading

jcrist1 left a comment •

edited

Loading

emarteca Aug 15, 2024 •

edited

Loading

emarteca commented Aug 15, 2024 •

edited

Loading

emarteca commented Aug 15, 2024 •

edited

Loading

jcrist1 commented Aug 16, 2024 •

edited

Loading

emarteca commented Aug 19, 2024 •

edited

Loading

emarteca commented Aug 20, 2024 •

edited

Loading