Skip to content

Workspace broken | UniFFI generates invalid Swift & Python bindings #2153

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

Open
Sajjon opened this issue Jun 14, 2024 · 17 comments
Open

Workspace broken | UniFFI generates invalid Swift & Python bindings #2153

Sajjon opened this issue Jun 14, 2024 · 17 comments

Comments

@Sajjon
Copy link
Contributor

Sajjon commented Jun 14, 2024

(@bendk asked me to create an issue )

Following up on my comment in 2087, using UniFFI with a Workspace works well in the land of Rust - especially with the new system for remote / custom / external types laid out in #2087.

However, when one generates Swift bindings, no cigar, the Swift code in each file tries to reference (file)private symbols such as lift/lower` on types in the other file - which are not visible. The Kotlin bindings, however, does seem to work. The Kotlin Bindgen test in my small UniFFI Workspace demo project Lemon Meringue Pie passes. I have not tried doing an actual Kotlin bindgen generation besides the bindgen test.

The Python bindings seems broken, for the small Python Bindgen tests in Lemon Meringue Pie, I get error:

    from .common import Uuid
ImportError: attempted relative import with no known parent package

We cannot simply change the access modifier to internal in the generated Swift code since each generated Swift file contains the same types, such as fileprivate protocol FfiConverterRustBuffer.

In this issue I lay out a straightforward idea on how to fix the broken Swift bindings specifically. I do not know enough about Python to lay out a plan for it, but perhaps the say idea is viable also for Python?

Idea of solution for Swift bindings

My idea is to:

  1. All generated Converters will be placed in separate files, in lets call it generated/ folder (as specified by --out-dir), and only be generated once, i.e. the contents of template/StringHelper.swift will be put Generated/ScaffoldingUniFFI folder
  2. The actual type - fileprivate struct FfiConverterString: FfiConverter - gets a changed visibility to internal struct instead of fileprivate.
  3. All protocols, types, functions and methods in RustBufferTemplate (and similar templates) get changed visibility to internal too
  4. Bindgen code for a single crate only contain the author written UniFFI code in one file per crate, places in Generated/Foo, Generated/Bar, Generated/Buzz, where Foo, Bar, `Buzz are the names of each of the users UniFFI exported crates.

This is a straightforward solution which should not involve too much changes here in UniFFI.

The only downside of this solution I can think of is that some users of UniFFI might need to migrate their build scripts from mv $out/Foobar.swift to `mv $out/Generated (or similar).

@mhammond
Copy link
Member

(@bendk asked me to create an issue )

Following up on my comment in 2087, using UniFFI with a Workspace works well in the land of Rust - especially with the new system for remote / custom / external types laid out in #2087.

However, when one generates Swift bindings, no cigar,

I don't really understand the problem you are describing - how is this different from what we do in our tests and examples?

The Python bindings seems broken, for the small Python Bindgen tests in Lemon Meringue Pie, I get error:

    from .common import Uuid
ImportError: attempted relative import with no known parent package

This looks just like https://mozilla.github.io/uniffi-rs/latest/python/configuration.html#external-packages - uniffi doesn't attempt to generate the complete package structure and __init__.py etc files.

In this issue I lay out a straightforward idea on how to fix the broken Swift bindings specifically.

Can you please try and clarify exactly what is broken for you, ideally by using our examples and fixtures?

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 14, 2024

@mhammond UniFFI does not support Swift bindgen of Cargo Workspace where CrateX depend in CrateY - where both CrateX and CrateY are UniFFI crates.

UniFFI generates broken Swift code.

@mhammond
Copy link
Member

We have a number of examples where CrateX depends on CrateY (eg, https://github.com/mozilla/uniffi-rs/tree/main/fixtures/ext-types) - I'm trying to understand what's different about your use-case which is broken vs that which apparently is not.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 14, 2024

Next week I can write a detailed report - with minimalistic code examples.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 25, 2024

Next week I can write a detailed report - with minimalistic code examples.

I did not forget, just have to focus on completing some tasks at work this week before my vacation, will get back to this next week!

@Sajjon
Copy link
Contributor Author

Sajjon commented Jul 11, 2024

@mhammond @bendk OK so I've created three small demo projects - projects which uses UniFFI with many crates having UniFFI as a dependency in a workspaces setup:

Repo Works? SemVer No of crates Description
Alpha ☑️ 0.28.0 2 Crate "two" references proc-macro exported types from crate "one", "imported" in UDL. Swift & Kotlin bindings tests works, Python does not compile.
Beta 0.28.0 2 Newtype does not work
Gamma ☑️ 0.28.0 3 Crate "one" references proc-macro exported types from crate "zero", "imported" in UDL. Crate "two" references types from "zero" and "one". Swift & Kotlin bindings tests works, Python does not compile.

Insights:

  • Python seems to not work at all with Workspace setup
  • Seems that newtype cannot be "imported" at all (see: repo "beta"). hopefully I'm simply missing something?
  • Other finding: UniFFI's binding test macro is excellent, the combined.modulemap merges many crates together, why do we not support this for prod? for the generate CLI?

Do you want me to create some demo projects using @bendk s fork (source branch of PR #2087 )?

@mhammond
Copy link
Member

I would like to see this reproduced in our ext-types fixture against main. There we have many crates, all in the top-level workspace, and all of which depend on each other in various ways, including directly and indirectly. It's also worth noting that what you describe above is exactly how mozilla uses uniffi in production.

Assuming Python doesn't compile due to an invalid package import, I believe your problem is that you don't have an appropriate uniffi.toml for your project. If true, I believe that would explain all the symptoms described. If flase, by working against that set of fixtures it would help to better understand what is different about your setup than our setups and form a great starting point for having our fixtures exercise that.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jul 13, 2024

@mhammond ok so here is an update:

  1. Workspace works with Python if one uses [bindings.python.external_packages]; crate_x = ""; crate_x = "" I had missed this completely, so my bad.
  2. newtypes can be imported across crates in workspace setup, by use of uniffi::ffi_converter_forward! macro call, however, please note that this macro is never mentioned in any documentation. I had used this macro many months ago but completely forgot about it. It seems to be the only way to get a proc-macro (custom_newtype!) newtype to be usable across crates in a workspace setup.
  3. What about my proposal of productifying the test macro's excellent combined.modulemap? It seems to be one missing piece in the Workspace puzzle.

@mhammond
Copy link
Member

newtypes can be imported across crates in workspace setup, by use of uniffi::ffi_converter_forward! macro call,

Oops - the docs should mention that, but I think that's going to go away with Ben's work.

What about my proposal of productifying the test macro's excellent combined.modulemap? It seems to be one missing piece in the Workspace puzzle.

While that seems fine, I'm not sure what problems you are actually seeing here. Or to put it another way, what are you trying to do that's different to our fixtures?

@kriswuollett
Copy link

I believe I've encountered this issue as well when trying to construct a Swift Package from a Rust Workspace.

Basically IMO types need to be namespace/package aware, e.g., ModuleA::TypeA1's dependency on ModuleB::TypeB1 needs to refer to it as ModuleB.TypeB1 not assume it can be referred to as TypeB1 in Swift code. Remove the assumption that types from different crates can be smashed into a single Swift module -- that may also alleviate some need for fixing the renaming types issue.

See code/situation in #2382 (comment). The final swift build step for me fails because the generated Swift files assume they are in the same Swift package target: error: cannot find 'FfiConverterTypeSomethingFromOtherPackage' in scope.

I'm using these versions at the moment:

[dependencies]
uniffi = { git = "http://github.com/mozilla/uniffi-rs", rev = "e6cb4a37cfd03ab053ab11324349feb280df3a22", features = [
    "cli",
    "default",
] }
uniffi_bindgen = { git = "http://github.com/mozilla/uniffi-rs", rev = "e6cb4a37cfd03ab053ab11324349feb280df3a22", features = [
    "default",
] }

@mhammond
Copy link
Member

It would be very helpful if you could explain how your use-case is different from our fixtures and/or demonstrate your problem in our fixtures.

@kriswuollett
Copy link

It would be very helpful if you could explain how your use-case is different from our fixtures and/or demonstrate your problem in our fixtures.

I created pull request #2396 that is a simplified bash version of the Rust code I have to build a Swift framework. It demonstrates what I believe to be the core issue that dependencies are not namespace aware, at least as far as I can tell in Swift, so one cannot put put the Swift code in different targets to avoid the other outstanding issue of naming clashes since can't name / prefix things.

Of course I could be using the library wrong and I'm missing supplying some option.

My preference for how things should work (without causing spam to linking to all the other issues) is:

  • Swift names are transformed to CamelCase (but okay not trying to figure out what segment is an "abbreviation" for all caps)
  • Configure a Swift name prefix for a type / everything in a mod, to avoid name conflicts even within a Rust package
  • Configure the expected Swift module name for the Swift code to avoid name conflicts across Swift package / Rust package

However even with that there still may be problems, maybe like this one if still relevant: https://forums.swift.org/t/ambiguous-use-of-a-same-name-var-or-func-of-extension-from-different-modules/58306.

If so perhaps the unappealing, but working, solution would be to generate Swift type names based on the package/mod path. Due to autocomplete, I don't really care if my type names are long, just want to not have to think how to adjust my Rust code to accommodate a FFI library too much.

@mhammond
Copy link
Member

I'm still a little confused though - are we maybe conflating 2 different problems here:

  • That the same name of an exported thing from two different crates will conflict in Swift - this is independent of xcode and is probably simple to demonstrate in our repo.
  • That it's not immediately obvious to Swift users how they might take our generated swift and use it in xcode. Your PR demonstrates that.

Is that accurate?

@kriswuollett
Copy link

Yes, that is accurate.

  • That it's not immediately obvious to Swift users how they might take our generated swift and use it in xcode. Your PR demonstrates that.

And also:

  • can't separate each FFI into separate Swift targets to avoid name conflicts

Finally for distribution which isn't necessarily Uniffi-related:

  • deciding how to distribute the XCFramework since I believe SPM has no "build.rs"-type step, either a HTTP URL without auth since binaryTarget remote auth seems broken, or local path dependency with something like git submodules and manual Rust build before final xcodebuild step

@Sajjon
Copy link
Contributor Author

Sajjon commented Jan 10, 2025

When I last tried using a multi-UniFII-crate setup last summer it worked beautifully for Kotlin.

Worked well for Swift with UniFFI bindgen tests but not for production. I later on realized it was because Uniffis bindgen test generation code merged all modulemaps into a combined, why it worked.

I think later last year a new Swift bindgen cli tool was released that does this (merged the modulemap of each rust crate into one combined). But I havent tried it yet.

@mhammond
Copy link
Member

And also:

* can't separate each FFI into separate Swift targets to avoid name conflicts

How is that different from the name collision I described?

Re the names: We've a few issues around swift naming which should be considered as a whole (so maybe we need one new meta issue?). I can't see why an option to rename swift items based on some kind of "pattern" wouldn't work (ie, being able to rename multiple items with a single config entry), which could probably deal with this. But ultimately I think it all will come down to "config" options, and having the swift bindings use them. We'd be happy to help someone having a go at this!

Re the new example, I'll need time to look in detail but conceptually I love it, thanks!

@kriswuollett
Copy link

How is that different from the name collision I described?

It probably is not different. What I was getting at, in probably a naive Swift way, was that perhaps qualifying the names could work like instead of code importing imported_types_lib and referring to the protocol as UniffiOneTrait, perhaps importing UniffiOne and referring to it as UniffiOne.UniffiOneTrait would work for some cases/types.

import imported_types_lib
import Foundation
// First step: implement a trait from an external crate in Swift and pass it to a function from this
// crate. This tests #2343 -- the codegen for this module needs to initialize the vtable from
// uniffi_one.
final class SwiftUniffiOneImpl: UniffiOneTrait {
func hello() -> String {
"Hello from Swift"
}
}
assert(invokeUniffiOneTrait(t: SwiftUniffiOneImpl()) == "Hello from Swift")

But seeing threads like https://forums.swift.org/t/fixing-modules-that-contain-a-type-with-the-same-name/3025/26, makes me think renaming/prefixing solutions may be the only thing that will easily/reliably work. I wouldn't mind just specifying a Swift name for a type in the macro, or even better an Objective-C style prefix for a given scope of types (crate/mod levels) since that would require less boilerplate and decision making.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants