Skip to content
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

Code generation for trait support in Kotlin #689

Merged
merged 4 commits into from
Sep 16, 2024

Conversation

emarteca
Copy link
Contributor

Code generation for trait support in the Kotlin backend.

We added trait support to Diplomat's internal representation and the C backend in this PR. The trait design is described in our design doc.

@emarteca emarteca marked this pull request as ready for review September 13, 2024 19:34
@emarteca
Copy link
Contributor Author

@jcrist1
Also re: your comments about destructors WRT callbacks, we've implemented the same strategy here for traits, where the destructor cleans up the VTable.

Copy link
Contributor

@jcrist1 jcrist1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look at the destructor code and it makes sense. I just want to check that I understand it:

Scenario 1

  • We create an obj in kotlin that implments TesterTrait, t
  • We pass that to fromTraitObj to instantiate the native struct which I'll call tVTable
  • This increments a global ref for the and tVTable holds a reference to t in its methods
  • We pass tVTable into a native method which immediately returns say an int,
  • this method consumes tVTable and drops it
  • this calls the dtor and the global jni reference is removed / decremented
  • this means GC can also clean up t (if nothing else is pointing to it)

Scenario 2

Can we do the following?

  • We create an obj in kotlin that implments TesterTrait, t
  • We pass that to fromTraitObj to instantiate the native struct which I'll call tVTable
  • This increments a global ref for the and tVTable holds a reference to t in its methods
  • We pass tVTable into a native method which immediately returns say an int,
  • this method boxes the trait internally and returns an opaque wrapper around it (say OpaqueTesterTrait(Box<dyn TesterTrait>))
  • the reference stays around until the returned opaque is cleaned up.
  • when OpaqueTesterTrait is cleaned up by the JVM it calls the drop method on the opaque which calls drop on the internal boxed dyn trait (I'm a little uncertain of how cleanup runs on trait objects). This I assume would call the dtor and decrement/remove the global reference to tVTable in the JVM, allowing cleanup of t as well

That sounds good to me 😄

Then I think we could make it much more ergonomic by making kotlin methods take the TesterTrait directly insteat of DiplomatTrait_TesterTrait_Wrapper

Finally I've got some style comments. In general it would be nice if the generated public api followed the kotlin style convention of camel case. I feel like it would make adoption more likely for kotlin users.

tool/src/kotlin/mod.rs Outdated Show resolved Hide resolved
.enumerate()
.map(|(index, param)| {
if let Some(param_name) = &param.name {
param_name.to_string()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (can probably use the formatter)

Type::ImplTrait(ref trt) => {
let trait_id = trt.id();
let resolved = self.tcx.resolve_trait(trait_id);
format!("DiplomatTrait_{}_Wrapper_Native", resolved.name).into()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the kotlin traits

Suggested change
format!("DiplomatTrait_{}_Wrapper_Native", resolved.name).into()
format!("DiplomatTrait{}WrapperNative", resolved.name).into()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left them as-is because now these types are internal, and the user would only interact with the trait itself, which just has the original name from rust (TesterTrait in this example)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm still not a fan of breaking naming conventions

@jcrist1 if you would like to do some uniform renaming I think that would be welcome.

tool/src/kotlin/mod.rs Show resolved Hide resolved
tool/src/kotlin/mod.rs Show resolved Hide resolved
@emarteca
Copy link
Contributor Author

emarteca commented Sep 13, 2024

Thanks for the comments -- yes, that is how the destructors work! A trait object t can only be GCd on the JVM side once drop has been called on the vtable pointer on the Rust side, and if the trait object passed in from Kotlin is boxed up into some wrapper then drop will only get called when this wrapper is dropped.

I'll go through and fix your code change suggestions now!

Copy link
Contributor

@jcrist1 jcrist1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice!

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also have a runnable test for this

Type::ImplTrait(ref trt) => {
let trait_id = trt.id();
let resolved = self.tcx.resolve_trait(trait_id);
format!("DiplomatTrait_{}_Wrapper_Native", resolved.name).into()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm still not a fan of breaking naming conventions

@jcrist1 if you would like to do some uniform renaming I think that would be welcome.

@Manishearth Manishearth merged commit de87900 into rust-diplomat:main Sep 16, 2024
20 checks passed
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.

3 participants