Skip to content

[XA.Tools.Bytecode] Add Kotlin support to our binding process. #505

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

Merged
merged 5 commits into from
Nov 22, 2019

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Oct 17, 2019

Various fixes and enhancements to allow better default Kotlin libraries bindings.

  • Hide Kotlin-internal classes, constructors, and methods.
  • Hide implementation methods (*-impl*).
  • Rename extension method parameter like $this$decodeBase64 to obj.
  • Use Kotlin provided method parameter names instead of p0, p1, etc.
  • Rename any method with a hyphen-<hashcode> to drop the invalid part (add-H4uI21a -> add).
    • This is how Kotlin ensures add (UInt) and add (Int) don't clash when compiled to Java.

@jpobst jpobst force-pushed the kotlin-support branch 2 times, most recently from f9497c7 to 84ccb6d Compare October 17, 2019 15:19
@jpobst jpobst marked this pull request as ready for review November 5, 2019 21:31
@jpobst jpobst requested a review from jonpryor November 5, 2019 21:31
@mattleibow mattleibow self-requested a review November 6, 2019 20:26
@jpobst jpobst changed the title [WIP] Add Kotlin support to our binding process. [XA.Tools.Bytecode] Add Kotlin support to our binding process. Nov 8, 2019
@jonpryor
Copy link
Member

jonpryor commented Nov 21, 2019

The last bullet point gives me some pause:

Rename any method with a hyphen-<hashcode> to drop the invalid part (add-H4uI21a -> add).

Does this happen with virtual instance members?

I fear the answer is "yes".

Thus, consider the Kotlin code:

class ExampleBase {
	public open fun foo(value : Int) {
	}
	public open fun foo(value : UInt) {
	}
}

Compile & Disassemble it, and:

$ kotlinc hello.kt
$ javap  -s ExampleBase.class 
Compiled from "hello.kt"
public final class ExampleBase {
  public void foo(int);
    descriptor: (I)V

  public void foo-WZ4Q5Ns(int);
    descriptor: (I)V

  public ExampleBase();
    descriptor: ()V
}

Based on the bullet point, I think we'd bind this as:

[Register ("ExampleBase", DoNotGenerateAcw=true)]
partial class ExampleBase {
    [Register ("foo", ...)]
    public virtual void Foo (int value) {
        _members.InstanceMethods.InvokeVirtualVoidMethod ("foo.(I)V", this, ...);
    }

    [Register ("foo-WZ4Q5Ns", ...)]
    public virtual void Foo (int value) {
        _members.InstanceMethods.InvokeVirtualVoidMethod ("foo-WZ4Q5Ns.(I)V", this, ...);
    }
}

At which point I scratch my head.

For starters, that can't compile: there's two Foo(int) methods.

Even if it could compile, the ExampleBase.foo(UInt) method cannot be overridden from C#, because we use Java as an intermediary:

// C#
class MyDerivedExample : ExampleBase {
    // Assume we "somehow" override the foo(UInt) overload
    public override void Foo (int value) {
    }
}

There's not actually any way to override ExampleBase.foo(UInt).

Now, if we bound ExampleBase.foo(UInt) as a non-virtual method, we avoid this conundrum, and we allow it to be invoked, because JniPeerMembers doesn't care about dashes and such.

As-is, though, I cannot tell what this PR would do with the above Kotlin class. Is this already handled and I just don't see it? Should it be addressed?

@jpobst
Copy link
Contributor Author

jpobst commented Nov 21, 2019

Yeah, that appears to be an issue that we will need to think through and fix. Perhaps if we have a foo (int) and a foo-abcdefa (int) we can just drop the second one? Or we could make the assumption that it's uint and generate foo (uint)?

Given that it is likely a rare corner case (that could be fixable via metadata if encountered) my inclination is to commit this as-is and add a new issue for this.

@jonpryor jonpryor merged commit 439bd83 into master Nov 22, 2019
@jonpryor jonpryor deleted the kotlin-support branch November 22, 2019 01:02
jonpryor pushed a commit that referenced this pull request Dec 4, 2019
Context: #525

Various fixes and enhancements to allow better default Kotlin
libraries bindings:

  * Hide Kotlin-internal classes, constructors, and methods.

  * Hide implementation methods (`*-impl*`).

  * Rename extension method parameter like
    `$this$decodeBase64` to `obj`. 

  * Use Kotlin provided method parameter names instead of
    `p0`, `p1`, etc.

  * Rename any method with a `-<mangling>` to drop the invalid part,
    e.g. `add-H4uI21a` is bound as `add`.
    ("Name mangling" like this is how Kotlin ensures `add(UInt)` and
    `add(Int)` don't clash when compiled to [Java][1].)

Note that the final bullet point -- "removing" name mangling -- may
result in C# code which is either not valid or not entirely usable.
See Issue #525 for details.  This will be addressed "later".

[1]: https://kotlinlang.org/docs/reference/inline-classes.html#mangling
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants