Allow for arbitrary BindingGenerator #1991

SalvatoreT · 2024-02-11T19:31:58Z

De-couple TargetLanguage from the generate_external_bindings calls. This will allow 3rd-party BindingGenerators to use the methods.

I'm not 100% certain this is the way to go, and I'm very open to feedback. cc @bendk @mhammond

Related to #299

mhammond

This is looking great, thanks! I think the uniffi-example-custom-types package tests are failing too, but I suspect that will be shallow. I wish all the config stuff was simpler :(

uniffi/src/cli.rs

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs

SalvatoreT · 2024-03-04T00:45:52Z

Updated, @mhammond!

mhammond

This looks great but CI is failing - I'm guessing there's some regression in the Config handling somewhere?

fixtures/docstring-proc-macro/tests/test_generated_bindings.rs

fixtures/docstring/tests/test_generated_bindings.rs

mhammond · 2024-03-18T03:21:19Z

I'd love to get this in the next release if we can get CI green.

SalvatoreT · 2024-03-18T04:40:27Z

I'm struggling with this one.

❯ cargo test -p unary-result-alias --test test_generated_bindings
   Compiling serde v1.0.186
   Compiling memchr v2.5.0
   Compiling goblin v0.8.0
   Compiling nom v7.1.3
   Compiling weedle2 v5.0.0 (/Users/sal/Developer/uniffi-rs/weedle2)
   Compiling camino v1.1.6
   Compiling serde_json v1.0.105
   Compiling semver v1.0.18
   Compiling cargo-platform v0.1.3
   Compiling basic-toml v0.1.4
   Compiling toml v0.5.11
   Compiling bincode v1.3.3
   Compiling uniffi_core v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_core)
   Compiling askama_derive v0.12.1
   Compiling cargo_metadata v0.15.4
   Compiling uniffi_macros v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_macros)
   Compiling uniffi_testing v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_testing)
   Compiling uniffi_udl v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_udl)
   Compiling askama v0.12.0
   Compiling uniffi_bindgen v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_bindgen)
   Compiling uniffi_build v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi_build)
   Compiling uniffi v0.26.1 (/Users/sal/Developer/uniffi-rs/uniffi)
   Compiling unary-result-alias v0.1.0 (/Users/sal/Developer/uniffi-rs/fixtures/regressions/unary-result-alias)
    Finished test [unoptimized + debuginfo] target(s) in 9.07s
     Running tests/test_generated_bindings.rs (target/debug/deps/test_generated_bindings-4938ae4946b33a5e)

running 3 tests
   Compiling unary-result-alias v0.1.0 (/Users/sal/Developer/uniffi-rs/fixtures/regressions/unary-result-alias)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
test uniffi_foreign_language_testcase_test_py ... ok
test uniffi_foreign_language_testcase_test_swift ... ok
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:20:16: error: unresolved reference: jna
import com.sun.jna.Library
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:21:16: error: unresolved reference: jna
import com.sun.jna.IntegerType
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:22:16: error: unresolved reference: jna
import com.sun.jna.Native
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:23:16: error: unresolved reference: jna
import com.sun.jna.Pointer
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:24:16: error: unresolved reference: jna
import com.sun.jna.Structure
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:25:16: error: unresolved reference: jna
import com.sun.jna.Callback
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:26:16: error: unresolved reference: jna
import com.sun.jna.ptr.*
               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:38:2: error: unresolved reference: Structure
@Structure.FieldOrder("capacity", "len", "data")
 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:39:25: error: unresolved reference: Structure
open class RustBuffer : Structure() {
                        ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:44:25: error: unresolved reference: Pointer
    @JvmField var data: Pointer? = null
                        ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:46:34: error: unresolved reference: Structure
    class ByValue: RustBuffer(), Structure.ByValue
                                 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:47:38: error: unresolved reference: Structure
    class ByReference: RustBuffer(), Structure.ByReference
                                     ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:65:64: error: unresolved reference: Pointer
        internal fun create(capacity: ULong, len: ULong, data: Pointer?): RustBuffer.ByValue {
                                                               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:81:13: error: unresolved reference: it
            it.order(ByteOrder.BIG_ENDIAN)
            ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:91:31: error: unresolved reference: ByReference
class RustBufferByReference : ByReference(16) {
                              ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:97:23: error: unresolved reference: getPointer
        val pointer = getPointer()
                      ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:107:23: error: unresolved reference: getPointer
        val pointer = getPointer()
                      ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:109:15: error: unresolved reference: writeField
        value.writeField("capacity", pointer.getLong(0))
              ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:110:15: error: unresolved reference: writeField
        value.writeField("len", pointer.getLong(8))
              ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:111:15: error: unresolved reference: writeField
        value.writeField("data", pointer.getLong(16))
              ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:123:2: error: unresolved reference: Structure
@Structure.FieldOrder("len", "data")
 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:124:27: error: unresolved reference: Structure
open class ForeignBytes : Structure() {
                          ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:126:25: error: unresolved reference: Pointer
    @JvmField var data: Pointer? = null
                        ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:128:37: error: unresolved reference: Structure
    class ByValue : ForeignBytes(), Structure.ByValue
                                    ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:167:17: error: unresolved reference: it
                it.order(ByteOrder.BIG_ENDIAN)
                ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:170:18: error: unresolved reference: writeField
            rbuf.writeField("len", bbuf.position().toLong())
                 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:208:2: error: unresolved reference: Structure
@Structure.FieldOrder("code", "error_buf")
 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:209:44: error: unresolved reference: Structure
internal open class UniffiRustCallStatus : Structure() {
                                           ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:213:44: error: unresolved reference: Structure
    class ByValue: UniffiRustCallStatus(), Structure.ByValue
                                           ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:350:35: error: unresolved reference: Library
private inline fun <reified Lib : Library> loadIndirect(
                                  ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:353:12: error: unresolved reference: Native
    return Native.load<Lib>(findLibraryName(componentName), Lib::class.java)
           ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:357:67: error: unresolved reference: jna
internal interface UniffiRustFutureContinuationCallback : com.sun.jna.Callback {
                                                                  ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:360:54: error: unresolved reference: jna
internal interface UniffiForeignFutureFree : com.sun.jna.Callback {
                                                     ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:363:58: error: unresolved reference: jna
internal interface UniffiCallbackInterfaceFree : com.sun.jna.Callback {
                                                         ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:366:2: error: unresolved reference: Structure
@Structure.FieldOrder("handle", "free")
 ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:370:5: error: unresolved reference: Structure
) : Structure() {
    ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:374:47: error: unresolved reference: Structure
    ): UniffiForeignFuture(`handle`,`free`,), Structure.ByValue
                                              ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:445:32: error: unresolved reference: Library
internal interface UniffiLib : Library {
                               ^
/Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa/uniffi/unary_result_alias/unary_result_alias.kt:554:8: error: unresolved reference: Pointer
    ): Pointer
       ^
test uniffi_foreign_language_testcase_test_kts ... FAILED

failures:

---- uniffi_foreign_language_testcase_test_kts stdout ----
Creating testing out_dir: /Users/sal/Developer/uniffi-rs/target/tmp/unary-result-alias-52ef0dc54a21b8aa
Error: running `kotlinc` failed


failures:
    uniffi_foreign_language_testcase_test_kts

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.18s

badboy · 2024-03-18T10:55:11Z

error: unresolved reference: jna

do you have jna.jar in your CLASSPATH?
For a local test run you might need to manually download it and set the environment variable.

mhammond · 2024-03-18T15:21:40Z

Yeah, I had to download jna-5.14.0.jar and ensure that's in my CLASSPATH. I don't quite understand why my installed Android Studio didn't come with what I needed, but here we are.

SalvatoreT · 2024-03-18T23:31:17Z

Adding jna-5.14.0.jar to the CLASSPATH does the trick. On to the next thing!

SalvatoreT · 2024-03-19T23:22:17Z

Okay, I figured out the bug!

Before

There's one combined Config that contains four languages inside its bindings field.

uniffi-rs/uniffi_bindgen/src/lib.rs

Lines 469 to 473 in c6ddcee

 #[derive(Debug, Clone, Default, Serialize, Deserialize)] 

 pub struct Config { 

 #[serde(default)] 

 bindings: bindings::Config, 

 }

uniffi-rs/uniffi_bindgen/src/bindings/mod.rs

Lines 92 to 102 in 6e4170d

 #[derive(Debug, Clone, Default, Serialize, Deserialize)] 

 pub struct Config { 

 #[serde(default)] 

 pub(crate) kotlin: kotlin::Config, 

 #[serde(default)] 

 pub(crate) swift: swift::Config, 

 #[serde(default)] 

 pub(crate) python: python::Config, 

 #[serde(default)] 

 pub(crate) ruby: ruby::Config, 

 }

The uniffi.toml files, group the different fields be the bindings.kotlin, bindings.swift, bindings.ruby, and bindings.python.

uniffi-rs/fixtures/docstring-proc-macro/uniffi.toml

Lines 1 to 9 in 45d0f34

 [bindings.kotlin] 

 package_name = "uniffi.fixture.docstring.proc.macro" 

 cdylib_name = "uniffi_fixture_docstring_proc_macro" 

 [bindings.python] 

 cdylib_name = "uniffi_fixture_docstring_proc_macro" 

 [bindings.swift] 

 cdylib_name = "uniffi_fixture_docstring_proc_macro"

After

The combined Config is no more, so the bindings.{language} keys aren't needed anymore.

I was getting an error because the package_name wasn't getting picked up. The following change fixed that.

--- a/examples/arithmetic/uniffi.toml
+++ b/examples/arithmetic/uniffi.toml
@@ -1,2 +1 @@
-[bindings.kotlin]
 package_name = "org.mozilla.uniffi.example.arithmetic"

Problem

I can go through almost all of the uniffi.toml files and remove bindings.{language}, and it works great. The only file that's a bit tricky is the examples/custom-types/uniffi.toml file which has three different custom_types.Url instances.

uniffi-rs/examples/custom-types/uniffi.toml

Lines 1 to 8 in 55139d6

 [bindings.swift.custom_types.Url] 

 # Name of the type in the Swift code 

 type_name = "URL" 

 # Modules that need to be imported 

 imports = ["Foundation"] 

 # Functions to convert between strings and URLs 

 into_custom = "URL(string: {})!" 

 from_custom = "String(describing: {})"

uniffi-rs/examples/custom-types/uniffi.toml

Lines 40 to 47 in 55139d6

 [bindings.kotlin.custom_types.Url] 

 # Name of the type in the Kotlin code 

 type_name = "URL" 

 # Classes that need to be imported 

 imports = [ "java.net.URI", "java.net.URL" ] 

 # Functions to convert between strings and URLs 

 into_custom = "URI({}).toURL()" 

 from_custom = "{}.toString()"

uniffi-rs/examples/custom-types/uniffi.toml

Lines 49 to 57 in 55139d6

 [bindings.python.custom_types.Url] 

 # We're going to be the urllib.parse.ParseResult class, which is the closest 

 # thing Python has to a Url class. No need to specify `type_name` though, 

 # since Python is loosely typed. 

 # modules to import 

 imports = ["urllib.parse"] 

 # Functions to convert between strings and the ParsedUrl class 

 into_custom = "urllib.parse.urlparse({})" 

 from_custom = "urllib.parse.urlunparse({})"

Possible Solutions

Add back a language key to the various language's Configs.
Dynamically set the uniffi.toml file for each test run.
Rename custom_types to {language}_custom_types
Give up?

What do y'all think?

mhammond · 2024-03-20T13:19:22Z

To be clear, one option is to change, eg, [bindings.swift.custom_types.Url] to [bindings.custom_types.Url]? If so, I don't think that will fly for various reasons.

Add back a language key to the various language's Configs.

At face value, this sounds like "restore things to how they worked before", so IMO is a bit of a no-brainer here?

SalvatoreT · 2024-03-21T17:52:22Z

I added back the usage of BindingGeneratorDefault, so the various bindings.{language}.* configurations would work without a major change.

I still want to move try_format_code into the Config which will probably require parsing the uniffi.toml file much earlier and passing it's generated Config in to the #generate_bindings calls. I can do that on a separate PR though.

De-couple TargetLanguage from the `generate_external_bindings` calls. This will allow 3rd-party BindingGenerators to use the methods. The `try_format_code` boolean had to be punched through (for now) to keep the API as similar as possible. I think a follow-up action should be to move the boolean to the Config.

mhammond

Fantastic, thanks!

Follows up on mozilla#1991.

Previously there were internal `Config` details which only worked with builtin bindings types. This leans in to the `BindingGenerator` trait to make things less coupled with the builtin bindings and better for external bindings. Follows up on mozilla#1991.

Previously there were internal `Config` details which only worked with builtin bindings types. This leans in to the `BindingGenerator` trait to make things less coupled with the builtin bindings and better for external bindings. Follows up on #1991.

SalvatoreT requested a review from a team as a code owner February 11, 2024 19:31

SalvatoreT requested review from mhammond and removed request for a team February 11, 2024 19:31

mhammond requested changes Feb 18, 2024

View reviewed changes

uniffi/src/cli.rs Outdated Show resolved Hide resolved

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs Outdated Show resolved Hide resolved

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs Outdated Show resolved Hide resolved

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch from 0239a0d to 3253d99 Compare March 4, 2024 00:45

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch from 3253d99 to aaad31f Compare March 4, 2024 23:28

SalvatoreT requested a review from mhammond March 4, 2024 23:42

mhammond requested changes Mar 9, 2024

View reviewed changes

fixtures/docstring-proc-macro/tests/test_generated_bindings.rs Outdated Show resolved Hide resolved

fixtures/docstring/tests/test_generated_bindings.rs Outdated Show resolved Hide resolved

bendk added the v0.27 Issues/PRs for the 0.27 release label Mar 14, 2024

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch from aaad31f to 3ab534a Compare March 18, 2024 04:21

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch from 3ab534a to a3479ac Compare March 19, 2024 23:09

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch 2 times, most recently from d9e9269 to e4c9c70 Compare March 21, 2024 17:48

SalvatoreT force-pushed the salvatoret/allow-arbitrary-BindingGenerator branch from e4c9c70 to 439acbd Compare March 21, 2024 17:53

mhammond approved these changes Mar 22, 2024

View reviewed changes

mhammond merged commit 6b09f11 into mozilla:main Mar 22, 2024
5 checks passed

SalvatoreT deleted the salvatoret/allow-arbitrary-BindingGenerator branch March 28, 2024 00:06

mhammond added a commit to mhammond/uniffi-rs that referenced this pull request Apr 8, 2024

Remove a common "config" for the bindings.

baca5c2

Follows up on mozilla#1991.

mhammond mentioned this pull request Apr 8, 2024

Allow the binding generator to not depend on builtin bindings. #2066

Merged

mhammond added a commit to mhammond/uniffi-rs that referenced this pull request Apr 14, 2024

Remove a common "config" for the bindings.

961a82a

Follows up on mozilla#1991.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for arbitrary BindingGenerator #1991

Allow for arbitrary BindingGenerator #1991

SalvatoreT commented Feb 11, 2024

mhammond left a comment

SalvatoreT commented Mar 4, 2024

mhammond left a comment

mhammond commented Mar 18, 2024

SalvatoreT commented Mar 18, 2024

badboy commented Mar 18, 2024

mhammond commented Mar 18, 2024

SalvatoreT commented Mar 18, 2024

SalvatoreT commented Mar 19, 2024

mhammond commented Mar 20, 2024

SalvatoreT commented Mar 21, 2024

mhammond left a comment

Allow for arbitrary BindingGenerator #1991

Allow for arbitrary BindingGenerator #1991

Conversation

SalvatoreT commented Feb 11, 2024

mhammond left a comment

Choose a reason for hiding this comment

SalvatoreT commented Mar 4, 2024

mhammond left a comment

Choose a reason for hiding this comment

mhammond commented Mar 18, 2024

SalvatoreT commented Mar 18, 2024

badboy commented Mar 18, 2024

mhammond commented Mar 18, 2024

SalvatoreT commented Mar 18, 2024

SalvatoreT commented Mar 19, 2024

Before

After

Problem

Possible Solutions

mhammond commented Mar 20, 2024

SalvatoreT commented Mar 21, 2024

mhammond left a comment

Choose a reason for hiding this comment